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 kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 48141ECAAA3 for ; Fri, 26 Aug 2022 01:44:11 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id C88516B0074; Thu, 25 Aug 2022 21:44:10 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id C0FC96B0075; Thu, 25 Aug 2022 21:44:10 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id A8958940007; Thu, 25 Aug 2022 21:44:10 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0014.hostedemail.com [216.40.44.14]) by kanga.kvack.org (Postfix) with ESMTP id 936576B0074 for ; Thu, 25 Aug 2022 21:44:10 -0400 (EDT) Received: from smtpin17.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay04.hostedemail.com (Postfix) with ESMTP id BFCA01A063F for ; Fri, 26 Aug 2022 01:44:09 +0000 (UTC) X-FDA: 79840048218.17.083C0A9 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by imf29.hostedemail.com (Postfix) with ESMTP id 9454F120008 for ; Fri, 26 Aug 2022 01:44:08 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1661478248; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=p5oPV+olrHUkMDvetbVRMjHtVUhIxR2PwsRxaCXUCXA=; b=QNwXQkii/uretsmOqu5m/KZTdIz+FTAphV7Oe2X7Hi0sX4o8CvEIbfa3skN3oqcF+kRrlj 0RCd+wbKiPiUmklB0fkQu7OKQ7FweqKVX+bDpVvoz4z+YL0EqoDJp4BFKhqGfEN4Zo6itT 7n25X15ckdV41vd9lYZ9KgnDAxnkTt8= Received: from mail-ed1-f70.google.com (mail-ed1-f70.google.com [209.85.208.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-612-_mmnZsXnO-Cvve3HEkEiZw-1; Thu, 25 Aug 2022 21:44:07 -0400 X-MC-Unique: _mmnZsXnO-Cvve3HEkEiZw-1 Received: by mail-ed1-f70.google.com with SMTP id p4-20020a056402500400b00447e8b6f62bso253557eda.17 for ; Thu, 25 Aug 2022 18:44:06 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc; bh=p5oPV+olrHUkMDvetbVRMjHtVUhIxR2PwsRxaCXUCXA=; b=D2jAKk+dzKwN/gvhEo0qasbQkVkqIf2vhubrX9ns3U0EgHBTXKnlIN75HP1Ggy4IDF f/bObQEzxfhksYVGhCnzqDQIuGarhppgIuXXL84T1qb3EnrlWeXIz0vjkVnpT9B21pFd VtYQPRv86jsg29U2b7kTpH1BfQY2vf8obOqJQu07n6pREXPZFCEka/hBLZWRu/imXlzr ULFlH1cHrUBWC5U9OdRue7zRej9qro4lOTsgJgqvFBtiM5288VmcHIY8BLbb8J5z0rc/ 3flvZiQzvUf4DU8AaX0n1n5R8uhK7Aj4HN3mxy+AElHgHMbXemGJ8uUZBtn3ByFVTDue UcFw== X-Gm-Message-State: ACgBeo3S8ZHcnSQMPhwF8WDurfhntauMFiOhUUEE/YB0jmMY0SjkCVvo bsQO4i8B++jbv5GiXUvVd9j+6LeFNiX6IpAeeDTpFvPaAVu2XvyrE6pfKidRfPCQgbmjLM7Rgrf EhAH3KGEDzKxFXOF6livxoJJgfnY= X-Received: by 2002:a05:6402:3714:b0:445:d91b:b0aa with SMTP id ek20-20020a056402371400b00445d91bb0aamr4965852edb.313.1661478245892; Thu, 25 Aug 2022 18:44:05 -0700 (PDT) X-Google-Smtp-Source: AA6agR5Gd3CcJVFzRVYY80RJx7gEeoPX/9eoVrzEpfthId6lHxXKuIxINBj+TfI9cvakWVM3GP+yI0ItM3UAtuh+2is= X-Received: by 2002:a05:6402:3714:b0:445:d91b:b0aa with SMTP id ek20-20020a056402371400b00445d91bb0aamr4965825edb.313.1661478245607; Thu, 25 Aug 2022 18:44:05 -0700 (PDT) MIME-Version: 1.0 References: <20220824163100.224449-1-david@redhat.com> <20220824163100.224449-2-david@redhat.com> <0db131cf-013e-6f0e-c90b-5c1e840d869c@nvidia.com> In-Reply-To: From: Dave Young Date: Fri, 26 Aug 2022 09:43:54 +0800 Message-ID: Subject: Re: [PATCH RFC 1/2] coding-style.rst: document BUG() and WARN() rules ("do not crash the kernel") To: David Hildenbrand Cc: John Hubbard , linux-kernel@vger.kernel.org, linux-mm@kvack.org, linux-doc@vger.kernel.org, kexec@lists.infradead.org, Linus Torvalds , Andrew Morton , Ingo Molnar , David Laight , Jonathan Corbet , Andy Whitcroft , Joe Perches , Dwaipayan Ray , Lukas Bulwahn , Baoquan He , Vivek Goyal , Stephen Johnston , Prarit Bhargava X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1661478249; a=rsa-sha256; cv=none; b=1Awt1pqHtAp7iXCcphPlCp98ZbjCUCn7O1qIV9hCb8rdM4Chc9T/oujjqk1H3QsHHF4kHf q6A5GQdw1dXpZDI6//n6dPNC4KP0ycmlsFiEVdaSlf2S6p9czBZlMUeTV8jZzQ3OA2v+9M 8FBgpCL09h7ZluLP3AIvt8QmikeaoiE= ARC-Authentication-Results: i=1; imf29.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b=QNwXQkii; spf=pass (imf29.hostedemail.com: domain of ruyang@redhat.com designates 170.10.133.124 as permitted sender) smtp.mailfrom=ruyang@redhat.com; dmarc=pass (policy=none) header.from=redhat.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1661478249; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=p5oPV+olrHUkMDvetbVRMjHtVUhIxR2PwsRxaCXUCXA=; b=hiYOpntKTcigpZEICQn8fv0aKFgBvNLlN54xa0s1OyzknuEQTVPi0Ikej22ZQb/T4stFvr xpHox9x/y1TnI6ESClsUiPjj5FTyIjwUSnN56QY0qTF74Kn1oU1J53EN74JSCnXJuCBjdR PioOcRTJi2adjQowhDXlm49seGLMHZc= X-Stat-Signature: ybxbq3iospptzed45i48a6jz6mab1uou X-Rspamd-Queue-Id: 9454F120008 X-Rspam-User: X-Rspamd-Server: rspam06 Authentication-Results: imf29.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b=QNwXQkii; spf=pass (imf29.hostedemail.com: domain of ruyang@redhat.com designates 170.10.133.124 as permitted sender) smtp.mailfrom=ruyang@redhat.com; dmarc=pass (policy=none) header.from=redhat.com X-HE-Tag: 1661478248-898161 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: Hi David, [Added more people in cc] On Thu, 25 Aug 2022 at 20:13, David Hildenbrand wrote: > > On 24.08.22 23:59, John Hubbard wrote: > > On 8/24/22 09:30, David Hildenbrand wrote: > >> diff --git a/Documentation/process/coding-style.rst b/Documentation/process/coding-style.rst > >> index 03eb53fd029a..a6d81ff578fe 100644 > >> --- a/Documentation/process/coding-style.rst > >> +++ b/Documentation/process/coding-style.rst > >> @@ -1186,6 +1186,33 @@ expression used. For instance: > >> #endif /* CONFIG_SOMETHING */ > >> > > > > I like the idea of adding this documentation, and this is the right > > place. Naturally, if one likes something, one must immediately change > > it. :) Therefore, here is an alternative writeup that I think captures > > what you and the email threads were saying. > > > > How's this sound? > > Much better, thanks! :) > > > > > diff --git a/Documentation/process/coding-style.rst b/Documentation/process/coding-style.rst > > index 03eb53fd029a..32df0d503388 100644 > > --- a/Documentation/process/coding-style.rst > > +++ b/Documentation/process/coding-style.rst > > @@ -1185,6 +1185,53 @@ expression used. For instance: > > ... > > #endif /* CONFIG_SOMETHING */ > > > > +22) Do not crash the kernel > > +--------------------------- > > + > > +Use WARN() rather than BUG() > > +**************************** > > + > > +Do not add new code that uses any of the BUG() variants, such as BUG(), > > +BUG_ON(), or VM_BUG_ON(). Instead, use a WARN*() variant, preferably > > +WARN_ON_ONCE(), and possibly with recovery code. Recovery code is not required > > +if there is no reasonable way to at least partially recover. > > I'll tend to keep in this section: > > "Unavoidable data corruption / security issues might be a very rare > exception to this rule and need good justification." > > Because there are rare exceptions, and I'd much rather document the > clear exception to this rule. > > > + > > +Use WARN_ON_ONCE() rather than WARN() or WARN_ON() > > +************************************************** > > + > > +WARN_ON_ONCE() is generally preferred over WARN() or WARN_ON(), because it is > > +common for a given warning condition, if it occurs at all, to occur multiple > > +times. (For example, once per file, or once per struct page.) This can fill up > > I'll drop the "For example" part. I feel like this doesn't really need > an example -- most probably we've all been there already when the kernel > log was flooded :) > > > +and wrap the kernel log, and can even slow the system enough that the excessive > > +logging turns into its own, additional problem. > > + > > +Do not WARN lightly > > +******************* > > + > > +WARN*() is intended for unexpected, this-should-never-happen situations. WARN*() > > +macros are not to be used for anything that is expected to happen during normal > > +operation. These are not pre- or post-condition asserts, for example. Again: > > +WARN*() must not be used for a condition that is expected to trigger easily, for > > +example, by user space actions. pr_warn_once() is a possible alternative, if you > > +need to notify the user of a problem. > > + > > +Do not worry about panic_on_warn users > > +************************************** > > + > > +A few more words about panic_on_warn: Remember that ``panic_on_warn`` is an > > +available kernel option, and that many users set this option. This is why there > > +is a "Do not WARN lightly" writeup, above. However, the existence of > > +panic_on_warn users is not a valid reason to avoid the judicious use WARN*(). > > +That is because, whoever enables panic_on_warn has explicitly asked the kernel > > +to crash if a WARN*() fires, and such users must be prepared to deal with the > > +consequences of a system that is somewhat more likely to crash. > > Side note: especially with kdump() I feel like we might see much more > widespread use of panic_on_warn to be able to actually extract debug > information in a controlled manner -- for example on enterprise distros. > ... which would then make these systems more likely to crash, because > there is no way to distinguish a rather harmless warning from a severe > warning :/ . But let's see if some kdump() folks will share their > opinion as reply to the cover letter. I can understand the intention of this patch, and I totally agree that BUG() should be used carefully, this is a good proposal if we can clearly define the standard about when to use BUG(). But I do have some worries, I think this standard is different for different sub components, it is not clear to me at least, so this may introduce an unstable running kernel and cause troubles (eg. data corruption) with a WARN instead of a BUG. Probably it would be better to say "Do not WARN lightly, and do not hesitate to use BUG if it is really needed"? About "patch_on_warn", it will depend on the admin/end user to set it, it is not a good idea for distribution to set it. It seems we are leaving it to end users to take the risk of a kernel panic even with all kernel WARN even if it is sometimes not necessary. > > -- > Thanks, > > David / dhildenb > Thanks Dave