Re: [PATCH] Notifier for significant events on i386

[Stephen Hemminger]

On Wed, 2002-12-11 at 23:34, Vamsi Krishna S . wrote:
> On Thu, Dec 12, 2002 at 12:25:47AM +0000, Stephen Hemminger wrote:
> > 
> > This patch changes notifier to use RCU.  No interface change, just a little
> > more memory in each notifier_block. Also some formatting cleanup.
> > Please review and give comments.
> > 
> > <snip patch>
> 
> This looks good. I have a few of comments:
> 
> - add read_lock_rcu() / read_unlock_rcu() around the loop in
>   notifier_call_chain() to be preempt-safe.
> 
> - I would suggest using struct list_head in the notifier_block
>   and use the RCU list routines from include/linux/list.h
>   instead of spreading subtle RCU memory-barrier black magic.

That would be good for a new interface, but the existing code depends on
the single linked behavior. Many initializer's are pre-C99 style, and
more importantly there is no distinction between a list element and a
list head.  To work with list macros the head has to be initialized
correctly.  It is better not to worry about changing the interface and
avoid having to change all the calling code.

The only advantage to the doubly-linked list (besides std macros) is
that it is possible to unregister without knowing the head. There was a
patch several months ago to do singly-linked list macros but it looks
like it never got accepted.  If the obscurity of the macro's is desired
then maybe the way to go is creating a slist.h with RCU extensions. 

> - Even though RCU list reading is lockless, premption needs to
>   be disabled while reading as mentioned above. So, we do
>   need an __notifier_call_chain() version for those handlers
>   that could sleep inside the handler: they will have to
>   handle the required locking themselves.

The use of notifier today is limited to things that can't sleep. As far
as I can tell, it is intended for system events like reboot, panic;
where sleeping doesn't make sense.  I think that is why the original
notifier_call_chain did not grab the read_lock.

-- 
Stephen Hemminger <[e-mail]>
Open Source Devlopment Lab