On Monday, May 07, 2012 12:35:31 Matthias Schiffer wrote:
On 05/06/2012 10:14 PM, Simon Wunderlich wrote:
Your patch does the trick, although I think this ugly function could use a rewrite. First counting bytes and then allocating this size to do exactly the same thing again is not really good style. If you would like to volunteer to do this job (or plan to work more on vis), please tell me, otherwise I'll put it in on my TODO list.
While I'am already at it, I guess I can also volunteer to do some more vis work :)
Great! The vis code will profit from some love. :)
Besides cleanup, are there more ideas about the vis? A nice feature I can think about would be adding some freely chosen identification string (for example the hostname) to the vis data, this could make big graphs much more readable. I wonder though if this would be possible without breaking compatiblity.
If you wish to replace the mac addresses with readable name you can simply use a bat-hosts file to tell batctl which mac address should be replaced. Check the bat-hosts section in our batctl online manpage: http://downloads.open-mesh.org/batman/manpages/batctl.8.html
- Is there any reason vis_seq_print_text() allocates a buffer at all
instead of just printing the data directy into the seq_file? Looking at the seq_printf implementation, there doesn't seem to be a problem calling it while holding the lock.
The output can directly printed only as long as no spinlock is held. Spinlocks are not allowed to sleep. All other *_print_text() functions have been converted to directly call seq_printf() after we converted the hash to use RCU locking instead of spinlocks. For this to work in the vis code as well the vis_hash_lock spinlock has to be removed ..
- In many places in the vis code hlist_for_each_entry_rcu() is used to
iterate over the hash lists, even though all access to vis_hash is guarded by the vis_hash_lock, so it seems to be okay to just use hlist_for_each_entry(). In some functions, vis_seq_print_text() being one of them, rcu_read_lock/unlock pairs could be removed as well with this change. Do I overlook something?
The vis code is only half way through the spinlock to RCU conversion. Before you remove the vis_hash_lock anywhere you have to ensure that the data structures are properly protected by other means. If half of the code uses one lock while the rest uses another we'll certainly run into trouble.
If you ask me what could/should be done, I'd say: * Replacing vis_hash_lock with RCU locking. * The vis code should be reviewed to remove code duplication. It implements quite a number of things already present in the main batman-adv code (for example its packet queue). * Boring code cleanups like using newly added macros / functions to make the code more readable.
I'll also drop by the batman IRC channel.
See you there!
Regards, Marek