From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 3C2E9C4167B for ; Fri, 10 Dec 2021 19:14:49 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S238278AbhLJTSW convert rfc822-to-8bit (ORCPT ); Fri, 10 Dec 2021 14:18:22 -0500 Received: from mail-ot1-f54.google.com ([209.85.210.54]:36352 "EHLO mail-ot1-f54.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231135AbhLJTSV (ORCPT ); Fri, 10 Dec 2021 14:18:21 -0500 Received: by mail-ot1-f54.google.com with SMTP id w6-20020a9d77c6000000b0055e804fa524so10607332otl.3; Fri, 10 Dec 2021 11:14:45 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=StDkoZeh1lHUw1DDm/KpSH6johK86ICDiu5dChogaP8=; b=8RVAJpIyNs4Dba7Od5GgfN2aEtWz2FBM6CbC2ZBZQe7Sbyp/bZFJINfXbj8q0V3lb0 iWuq+JAcSTr/r3hfTTW18xc0aJ8sacN3j7JUswGQ4cWu9glG5gWQRPmU0uVEwu3ESIrO wfPzQvoPi+wgsMyolB5VY3FCfZ0iy0HHUXiKmU1/MuF8Jh5GY2NNG7zwbeAPuQzPrOjb 831NkzP9H/l0Oey3jpcg53MnOFXO0BMTZLdDaPe0CWrooLhvj9VV6VClntbHuiZhZLiU JAGmAex1rHn2yofKlBA3gxj5zYBQExyKAidO9j1QTLHEf6ICgdRb5sl1sFiLPXCJM1ZL d/Xw== X-Gm-Message-State: AOAM531Ul0m2YoPCR25bw+CkKm7y5Rv5fmzoHrqxIP4u0lEicPIA6Eqk GNPdni0XPikcr9N+KhaPxwKmuo0ducvNExbaNkE= X-Google-Smtp-Source: ABdhPJwI6PbHk846M9cWi0KvS3EvMhQYMsgwdseNjfmW3x73dR5HGBiuvU5f7TYKM/YR00y87Pqhei6Yd6KAs40K6Eo= X-Received: by 2002:a05:6830:1e57:: with SMTP id e23mr12575530otj.16.1639163685443; Fri, 10 Dec 2021 11:14:45 -0800 (PST) MIME-Version: 1.0 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: <45025b2d-4be1-f694-be61-31903795cf5d@gmail.com> From: "Rafael J. Wysocki" Date: Fri, 10 Dec 2021 20:14:34 +0100 Message-ID: Subject: Re: [PATCH v4 05/25] reboot: Warn if restart handler has duplicated priority To: Dmitry Osipenko Cc: "Rafael J. Wysocki" , =?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 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8BIT Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 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?