coding-style.rst: document BUG() and WARN() rules ("do not crash the kernel")
Linus notes [1] that the introduction of new code that uses VM_BUG_ON() is just as bad as BUG_ON(), because it will crash the kernel on distributions that enable CONFIG_DEBUG_VM (like Fedora): VM_BUG_ON() has the exact same semantics as BUG_ON. It is literally no different, the only difference is "we can make the code smaller because these are less important". [2] This resulted in a more generic discussion about usage of BUG() and friends. While there might be corner cases that still deserve a BUG_ON(), most BUG_ON() cases should simply use WARN_ON_ONCE() and implement a recovery path if reasonable: The only possible case where BUG_ON can validly be used is "I have some fundamental data corruption and cannot possibly return an error". [2] As a very good approximation is the general rule: "absolutely no new BUG_ON() calls _ever_" [2] ... not even if something really shouldn't ever happen and is merely for documenting that an invariant always has to hold. However, there are sill exceptions where BUG_ON() may be used: If you have a "this is major internal corruption, there's no way we can continue", then BUG_ON() is appropriate. [3] There is only one good BUG_ON(): Now, that said, there is one very valid sub-form of BUG_ON(): BUILD_BUG_ON() is absolutely 100% fine. [2] While WARN will also crash the machine with panic_on_warn set, that's exactly to be expected: So we have two very different cases: the "virtual machine with good logging where a dead machine is fine" - use 'panic_on_warn'. And the actual real hardware with real drivers, running real loads by users. [4] The basic idea is that warnings will similarly get reported by users and be found during testing. However, in contrast to a BUG(), there is a way to actually influence the expected behavior (e.g., panic_on_warn) and to eventually keep the machine alive to extract some debug info. Ingo notes that not all WARN_ON_ONCE cases need recovery. If we don't ever expect this code to trigger in any case, recovery code is not really helpful. I'd prefer to keep all these warnings 'simple' - i.e. no attempted recovery & control flow, unless we ever expect these to trigger. [5] There have been different rules floating around that were never properly documented. Let's try to clarify. [1] https://lkml.kernel.org/r/CAHk-=wiEAH+ojSpAgx_Ep=NKPWHU8AdO3V56BXcCsU97oYJ1EA@mail.gmail.com [2] https://lore.kernel.org/r/CAHk-=wg40EAZofO16Eviaj7mfqDhZ2gVEbvfsMf6gYzspRjYvw@mail.gmail.com [3] https://lkml.kernel.org/r/CAHk-=wit-DmhMfQErY29JSPjFgebx_Ld+pnerc4J2Ag990WwAA@mail.gmail.com [4] https://lore.kernel.org/r/CAHk-=wgF7K2gSSpy=m_=K3Nov4zaceUX9puQf1TjkTJLA2XC_g@mail.gmail.com [5] https://lore.kernel.org/r/YwIW+mVeZoTOxn%2F4@gmail.com Reviewed-by: John Hubbard <jhubbard@nvidia.com> Signed-off-by: David Hildenbrand <david@redhat.com> Link: https://lore.kernel.org/r/20220923113426.52871-2-david@redhat.com Signed-off-by: Jonathan Corbet <corbet@lwn.net>
This commit is contained in:
parent
657ed9c9bc
commit
1cfd9d7e43
|
@ -1186,6 +1186,68 @@ expression used. For instance:
|
|||
#endif /* CONFIG_SOMETHING */
|
||||
|
||||
|
||||
22) Do not crash the kernel
|
||||
---------------------------
|
||||
|
||||
In general, the decision to crash the kernel belongs to the user, rather
|
||||
than to the kernel developer.
|
||||
|
||||
Avoid panic()
|
||||
*************
|
||||
|
||||
panic() should be used with care and primarily only during system boot.
|
||||
panic() is, for example, acceptable when running out of memory during boot and
|
||||
not being able to continue.
|
||||
|
||||
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'm too lazy to do error handling" is not an excuse for using BUG(). Major
|
||||
internal corruptions with no way of continuing may still use BUG(), but need
|
||||
good justification.
|
||||
|
||||
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. This can fill up 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.
|
||||
|
||||
Use BUILD_BUG_ON() for compile-time assertions
|
||||
**********************************************
|
||||
|
||||
The use of BUILD_BUG_ON() is acceptable and encouraged, because it is a
|
||||
compile-time assertion that has no effect at runtime.
|
||||
|
||||
Appendix I) References
|
||||
----------------------
|
||||
|
||||
|
|
Loading…
Reference in New Issue