* [PATCH] mm: Use BUG_ON instead of if condition followed by BUG
@ 2021-11-24 3:08 cgel.zte
2021-11-24 11:10 ` David Hildenbrand
2021-11-24 13:23 ` Matthew Wilcox
0 siblings, 2 replies; 9+ messages in thread
From: cgel.zte @ 2021-11-24 3:08 UTC (permalink / raw)
To: akpm; +Cc: linux-mm, linux-kernel, chiminghao, Zeal Robot
From: chiminghao <chi.minghao@zte.com.cn>
Fix the following coccinelle report:
./mm/memory_hotplug.c:2210:2-5:
WARNING Use BUG_ON instead of if condition followed by BUG.
Reported-by: Zeal Robot <zealci@zte.com.cn>
Signed-off-by: chiminghao <chi.minghao@zte.com.cn>
---
mm/memory_hotplug.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 3de7933e5302..aecb12bb7513 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -2212,8 +2212,7 @@ void __remove_memory(u64 start, u64 size)
* trigger BUG() if some memory is not offlined prior to calling this
* function
*/
- if (try_remove_memory(start, size))
- BUG();
+ BUG_ON(try_remove_memory(start, size));
}
/*
--
2.25.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] mm: Use BUG_ON instead of if condition followed by BUG
2021-11-24 3:08 [PATCH] mm: Use BUG_ON instead of if condition followed by BUG cgel.zte
@ 2021-11-24 11:10 ` David Hildenbrand
2021-11-24 13:23 ` Matthew Wilcox
1 sibling, 0 replies; 9+ messages in thread
From: David Hildenbrand @ 2021-11-24 11:10 UTC (permalink / raw)
To: cgel.zte, akpm; +Cc: linux-mm, linux-kernel, chiminghao, Zeal Robot
On 24.11.21 04:08, cgel.zte@gmail.com wrote:
> From: chiminghao <chi.minghao@zte.com.cn>
"mm/memory_hotplug: Use BUG_ON instead of if condition followed by BUG"
Would be better
>
> Fix the following coccinelle report:
> ./mm/memory_hotplug.c:2210:2-5:
> WARNING Use BUG_ON instead of if condition followed by BUG.
>
> Reported-by: Zeal Robot <zealci@zte.com.cn>
> Signed-off-by: chiminghao <chi.minghao@zte.com.cn>
> ---
> mm/memory_hotplug.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 3de7933e5302..aecb12bb7513 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -2212,8 +2212,7 @@ void __remove_memory(u64 start, u64 size)
> * trigger BUG() if some memory is not offlined prior to calling this
> * function
> */
> - if (try_remove_memory(start, size))
> - BUG();
> + BUG_ON(try_remove_memory(start, size));
> }
>
> /*
>
Acked-by: David Hildenbrand <david@redhat.com>
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] mm: Use BUG_ON instead of if condition followed by BUG
2021-11-24 3:08 [PATCH] mm: Use BUG_ON instead of if condition followed by BUG cgel.zte
2021-11-24 11:10 ` David Hildenbrand
@ 2021-11-24 13:23 ` Matthew Wilcox
2021-11-24 22:27 ` Andrew Morton
2021-11-25 0:00 ` Matthew Wilcox
1 sibling, 2 replies; 9+ messages in thread
From: Matthew Wilcox @ 2021-11-24 13:23 UTC (permalink / raw)
To: cgel.zte; +Cc: akpm, linux-mm, linux-kernel, chiminghao, Zeal Robot
On Wed, Nov 24, 2021 at 03:08:49AM +0000, cgel.zte@gmail.com wrote:
> From: chiminghao <chi.minghao@zte.com.cn>
>
> Fix the following coccinelle report:
> ./mm/memory_hotplug.c:2210:2-5:
> WARNING Use BUG_ON instead of if condition followed by BUG.
What coccinelle script is reporting this?
> - if (try_remove_memory(start, size))
> - BUG();
> + BUG_ON(try_remove_memory(start, size));
I really, really, really do not like this. For functions with
side-effects, this is bad style. If it's a pure predicate, then
sure, but this is bad.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] mm: Use BUG_ON instead of if condition followed by BUG
2021-11-24 13:23 ` Matthew Wilcox
@ 2021-11-24 22:27 ` Andrew Morton
2021-11-24 22:45 ` John Hubbard
2021-11-25 0:00 ` Matthew Wilcox
1 sibling, 1 reply; 9+ messages in thread
From: Andrew Morton @ 2021-11-24 22:27 UTC (permalink / raw)
To: Matthew Wilcox; +Cc: cgel.zte, linux-mm, linux-kernel, chiminghao, Zeal Robot
On Wed, 24 Nov 2021 13:23:42 +0000 Matthew Wilcox <willy@infradead.org> wrote:
> On Wed, Nov 24, 2021 at 03:08:49AM +0000, cgel.zte@gmail.com wrote:
> > From: chiminghao <chi.minghao@zte.com.cn>
> >
> > Fix the following coccinelle report:
> > ./mm/memory_hotplug.c:2210:2-5:
> > WARNING Use BUG_ON instead of if condition followed by BUG.
>
> What coccinelle script is reporting this?
>
> > - if (try_remove_memory(start, size))
> > - BUG();
> > + BUG_ON(try_remove_memory(start, size));
>
> I really, really, really do not like this. For functions with
> side-effects, this is bad style. If it's a pure predicate, then
> sure, but this is bad.
I don't like it either. Yes, BUG() is special but it's such dangerous
practice. I'd vote to change coccinelle.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] mm: Use BUG_ON instead of if condition followed by BUG
2021-11-24 22:27 ` Andrew Morton
@ 2021-11-24 22:45 ` John Hubbard
2021-11-25 3:32 ` Matthew Wilcox
0 siblings, 1 reply; 9+ messages in thread
From: John Hubbard @ 2021-11-24 22:45 UTC (permalink / raw)
To: Andrew Morton, Matthew Wilcox
Cc: cgel.zte, linux-mm, linux-kernel, chiminghao, Zeal Robot
On 11/24/21 14:27, Andrew Morton wrote:
> On Wed, 24 Nov 2021 13:23:42 +0000 Matthew Wilcox <willy@infradead.org> wrote:
>
>> On Wed, Nov 24, 2021 at 03:08:49AM +0000, cgel.zte@gmail.com wrote:
>>> From: chiminghao <chi.minghao@zte.com.cn>
>>>
>>> Fix the following coccinelle report:
>>> ./mm/memory_hotplug.c:2210:2-5:
>>> WARNING Use BUG_ON instead of if condition followed by BUG.
>>
>> What coccinelle script is reporting this?
>>
>>> - if (try_remove_memory(start, size))
>>> - BUG();
>>> + BUG_ON(try_remove_memory(start, size));
>>
>> I really, really, really do not like this. For functions with
>> side-effects, this is bad style. If it's a pure predicate, then
>> sure, but this is bad.
>
> I don't like it either. Yes, BUG() is special but it's such dangerous
> practice. I'd vote to change coccinelle.
>
Definitely! Or at least use a safer pattern/habit, with just a passive
variable in the BUG_ON() call, approximately like this:
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 852041f6be41..48bd5ff341e7 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -2201,13 +2201,12 @@ static int __ref try_remove_memory(u64 start, u64 size)
*/
void __remove_memory(u64 start, u64 size)
{
-
+ int ret = try_remove_memory(start, size);
/*
* trigger BUG() if some memory is not offlined prior to calling this
* function
*/
- if (try_remove_memory(start, size))
- BUG();
+ BUG_ON(ret);
}
/*
...and by the way, while going to type that, I immediately stumbled upon
another pre-existing case of this sort of thing, in try_remove_memory(),
which does this:
static int __ref try_remove_memory(u64 start, u64 size)
{
struct vmem_altmap mhp_altmap = {};
struct vmem_altmap *altmap = NULL;
unsigned long nr_vmemmap_pages;
int rc = 0, nid = NUMA_NO_NODE;
BUG_ON(check_hotplug_memory_range(start, size));
...
thanks,
--
John Hubbard
NVIDIA
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] mm: Use BUG_ON instead of if condition followed by BUG
2021-11-24 13:23 ` Matthew Wilcox
2021-11-24 22:27 ` Andrew Morton
@ 2021-11-25 0:00 ` Matthew Wilcox
2021-11-27 18:05 ` Julia Lawall
1 sibling, 1 reply; 9+ messages in thread
From: Matthew Wilcox @ 2021-11-25 0:00 UTC (permalink / raw)
To: cgel.zte
Cc: akpm, linux-mm, linux-kernel, chiminghao, Zeal Robot,
Julia Lawall, Michal Marek
On Wed, Nov 24, 2021 at 01:23:42PM +0000, Matthew Wilcox wrote:
> On Wed, Nov 24, 2021 at 03:08:49AM +0000, cgel.zte@gmail.com wrote:
> > From: chiminghao <chi.minghao@zte.com.cn>
> >
> > Fix the following coccinelle report:
> > ./mm/memory_hotplug.c:2210:2-5:
> > WARNING Use BUG_ON instead of if condition followed by BUG.
>
> What coccinelle script is reporting this?
Maybe I found it?
scripts/coccinelle/misc/bugon.cocci:msg="WARNING: Use BUG_ON instead of if condition followed by BUG.\nPlease make sure the condition has no side effects (see conditional BUG_ON definition in include/asm-generic/bug.h)"
Julia, Michal, can we delete this script, please? It's being abused.
> > - if (try_remove_memory(start, size))
> > - BUG();
> > + BUG_ON(try_remove_memory(start, size));
>
> I really, really, really do not like this. For functions with
> side-effects, this is bad style. If it's a pure predicate, then
> sure, but this is bad.
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] mm: Use BUG_ON instead of if condition followed by BUG
2021-11-24 22:45 ` John Hubbard
@ 2021-11-25 3:32 ` Matthew Wilcox
2021-11-25 5:29 ` John Hubbard
0 siblings, 1 reply; 9+ messages in thread
From: Matthew Wilcox @ 2021-11-25 3:32 UTC (permalink / raw)
To: John Hubbard
Cc: Andrew Morton, cgel.zte, linux-mm, linux-kernel, chiminghao, Zeal Robot
On Wed, Nov 24, 2021 at 02:45:59PM -0800, John Hubbard wrote:
> @@ -2201,13 +2201,12 @@ static int __ref try_remove_memory(u64 start, u64 size)
> */
> void __remove_memory(u64 start, u64 size)
> {
> -
> + int ret = try_remove_memory(start, size);
> /*
> * trigger BUG() if some memory is not offlined prior to calling this
> * function
> */
> - if (try_remove_memory(start, size))
> - BUG();
> + BUG_ON(ret);
> }
I'd rather leave it the way it is. I don't see why the version you
propose is better.
> ...and by the way, while going to type that, I immediately stumbled upon
> another pre-existing case of this sort of thing, in try_remove_memory(),
> which does this:
>
> static int __ref try_remove_memory(u64 start, u64 size)
> {
> struct vmem_altmap mhp_altmap = {};
> struct vmem_altmap *altmap = NULL;
> unsigned long nr_vmemmap_pages;
> int rc = 0, nid = NUMA_NO_NODE;
>
> BUG_ON(check_hotplug_memory_range(start, size));
That needs to be fixed.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] mm: Use BUG_ON instead of if condition followed by BUG
2021-11-25 3:32 ` Matthew Wilcox
@ 2021-11-25 5:29 ` John Hubbard
0 siblings, 0 replies; 9+ messages in thread
From: John Hubbard @ 2021-11-25 5:29 UTC (permalink / raw)
To: Matthew Wilcox
Cc: Andrew Morton, cgel.zte, linux-mm, linux-kernel, chiminghao, Zeal Robot
On 11/24/21 19:32, Matthew Wilcox wrote:
> On Wed, Nov 24, 2021 at 02:45:59PM -0800, John Hubbard wrote:
>> @@ -2201,13 +2201,12 @@ static int __ref try_remove_memory(u64 start, u64 size)
>> */
>> void __remove_memory(u64 start, u64 size)
>> {
>> -
>> + int ret = try_remove_memory(start, size);
>> /*
>> * trigger BUG() if some memory is not offlined prior to calling this
>> * function
>> */
>> - if (try_remove_memory(start, size))
>> - BUG();
>> + BUG_ON(ret);
>> }
>
> I'd rather leave it the way it is. I don't see why the version you
> propose is better.
In isolation, it's *not* better. It's only potentially useful in the
context of "code plus tools". That is to say, if the coccinelle change
request were rejected, then this provides a way forward that is not
worse than the existing code, and also works around the warning.
>
>> ...and by the way, while going to type that, I immediately stumbled upon
>> another pre-existing case of this sort of thing, in try_remove_memory(),
>> which does this:
>>
>> static int __ref try_remove_memory(u64 start, u64 size)
>> {
>> struct vmem_altmap mhp_altmap = {};
>> struct vmem_altmap *altmap = NULL;
>> unsigned long nr_vmemmap_pages;
>> int rc = 0, nid = NUMA_NO_NODE;
>>
>> BUG_ON(check_hotplug_memory_range(start, size));
>
> That needs to be fixed.
Yes it does. :) I pointed it out in hopes that Chiminghao might be inspired
to go find and fix some of these.
thanks,
--
John Hubbard
NVIDIA
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] mm: Use BUG_ON instead of if condition followed by BUG
2021-11-25 0:00 ` Matthew Wilcox
@ 2021-11-27 18:05 ` Julia Lawall
0 siblings, 0 replies; 9+ messages in thread
From: Julia Lawall @ 2021-11-27 18:05 UTC (permalink / raw)
To: Matthew Wilcox
Cc: cgel.zte, akpm, linux-mm, linux-kernel, chiminghao, Zeal Robot,
Julia Lawall, Michal Marek
On Thu, 25 Nov 2021, Matthew Wilcox wrote:
> On Wed, Nov 24, 2021 at 01:23:42PM +0000, Matthew Wilcox wrote:
> > On Wed, Nov 24, 2021 at 03:08:49AM +0000, cgel.zte@gmail.com wrote:
> > > From: chiminghao <chi.minghao@zte.com.cn>
> > >
> > > Fix the following coccinelle report:
> > > ./mm/memory_hotplug.c:2210:2-5:
> > > WARNING Use BUG_ON instead of if condition followed by BUG.
> >
> > What coccinelle script is reporting this?
>
> Maybe I found it?
>
> scripts/coccinelle/misc/bugon.cocci:msg="WARNING: Use BUG_ON instead of if condition followed by BUG.\nPlease make sure the condition has no side effects (see conditional BUG_ON definition in include/asm-generic/bug.h)"
>
> Julia, Michal, can we delete this script, please? It's being abused.
OK
julia
>
> > > - if (try_remove_memory(start, size))
> > > - BUG();
> > > + BUG_ON(try_remove_memory(start, size));
> >
> > I really, really, really do not like this. For functions with
> > side-effects, this is bad style. If it's a pure predicate, then
> > sure, but this is bad.
> >
>
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-11-27 18:07 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-24 3:08 [PATCH] mm: Use BUG_ON instead of if condition followed by BUG cgel.zte
2021-11-24 11:10 ` David Hildenbrand
2021-11-24 13:23 ` Matthew Wilcox
2021-11-24 22:27 ` Andrew Morton
2021-11-24 22:45 ` John Hubbard
2021-11-25 3:32 ` Matthew Wilcox
2021-11-25 5:29 ` John Hubbard
2021-11-25 0:00 ` Matthew Wilcox
2021-11-27 18:05 ` Julia Lawall
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).