* [PATCH v1 1/3] coding-style.rst: document BUG() and WARN() rules ("do not crash the kernel")
@ 2022-09-20 12:23 ` David Hildenbrand
0 siblings, 0 replies; 40+ messages in thread
From: David Hildenbrand @ 2022-09-20 12:23 UTC (permalink / raw)
To: linux-kernel
Cc: Lukas Bulwahn, Jonathan Corbet, Baoquan He, David Hildenbrand,
Linus Torvalds, Dave Young, linux-doc, Nicholas Piggin,
Jani Nikula, linux-mm, David Laight, Dwaipayan Ray,
Andy Whitcroft, Joe Perches, Andrew Morton, linuxppc-dev,
Ingo Molnar, Vivek Goyal
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
[2] 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
Signed-off-by: David Hildenbrand <david@redhat.com>
---
Documentation/process/coding-style.rst | 61 ++++++++++++++++++++++++++
1 file changed, 61 insertions(+)
diff --git a/Documentation/process/coding-style.rst b/Documentation/process/coding-style.rst
index 03eb53fd029a..e05899cbfd49 100644
--- a/Documentation/process/coding-style.rst
+++ b/Documentation/process/coding-style.rst
@@ -1186,6 +1186,67 @@ expression used. For instance:
#endif /* CONFIG_SOMETHING */
+22) Do not crash the kernel
+---------------------------
+
+In general, it is not the kernel developer's decision to crash the kernel.
+
+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
----------------------
--
2.37.3
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [PATCH v1 1/3] coding-style.rst: document BUG() and WARN() rules ("do not crash the kernel")
2022-09-20 12:23 ` David Hildenbrand
@ 2022-09-21 4:40 ` Kalle Valo
-1 siblings, 0 replies; 40+ messages in thread
From: Kalle Valo @ 2022-09-21 4:40 UTC (permalink / raw)
To: David Hildenbrand
Cc: linux-kernel, linux-mm, linux-doc, linuxppc-dev, Linus Torvalds,
Andrew Morton, Ingo Molnar, David Laight, Jonathan Corbet,
Andy Whitcroft, Joe Perches, Dwaipayan Ray, Lukas Bulwahn,
Baoquan He, Vivek Goyal, Dave Young, Jani Nikula,
Michael Ellerman, Nicholas Piggin, Christophe Leroy
David Hildenbrand <david@redhat.com> writes:
> 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
> [2] 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
>
> Signed-off-by: David Hildenbrand <david@redhat.com>
[...]
> +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.
FWIW I have had cases where WARN() messages caused a reboot, maybe
mention that here? In my case the logging was so excessive that the
watchdog wasn't updated and in the end the device was forcefully
rebooted.
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v1 1/3] coding-style.rst: document BUG() and WARN() rules ("do not crash the kernel")
@ 2022-09-21 4:40 ` Kalle Valo
0 siblings, 0 replies; 40+ messages in thread
From: Kalle Valo @ 2022-09-21 4:40 UTC (permalink / raw)
To: David Hildenbrand
Cc: Lukas Bulwahn, Baoquan He, linux-doc, linuxppc-dev, Dave Young,
Jonathan Corbet, Nicholas Piggin, linux-kernel, Jani Nikula,
linux-mm, David Laight, Dwaipayan Ray, Andy Whitcroft,
Joe Perches, Andrew Morton, Linus Torvalds, Ingo Molnar,
Vivek Goyal
David Hildenbrand <david@redhat.com> writes:
> 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
> [2] 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
>
> Signed-off-by: David Hildenbrand <david@redhat.com>
[...]
> +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.
FWIW I have had cases where WARN() messages caused a reboot, maybe
mention that here? In my case the logging was so excessive that the
watchdog wasn't updated and in the end the device was forcefully
rebooted.
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v1 1/3] coding-style.rst: document BUG() and WARN() rules ("do not crash the kernel")
2022-09-21 4:40 ` Kalle Valo
@ 2022-09-22 14:12 ` David Hildenbrand
-1 siblings, 0 replies; 40+ messages in thread
From: David Hildenbrand @ 2022-09-22 14:12 UTC (permalink / raw)
To: Kalle Valo
Cc: linux-kernel, linux-mm, linux-doc, linuxppc-dev, Linus Torvalds,
Andrew Morton, Ingo Molnar, David Laight, Jonathan Corbet,
Andy Whitcroft, Joe Perches, Dwaipayan Ray, Lukas Bulwahn,
Baoquan He, Vivek Goyal, Dave Young, Jani Nikula,
Michael Ellerman, Nicholas Piggin, Christophe Leroy
On 21.09.22 06:40, Kalle Valo wrote:
> David Hildenbrand <david@redhat.com> writes:
>
>> 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
>> [2] 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
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>
> [...]
>
>> +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.
>
> FWIW I have had cases where WARN() messages caused a reboot, maybe
> mention that here? In my case the logging was so excessive that the
> watchdog wasn't updated and in the end the device was forcefully
> rebooted.
>
That should be covered by the last part, no? What would be your suggestion?
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v1 1/3] coding-style.rst: document BUG() and WARN() rules ("do not crash the kernel")
@ 2022-09-22 14:12 ` David Hildenbrand
0 siblings, 0 replies; 40+ messages in thread
From: David Hildenbrand @ 2022-09-22 14:12 UTC (permalink / raw)
To: Kalle Valo
Cc: Lukas Bulwahn, Baoquan He, linux-doc, linuxppc-dev, Dave Young,
Jonathan Corbet, Nicholas Piggin, linux-kernel, Jani Nikula,
linux-mm, David Laight, Dwaipayan Ray, Andy Whitcroft,
Joe Perches, Andrew Morton, Linus Torvalds, Ingo Molnar,
Vivek Goyal
On 21.09.22 06:40, Kalle Valo wrote:
> David Hildenbrand <david@redhat.com> writes:
>
>> 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
>> [2] 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
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>
> [...]
>
>> +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.
>
> FWIW I have had cases where WARN() messages caused a reboot, maybe
> mention that here? In my case the logging was so excessive that the
> watchdog wasn't updated and in the end the device was forcefully
> rebooted.
>
That should be covered by the last part, no? What would be your suggestion?
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v1 1/3] coding-style.rst: document BUG() and WARN() rules ("do not crash the kernel")
2022-09-22 14:12 ` David Hildenbrand
@ 2022-09-26 7:44 ` Kalle Valo
-1 siblings, 0 replies; 40+ messages in thread
From: Kalle Valo @ 2022-09-26 7:44 UTC (permalink / raw)
To: David Hildenbrand
Cc: linux-kernel, linux-mm, linux-doc, linuxppc-dev, Linus Torvalds,
Andrew Morton, Ingo Molnar, David Laight, Jonathan Corbet,
Andy Whitcroft, Joe Perches, Dwaipayan Ray, Lukas Bulwahn,
Baoquan He, Vivek Goyal, Dave Young, Jani Nikula,
Michael Ellerman, Nicholas Piggin, Christophe Leroy
David Hildenbrand <david@redhat.com> writes:
>>> +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.
>>
>> FWIW I have had cases where WARN() messages caused a reboot, maybe
>> mention that here? In my case the logging was so excessive that the
>> watchdog wasn't updated and in the end the device was forcefully
>> rebooted.
>>
>
> That should be covered by the last part, no? What would be your suggestion?
I was just thinking that maybe make it more obvious that even WARN_ON()
can crash the system, something along these lines:
"..., additional problem like stalling the system so much that it causes
a reboot."
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v1 1/3] coding-style.rst: document BUG() and WARN() rules ("do not crash the kernel")
@ 2022-09-26 7:44 ` Kalle Valo
0 siblings, 0 replies; 40+ messages in thread
From: Kalle Valo @ 2022-09-26 7:44 UTC (permalink / raw)
To: David Hildenbrand
Cc: Lukas Bulwahn, Baoquan He, linux-doc, linuxppc-dev, Dave Young,
Jonathan Corbet, Nicholas Piggin, linux-kernel, Jani Nikula,
linux-mm, David Laight, Dwaipayan Ray, Andy Whitcroft,
Joe Perches, Andrew Morton, Linus Torvalds, Ingo Molnar,
Vivek Goyal
David Hildenbrand <david@redhat.com> writes:
>>> +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.
>>
>> FWIW I have had cases where WARN() messages caused a reboot, maybe
>> mention that here? In my case the logging was so excessive that the
>> watchdog wasn't updated and in the end the device was forcefully
>> rebooted.
>>
>
> That should be covered by the last part, no? What would be your suggestion?
I was just thinking that maybe make it more obvious that even WARN_ON()
can crash the system, something along these lines:
"..., additional problem like stalling the system so much that it causes
a reboot."
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v1 1/3] coding-style.rst: document BUG() and WARN() rules ("do not crash the kernel")
2022-09-26 7:44 ` Kalle Valo
@ 2022-10-04 12:32 ` David Hildenbrand
-1 siblings, 0 replies; 40+ messages in thread
From: David Hildenbrand @ 2022-10-04 12:32 UTC (permalink / raw)
To: Kalle Valo
Cc: linux-kernel, linux-mm, linux-doc, linuxppc-dev, Linus Torvalds,
Andrew Morton, Ingo Molnar, David Laight, Jonathan Corbet,
Andy Whitcroft, Joe Perches, Dwaipayan Ray, Lukas Bulwahn,
Baoquan He, Vivek Goyal, Dave Young, Jani Nikula,
Michael Ellerman, Nicholas Piggin, Christophe Leroy
On 26.09.22 09:44, Kalle Valo wrote:
> David Hildenbrand <david@redhat.com> writes:
>
>>>> +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.
>>>
>>> FWIW I have had cases where WARN() messages caused a reboot, maybe
>>> mention that here? In my case the logging was so excessive that the
>>> watchdog wasn't updated and in the end the device was forcefully
>>> rebooted.
>>>
>>
>> That should be covered by the last part, no? What would be your suggestion?
>
> I was just thinking that maybe make it more obvious that even WARN_ON()
> can crash the system, something along these lines:
>
> "..., additional problem like stalling the system so much that it causes
> a reboot."
Hi Kalle,
sorry for the late reply. Jonathan already queued v2 and sent it upstream.
I think that's it is already covered by the statement and that the
additional example isn't required -- most of us learned the hard way
that "excessive logging turns into its own problem" includes all weird
kinds of kernel crashes. A panic/reboot due to a watchdog not firing is
one such possible outcome.
Thanks!
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v1 1/3] coding-style.rst: document BUG() and WARN() rules ("do not crash the kernel")
@ 2022-10-04 12:32 ` David Hildenbrand
0 siblings, 0 replies; 40+ messages in thread
From: David Hildenbrand @ 2022-10-04 12:32 UTC (permalink / raw)
To: Kalle Valo
Cc: Lukas Bulwahn, Baoquan He, linux-doc, linuxppc-dev, Dave Young,
Jonathan Corbet, Nicholas Piggin, linux-kernel, Jani Nikula,
linux-mm, David Laight, Dwaipayan Ray, Andy Whitcroft,
Joe Perches, Andrew Morton, Linus Torvalds, Ingo Molnar,
Vivek Goyal
On 26.09.22 09:44, Kalle Valo wrote:
> David Hildenbrand <david@redhat.com> writes:
>
>>>> +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.
>>>
>>> FWIW I have had cases where WARN() messages caused a reboot, maybe
>>> mention that here? In my case the logging was so excessive that the
>>> watchdog wasn't updated and in the end the device was forcefully
>>> rebooted.
>>>
>>
>> That should be covered by the last part, no? What would be your suggestion?
>
> I was just thinking that maybe make it more obvious that even WARN_ON()
> can crash the system, something along these lines:
>
> "..., additional problem like stalling the system so much that it causes
> a reboot."
Hi Kalle,
sorry for the late reply. Jonathan already queued v2 and sent it upstream.
I think that's it is already covered by the statement and that the
additional example isn't required -- most of us learned the hard way
that "excessive logging turns into its own problem" includes all weird
kinds of kernel crashes. A panic/reboot due to a watchdog not firing is
one such possible outcome.
Thanks!
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v1 1/3] coding-style.rst: document BUG() and WARN() rules ("do not crash the kernel")
2022-09-20 12:23 ` David Hildenbrand
@ 2022-09-22 13:43 ` Akira Yokosawa
-1 siblings, 0 replies; 40+ messages in thread
From: Akira Yokosawa @ 2022-09-22 13:43 UTC (permalink / raw)
To: David Hildenbrand
Cc: David.Laight, akpm, apw, bhe, christophe.leroy, corbet,
dwaipayanray1, dyoung, jani.nikula, joe, linux-doc, linux-kernel,
linux-mm, linuxppc-dev, lukas.bulwahn, mingo, mpe, npiggin,
torvalds, vgoyal, Akira Yokosawa
Hi,
Minor nits on section title adornments.
See inline comments below.
On Tue, 20 Sep 2022 14:23:00 +0200, David Hildenbrand wrote:
> 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
> [2] 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
>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
> Documentation/process/coding-style.rst | 61 ++++++++++++++++++++++++++
> 1 file changed, 61 insertions(+)
>
> diff --git a/Documentation/process/coding-style.rst b/Documentation/process/coding-style.rst
> index 03eb53fd029a..e05899cbfd49 100644
> --- a/Documentation/process/coding-style.rst
> +++ b/Documentation/process/coding-style.rst
> @@ -1186,6 +1186,67 @@ expression used. For instance:
> #endif /* CONFIG_SOMETHING */
>
>
> +22) Do not crash the kernel
> +---------------------------
> +
> +In general, it is not the kernel developer's decision to crash the kernel.
> +
> +Avoid panic()
> +=============
This looks to me like a subsection-level title. The adornment symbol
needs to be:
*************
> +
> +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()
> +============================
Ditto.
> +
> +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()
> +**************************************************
These wrong adornment symbol confuse ReST parser of Sphinx and results in
the build error from "make htmldocs" at this title (long message folded):
Sphinx parallel build error:
docutils.utils.SystemMessage: /xxx/Documentation/process/coding-style.rst:1213:
(SEVERE/4) Title level inconsistent:
Please fix in v2.
Thanks, Akira
> +
> +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.
> +
[...]
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v1 1/3] coding-style.rst: document BUG() and WARN() rules ("do not crash the kernel")
@ 2022-09-22 13:43 ` Akira Yokosawa
0 siblings, 0 replies; 40+ messages in thread
From: Akira Yokosawa @ 2022-09-22 13:43 UTC (permalink / raw)
To: David Hildenbrand
Cc: linuxppc-dev, bhe, corbet, Akira Yokosawa, dwaipayanray1,
linux-doc, npiggin, linux-kernel, linux-mm, joe, torvalds,
David.Laight, jani.nikula, apw, lukas.bulwahn, akpm, dyoung,
mingo, vgoyal
Hi,
Minor nits on section title adornments.
See inline comments below.
On Tue, 20 Sep 2022 14:23:00 +0200, David Hildenbrand wrote:
> 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
> [2] 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
>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
> Documentation/process/coding-style.rst | 61 ++++++++++++++++++++++++++
> 1 file changed, 61 insertions(+)
>
> diff --git a/Documentation/process/coding-style.rst b/Documentation/process/coding-style.rst
> index 03eb53fd029a..e05899cbfd49 100644
> --- a/Documentation/process/coding-style.rst
> +++ b/Documentation/process/coding-style.rst
> @@ -1186,6 +1186,67 @@ expression used. For instance:
> #endif /* CONFIG_SOMETHING */
>
>
> +22) Do not crash the kernel
> +---------------------------
> +
> +In general, it is not the kernel developer's decision to crash the kernel.
> +
> +Avoid panic()
> +=============
This looks to me like a subsection-level title. The adornment symbol
needs to be:
*************
> +
> +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()
> +============================
Ditto.
> +
> +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()
> +**************************************************
These wrong adornment symbol confuse ReST parser of Sphinx and results in
the build error from "make htmldocs" at this title (long message folded):
Sphinx parallel build error:
docutils.utils.SystemMessage: /xxx/Documentation/process/coding-style.rst:1213:
(SEVERE/4) Title level inconsistent:
Please fix in v2.
Thanks, Akira
> +
> +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.
> +
[...]
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v1 1/3] coding-style.rst: document BUG() and WARN() rules ("do not crash the kernel")
2022-09-22 13:43 ` Akira Yokosawa
@ 2022-09-22 14:41 ` David Hildenbrand
-1 siblings, 0 replies; 40+ messages in thread
From: David Hildenbrand @ 2022-09-22 14:41 UTC (permalink / raw)
To: Akira Yokosawa
Cc: David.Laight, akpm, apw, bhe, christophe.leroy, corbet,
dwaipayanray1, dyoung, jani.nikula, joe, linux-doc, linux-kernel,
linux-mm, linuxppc-dev, lukas.bulwahn, mingo, mpe, npiggin,
torvalds, vgoyal
>>
>> +22) Do not crash the kernel
>> +---------------------------
>> +
>> +In general, it is not the kernel developer's decision to crash the kernel.
>> +
>> +Avoid panic()
>> +=============
> This looks to me like a subsection-level title. The adornment symbol
> needs to be:
>
> *************
>
>> +
>> +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()
>> +============================
> Ditto.
>
>> +
>> +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()
>> +**************************************************
> These wrong adornment symbol confuse ReST parser of Sphinx and results in
> the build error from "make htmldocs" at this title (long message folded):
Thanks,
the following on top should do the trick:
diff --git a/Documentation/process/coding-style.rst b/Documentation/process/coding-style.rst
index e05899cbfd49..9efde65ac2f3 100644
--- a/Documentation/process/coding-style.rst
+++ b/Documentation/process/coding-style.rst
@@ -1192,14 +1192,14 @@ expression used. For instance:
In general, it is not the kernel developer's decision to crash the kernel.
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
--
Thanks,
David / dhildenb
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [PATCH v1 1/3] coding-style.rst: document BUG() and WARN() rules ("do not crash the kernel")
@ 2022-09-22 14:41 ` David Hildenbrand
0 siblings, 0 replies; 40+ messages in thread
From: David Hildenbrand @ 2022-09-22 14:41 UTC (permalink / raw)
To: Akira Yokosawa
Cc: linuxppc-dev, bhe, corbet, dwaipayanray1, linux-doc, npiggin,
linux-kernel, linux-mm, joe, torvalds, David.Laight, jani.nikula,
apw, lukas.bulwahn, akpm, dyoung, mingo, vgoyal
>>
>> +22) Do not crash the kernel
>> +---------------------------
>> +
>> +In general, it is not the kernel developer's decision to crash the kernel.
>> +
>> +Avoid panic()
>> +=============
> This looks to me like a subsection-level title. The adornment symbol
> needs to be:
>
> *************
>
>> +
>> +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()
>> +============================
> Ditto.
>
>> +
>> +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()
>> +**************************************************
> These wrong adornment symbol confuse ReST parser of Sphinx and results in
> the build error from "make htmldocs" at this title (long message folded):
Thanks,
the following on top should do the trick:
diff --git a/Documentation/process/coding-style.rst b/Documentation/process/coding-style.rst
index e05899cbfd49..9efde65ac2f3 100644
--- a/Documentation/process/coding-style.rst
+++ b/Documentation/process/coding-style.rst
@@ -1192,14 +1192,14 @@ expression used. For instance:
In general, it is not the kernel developer's decision to crash the kernel.
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
--
Thanks,
David / dhildenb
^ permalink raw reply related [flat|nested] 40+ messages in thread
* Re: [PATCH v1 1/3] coding-style.rst: document BUG() and WARN() rules ("do not crash the kernel")
2022-09-20 12:23 ` David Hildenbrand
@ 2022-09-23 2:26 ` John Hubbard
-1 siblings, 0 replies; 40+ messages in thread
From: John Hubbard @ 2022-09-23 2:26 UTC (permalink / raw)
To: David Hildenbrand, linux-kernel
Cc: linux-mm, linux-doc, linuxppc-dev, Linus Torvalds, Andrew Morton,
Ingo Molnar, David Laight, Jonathan Corbet, Andy Whitcroft,
Joe Perches, Dwaipayan Ray, Lukas Bulwahn, Baoquan He,
Vivek Goyal, Dave Young, Jani Nikula, Michael Ellerman,
Nicholas Piggin, Christophe Leroy
On 9/20/22 05:23, David Hildenbrand wrote:
> [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
> [2] https://lkml.kernel.org/r/CAHk-=wit-DmhMfQErY29JSPjFgebx_Ld+pnerc4J2Ag990WwAA@mail.gmail.com
s/2/3/
...
> diff --git a/Documentation/process/coding-style.rst b/Documentation/process/coding-style.rst
> index 03eb53fd029a..e05899cbfd49 100644
> --- a/Documentation/process/coding-style.rst
> +++ b/Documentation/process/coding-style.rst
> @@ -1186,6 +1186,67 @@ expression used. For instance:
> #endif /* CONFIG_SOMETHING */
>
>
> +22) Do not crash the kernel
> +---------------------------
> +
> +In general, it is not the kernel developer's decision to crash the kernel.
What do you think of this alternate wording:
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
> ----------------------
>
I like the wording, it feels familiar somehow! :)
Reviewed-by: John Hubbard <jhubbard@nvidia.com>
thanks,
--
John Hubbard
NVIDIA
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v1 1/3] coding-style.rst: document BUG() and WARN() rules ("do not crash the kernel")
@ 2022-09-23 2:26 ` John Hubbard
0 siblings, 0 replies; 40+ messages in thread
From: John Hubbard @ 2022-09-23 2:26 UTC (permalink / raw)
To: David Hildenbrand, linux-kernel
Cc: Lukas Bulwahn, Baoquan He, linux-doc, linuxppc-dev, Dave Young,
Jonathan Corbet, Nicholas Piggin, Jani Nikula, linux-mm,
David Laight, Dwaipayan Ray, Andy Whitcroft, Joe Perches,
Andrew Morton, Linus Torvalds, Ingo Molnar, Vivek Goyal
On 9/20/22 05:23, David Hildenbrand wrote:
> [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
> [2] https://lkml.kernel.org/r/CAHk-=wit-DmhMfQErY29JSPjFgebx_Ld+pnerc4J2Ag990WwAA@mail.gmail.com
s/2/3/
...
> diff --git a/Documentation/process/coding-style.rst b/Documentation/process/coding-style.rst
> index 03eb53fd029a..e05899cbfd49 100644
> --- a/Documentation/process/coding-style.rst
> +++ b/Documentation/process/coding-style.rst
> @@ -1186,6 +1186,67 @@ expression used. For instance:
> #endif /* CONFIG_SOMETHING */
>
>
> +22) Do not crash the kernel
> +---------------------------
> +
> +In general, it is not the kernel developer's decision to crash the kernel.
What do you think of this alternate wording:
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
> ----------------------
>
I like the wording, it feels familiar somehow! :)
Reviewed-by: John Hubbard <jhubbard@nvidia.com>
thanks,
--
John Hubbard
NVIDIA
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v1 1/3] coding-style.rst: document BUG() and WARN() rules ("do not crash the kernel")
2022-09-23 2:26 ` John Hubbard
@ 2022-09-23 2:37 ` John Hubbard
-1 siblings, 0 replies; 40+ messages in thread
From: John Hubbard @ 2022-09-23 2:37 UTC (permalink / raw)
To: David Hildenbrand, linux-kernel
Cc: linux-mm, linux-doc, linuxppc-dev, Linus Torvalds, Andrew Morton,
Ingo Molnar, David Laight, Jonathan Corbet, Andy Whitcroft,
Joe Perches, Dwaipayan Ray, Lukas Bulwahn, Baoquan He,
Vivek Goyal, Dave Young, Jani Nikula, Michael Ellerman,
Nicholas Piggin, Christophe Leroy
On 9/22/22 19:26, John Hubbard wrote:
>
> Reviewed-by: John Hubbard <jhubbard@nvidia.com>
>
I forgot to mention that I had applied your fix to Akira's
issue, before reviewing. So that fix works and builds and
looks nice too.
thanks,
--
John Hubbard
NVIDIA
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v1 1/3] coding-style.rst: document BUG() and WARN() rules ("do not crash the kernel")
@ 2022-09-23 2:37 ` John Hubbard
0 siblings, 0 replies; 40+ messages in thread
From: John Hubbard @ 2022-09-23 2:37 UTC (permalink / raw)
To: David Hildenbrand, linux-kernel
Cc: Lukas Bulwahn, Baoquan He, linux-doc, linuxppc-dev, Dave Young,
Jonathan Corbet, Nicholas Piggin, Jani Nikula, linux-mm,
David Laight, Dwaipayan Ray, Andy Whitcroft, Joe Perches,
Andrew Morton, Linus Torvalds, Ingo Molnar, Vivek Goyal
On 9/22/22 19:26, John Hubbard wrote:
>
> Reviewed-by: John Hubbard <jhubbard@nvidia.com>
>
I forgot to mention that I had applied your fix to Akira's
issue, before reviewing. So that fix works and builds and
looks nice too.
thanks,
--
John Hubbard
NVIDIA
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v1 1/3] coding-style.rst: document BUG() and WARN() rules ("do not crash the kernel")
2022-09-23 2:26 ` John Hubbard
@ 2022-09-23 10:55 ` David Hildenbrand
-1 siblings, 0 replies; 40+ messages in thread
From: David Hildenbrand @ 2022-09-23 10:55 UTC (permalink / raw)
To: John Hubbard, linux-kernel
Cc: linux-mm, linux-doc, linuxppc-dev, Linus Torvalds, Andrew Morton,
Ingo Molnar, David Laight, Jonathan Corbet, Andy Whitcroft,
Joe Perches, Dwaipayan Ray, Lukas Bulwahn, Baoquan He,
Vivek Goyal, Dave Young, Jani Nikula, Michael Ellerman,
Nicholas Piggin, Christophe Leroy
On 23.09.22 04:26, John Hubbard wrote:
> On 9/20/22 05:23, David Hildenbrand wrote:
>> [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
>> [2] https://lkml.kernel.org/r/CAHk-=wit-DmhMfQErY29JSPjFgebx_Ld+pnerc4J2Ag990WwAA@mail.gmail.com
>
> s/2/3/
Thanks!
>
> ...
>> diff --git a/Documentation/process/coding-style.rst b/Documentation/process/coding-style.rst
>> index 03eb53fd029a..e05899cbfd49 100644
>> --- a/Documentation/process/coding-style.rst
>> +++ b/Documentation/process/coding-style.rst
>> @@ -1186,6 +1186,67 @@ expression used. For instance:
>> #endif /* CONFIG_SOMETHING */
>>
>>
>> +22) Do not crash the kernel
>> +---------------------------
>> +
>> +In general, it is not the kernel developer's decision to crash the kernel.
>
> What do you think of this alternate wording:
>
> In general, the decision to crash the kernel belongs to the user, rather
> than to the kernel developer.
Ack
[...]
> I like the wording, it feels familiar somehow! :)
:)
>
> Reviewed-by: John Hubbard <jhubbard@nvidia.com>
Thanks!
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v1 1/3] coding-style.rst: document BUG() and WARN() rules ("do not crash the kernel")
@ 2022-09-23 10:55 ` David Hildenbrand
0 siblings, 0 replies; 40+ messages in thread
From: David Hildenbrand @ 2022-09-23 10:55 UTC (permalink / raw)
To: John Hubbard, linux-kernel
Cc: Lukas Bulwahn, Baoquan He, linux-doc, linuxppc-dev, Dave Young,
Jonathan Corbet, Nicholas Piggin, Jani Nikula, linux-mm,
David Laight, Dwaipayan Ray, Andy Whitcroft, Joe Perches,
Andrew Morton, Linus Torvalds, Ingo Molnar, Vivek Goyal
On 23.09.22 04:26, John Hubbard wrote:
> On 9/20/22 05:23, David Hildenbrand wrote:
>> [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
>> [2] https://lkml.kernel.org/r/CAHk-=wit-DmhMfQErY29JSPjFgebx_Ld+pnerc4J2Ag990WwAA@mail.gmail.com
>
> s/2/3/
Thanks!
>
> ...
>> diff --git a/Documentation/process/coding-style.rst b/Documentation/process/coding-style.rst
>> index 03eb53fd029a..e05899cbfd49 100644
>> --- a/Documentation/process/coding-style.rst
>> +++ b/Documentation/process/coding-style.rst
>> @@ -1186,6 +1186,67 @@ expression used. For instance:
>> #endif /* CONFIG_SOMETHING */
>>
>>
>> +22) Do not crash the kernel
>> +---------------------------
>> +
>> +In general, it is not the kernel developer's decision to crash the kernel.
>
> What do you think of this alternate wording:
>
> In general, the decision to crash the kernel belongs to the user, rather
> than to the kernel developer.
Ack
[...]
> I like the wording, it feels familiar somehow! :)
:)
>
> Reviewed-by: John Hubbard <jhubbard@nvidia.com>
Thanks!
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 40+ messages in thread