Re: [PATCH v4 05/25] reboot: Warn if restart handler has duplicated priority

[Michał Mirosław <mirq-linux@xxxxxxxxxxxx>]

On Mon, Nov 29, 2021 at 12:06:19AM +0300, Dmitry Osipenko wrote:
> 28.11.2021 03:28, Michał Mirosław пишет:
> > On Fri, Nov 26, 2021 at 09:00:41PM +0300, Dmitry Osipenko wrote:
> >> Add sanity check which ensures that there are no two restart handlers
> >> registered with the same priority. Normally it's a direct sign of a
> >> problem if two handlers use the same priority.
> > 
> > The patch doesn't ensure the property that there are no duplicated-priority
> > entries on the chain.
> 
> It's not the exact point of this patch.
> 
> > I'd rather see a atomic_notifier_chain_register_unique() that returns
> > -EBUSY or something istead of adding an entry with duplicate priority.
> > That way it would need only one list traversal unless you want to
> > register the duplicate anyway (then you would call the older
> > atomic_notifier_chain_register() after reporting the error).
> 
> The point of this patch is to warn developers about the problem that
> needs to be fixed. We already have such troubling drivers in mainline.
> 
> It's not critical to register different handlers with a duplicated
> priorities, but such cases really need to be corrected. We shouldn't
> break users' machines during transition to the new API, meanwhile
> developers should take action of fixing theirs drivers.
> 
> > (Or you could return > 0 when a duplicate is registered in
> > atomic_notifier_chain_register() if the callers are prepared
> > for that. I don't really like this way, though.)
> 
> I had a similar thought at some point before and decided that I'm not in
> favor of this approach. It's nicer to have a dedicated function that
> verifies the uniqueness, IMO.

I don't like the part that it traverses the list second time to check
the uniqueness. But actually you could avoid that if
notifier_chain_register() would always add equal-priority entries in
reverse order:

 static int notifier_chain_register(struct notifier_block **nl,
 		struct notifier_block *n)
 {
 	while ((*nl) != NULL) {
 		if (unlikely((*nl) == n)) {
 			WARN(1, "double register detected");
 			return 0;
 		}
-		if (n->priority > (*nl)->priority)
+		if (n->priority >= (*nl)->priority)
 			break;
 		nl = &((*nl)->next);
 	}
 	n->next = *nl;
 	rcu_assign_pointer(*nl, n);
 	return 0;
 }

Then the check for uniqueness after adding would be:

 WARN(nb->next && nb->priority == nb->next->priority);

Best Regards
Michał Mirosław