From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dmitry Osipenko Date: Fri, 10 Dec 2021 19:42:43 +0000 Subject: Re: [PATCH v4 05/25] reboot: Warn if restart handler has duplicated priority Message-Id: <45228c88-4d51-591e-5da5-9ec468e71684@gmail.com> List-Id: References: <20211126180101.27818-1-digetx@gmail.com> <20211126180101.27818-6-digetx@gmail.com> <033ddf2a-6223-1a82-ec64-30f17c891f67@gmail.com> <091321ea-4919-0579-88a8-23d05871575d@gmail.com> <45025b2d-4be1-f694-be61-31903795cf5d@gmail.com> In-Reply-To: MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit To: "Rafael J. Wysocki" Cc: =?UTF-8?B?TWljaGHFgiBNaXJvc8WCYXc=?= , Thierry Reding , Jonathan Hunter , Russell King , Catalin Marinas , Will Deacon , Guo Ren , Geert Uytterhoeven , Greg Ungerer , Joshua Thompson , Thomas Bogendoerfer , Sebastian Reichel , Linus Walleij , Philipp Zabel , Greentime Hu , Vincent Chen , "James E.J. Bottomley" , Helge Deller , Michael Ellerman , Benjamin Herrenschmidt , Paul Mackerras , Paul Walmsley , Palmer Dabbelt , Albert Ou , Yoshinori Sato , Rich Felker , Thomas Gleixner , Ingo Molnar , Borislav Petkov , Dave Hansen , the arch/x86 maintainers , "H. Peter Anvin" , Boris Ostrovsky , Juergen Gross , Stefano Stabellini , Len Brown , Santosh Shilimkar , Krzysztof Kozlowski , Liam Girdwood , Mark Brown , Pavel Machek , Lee Jones , Andrew Morton , Guenter Roeck , Daniel Lezcano , Andy Shevchenko , Ulf Hansson , alankao@andestech.com, "K . C . Kuen-Chern Lin" , Linux ARM , Linux Kernel Mailing List , linux-csky@vger.kernel.org, linux-ia64@vger.kernel.org, linux-m68k@lists.linux-m68k.org, linux-mips@vger.kernel.org, linux-parisc@vger.kernel.org, linuxppc-dev , linux-riscv@lists.infradead.org, Linux-sh list , xen-devel@lists.xenproject.org, ACPI Devel Maling List , Linux PM , linux-tegra 10.12.2021 22:14, Rafael J. Wysocki пишет: > On Fri, Dec 10, 2021 at 8:04 PM Dmitry Osipenko wrote: >> >> 10.12.2021 21:27, Rafael J. Wysocki пишет: >>> On Mon, Nov 29, 2021 at 12:34 PM Dmitry Osipenko wrote: >>>> >>>> 29.11.2021 03:26, Michał Mirosław пишет: >>>>> 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); >>>> >>>> We can't just change the registration order because invocation order of >>>> the call chain depends on the registration order >>> >>> It doesn't if unique priorities are required and isn't that what you want? >>> >>>> and some of current >>>> users may rely on that order. I'm pretty sure that changing the order >>>> will have unfortunate consequences. >>> >>> Well, the WARN() doesn't help much then. >>> >>> Either you can make all of the users register with unique priorities, >>> and then you can make the registration reject non-unique ones, or you >>> cannot assume them to be unique. >> >> There is no strong requirement for priorities to be unique, the reboot.c >> code will work properly. > > In which case adding the WARN() is not appropriate IMV. > > Also I've looked at the existing code and at least in some cases the > order in which the notifiers run doesn't matter. I'm not sure what > the purpose of this patch is TBH. The purpose is to let developer know that driver needs to be corrected. >> The potential problem is on the user's side and the warning is intended >> to aid the user. > > Unless somebody has the panic_on_warn mentioned previously set and > really the user need not understand what the WARN() is about. IOW, > WARN() helps developers, not users. > >> We can make it a strong requirement, but only after converting and >> testing all kernel drivers. > > Right. > >> I'll consider to add patches for that. > > But can you avoid adding more patches to this series? I won't add more patches since such patches can be added only after completion of transition to the new API of the whole kernel.