On Monday, May 07, 2012 06:35:31 AM Matthias Schiffer wrote: [....]
I have some questions about the code though:
- 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.
This is something which came from an old... old... old implementation. It didn't use debugfs and seq_printf and therefore stupid tricks had to be used. Actually, the current implementation is broken and has to be changed (but no one wanted to touch the vis code).
- 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?
I think it would be better to reduce the spin-locking and change the code to use rcu_read_lock. But maybe we have to think a lot about the data structures to generate consistent output... so maybe it is not possible (when also wanting it implemented in an efficient way).
Kind regards, Sven