RE: [PATCH] kernel/notifier.c: remove notifier_chain_register

[Nixiaoming]

On Fri, 14 Jun 2019 03:38 AM Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> wrote:
>On Thu, 13 Jun 2019 22:07:44 +0800 Xiaoming Ni <nixiaoming@xxxxxxxxxx> wrote:
>
>> Registering the same notifier to a hook repeatedly can cause the hook
>> list to form a ring or lose other members of the list.
>> .....
>> 
>> diff --git a/kernel/notifier.c b/kernel/notifier.c
>> index d9f5081..56efd54 100644
>> --- a/kernel/notifier.c
>> +++ b/kernel/notifier.c
>> @@ -19,20 +19,6 @@
>>   *	are layered on top of these, with appropriate locking added.
>>   */
>>  
>> -static int notifier_chain_register(struct notifier_block **nl,
>> -		struct notifier_block *n)
>> -{
>> -	while ((*nl) != NULL) {
>> -		WARN_ONCE(((*nl) == n), "double register detected");
>> -		if (n->priority > (*nl)->priority)
>> -			break;
>> -		nl = &((*nl)->next);
>> -	}
>> -	n->next = *nl;
>> -	rcu_assign_pointer(*nl, n);
>> -	return 0;
>> -}
>
>Registering an already-registered notifier is a bug (except for in
>net/sunrpc/rpc_pipe.c, apparently).  The effect of this change is to
>remove the warning about the presence of the bug, so the bug is less
>likely to get fixed.
>
thanks for your guidance,

Should I modify this way 
   1 notifier_chain_cond_register() and notifier_chain_register() should be combined into one function.
   2 The warning information needs to be displayed while prohibiting duplicate registration.
		@@ -23,7 +23,10 @@ static int notifier_chain_register(struct notifier_block **nl,
						struct notifier_block *n)
		 {
				while ((*nl) != NULL) {
		-           WARN_ONCE(((*nl) == n), "double register detected");
		+         if (unlikely((*nl) == n)) {
		+                 WARN(1, "double register detected");
		+                 return 0;
		+         }
						if (n->priority > (*nl)->priority)
								break;

>I think it would be better to remove notifier_chain_cond_register() and
>blocking_notifier_chain_cond_register() and to figure out why
>net/sunrpc/rpc_pipe.c is using it and to redo the rpc code so it no
>longer has that need.
>
thanks for your guidance,
I re-examine the submission record and analyze it as follows

notifier_chain_cond_register() was introduced by commit 6546bc4279241e8fa43
 ("ipc: re-enable msgmni automatic recomputing msgmni if ââset to negative")
From the patch description information, it should be done to avoid repeated registrations,
 but I don't know why not directly modify notifier_chain_cond_register().
 
notifier_chain_cond_register() is only called by blocking_notifier_chain_cond_register()
blocking_notifier_chain_cond_register() has less processing of the SYSTEM_BOOTING state 
than blocking_notifier_chain_egister().
may also be a bug.

ipc/ipcns_notifier.c and the call to blocking_notifier_chain_cond_register() are removed 
in commit 0050ee059f7fc86b1df252 ("ipc/msg: increase MSGMNI, remove scaling").

now blocking_notifier_chain_cond_register() is only used in net/sunrpc/rpc_pipe.c, 
commit 2d00131acc641b2cb6 ("SUNRPC: send notification events on pipefs sb creation and destruction")
Using blocking_notifier_chain_cond_register() may also be to avoid duplicate registrations??

thanks