From: chiminghao <[email protected]>
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 <[email protected]>
Signed-off-by: chiminghao <[email protected]>
---
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
On 24.11.21 04:08, [email protected] wrote:
> From: chiminghao <[email protected]>
"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 <[email protected]>
> Signed-off-by: chiminghao <[email protected]>
> ---
> 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 <[email protected]>
--
Thanks,
David / dhildenb
On Wed, Nov 24, 2021 at 03:08:49AM +0000, [email protected] wrote:
> From: chiminghao <[email protected]>
>
> 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.
On Wed, 24 Nov 2021 13:23:42 +0000 Matthew Wilcox <[email protected]> wrote:
> On Wed, Nov 24, 2021 at 03:08:49AM +0000, [email protected] wrote:
> > From: chiminghao <[email protected]>
> >
> > 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.
On 11/24/21 14:27, Andrew Morton wrote:
> On Wed, 24 Nov 2021 13:23:42 +0000 Matthew Wilcox <[email protected]> wrote:
>
>> On Wed, Nov 24, 2021 at 03:08:49AM +0000, [email protected] wrote:
>>> From: chiminghao <[email protected]>
>>>
>>> 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
On Wed, Nov 24, 2021 at 01:23:42PM +0000, Matthew Wilcox wrote:
> On Wed, Nov 24, 2021 at 03:08:49AM +0000, [email protected] wrote:
> > From: chiminghao <[email protected]>
> >
> > 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.
>
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.
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
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, [email protected] wrote:
> > > From: chiminghao <[email protected]>
> > >
> > > 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.
> >
>