* Re: [PATCH] mm/mmap: replace if (cond) BUG() with BUG_ON()
@ 2021-01-06 4:28 ` Hugh Dickins
0 siblings, 0 replies; 21+ messages in thread
From: Hugh Dickins @ 2021-01-06 4:28 UTC (permalink / raw)
To: Alex Shi, Andrew Morton
Cc: Minchan Kim, Andrea Arcangeli, Michal Hocko, linux-mm, linux-kernel
On Sat, 12 Dec 2020, Alex Shi wrote:
>
> I'm very sorry, a typo here. the patch should be updated:
>
> From ed4fa1c6d5bed5766c5f0c35af0c597855d7be06 Mon Sep 17 00:00:00 2001
> From: Alex Shi <alex.shi@linux.alibaba.com>
> Date: Fri, 11 Dec 2020 21:26:46 +0800
> Subject: [PATCH] mm/mmap: replace if (cond) BUG() with BUG_ON()
>
> coccinelle reports some warnings:
> WARNING: Use BUG_ON instead of if condition followed by BUG.
>
> It could be fixed by BUG_ON().
>
> Reported-by: abaci@linux.alibaba.com
> Signed-off-by: Alex Shi <alex.shi@linux.alibaba.com>
When diffing mmotm just now, I was sorry to find this: NAK.
Alex, please consider why the authors of these lines (whom you
did not Cc) chose to write them without BUG_ON(): it has always
been preferred practice to use BUG_ON() on predicates, but not on
functionally effective statements (sorry, I've forgotten the proper
term: I'd say statements with side-effects, but here they are not
just side-effects: they are their main purpose).
We prefer not to hide those away inside BUG macros: please fix your
"abaci" to respect kernel style here - unless it turns out that the
kernel has moved away from that, and it's me who's behind the times.
Andrew, if you agree, please drop
mm-mmap-replace-if-cond-bug-with-bug_on.patch
from your stack.
(And did Minchan really Ack it? I see an Ack from Minchan to a
similar mm/zsmalloc patch: which surprises me, but is Minchan's
business not mine; but that patch is not in mmotm.)
On the whole, I think there are far too many patches submitted,
where Developer B chooses to rewrite a line to their own preference,
without respecting that Author A chose to write it in another way.
That's great when it really does improve readability, but often not.
Thanks,
Hugh
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: linux-mm@kvack.org
> Cc: linux-kernel@vger.kernel.org
> ---
> mm/mmap.c | 22 ++++++++--------------
> 1 file changed, 8 insertions(+), 14 deletions(-)
>
> diff --git a/mm/mmap.c b/mm/mmap.c
> index 8144fc3c5a78..107fa91bb59f 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -712,9 +712,8 @@ static void __insert_vm_struct(struct mm_struct *mm, struct vm_area_struct *vma)
> struct vm_area_struct *prev;
> struct rb_node **rb_link, *rb_parent;
>
> - if (find_vma_links(mm, vma->vm_start, vma->vm_end,
> - &prev, &rb_link, &rb_parent))
> - BUG();
> + BUG_ON(find_vma_links(mm, vma->vm_start, vma->vm_end,
> + &prev, &rb_link, &rb_parent));
> __vma_link(mm, vma, prev, rb_link, rb_parent);
> mm->map_count++;
> }
> @@ -3585,9 +3584,8 @@ static void vm_lock_anon_vma(struct mm_struct *mm, struct anon_vma *anon_vma)
> * can't change from under us thanks to the
> * anon_vma->root->rwsem.
> */
> - if (__test_and_set_bit(0, (unsigned long *)
> - &anon_vma->root->rb_root.rb_root.rb_node))
> - BUG();
> + BUG_ON(__test_and_set_bit(0, (unsigned long *)
> + &anon_vma->root->rb_root.rb_root.rb_node));
> }
> }
>
> @@ -3603,8 +3601,7 @@ static void vm_lock_mapping(struct mm_struct *mm, struct address_space *mapping)
> * mm_all_locks_mutex, there may be other cpus
> * changing other bitflags in parallel to us.
> */
> - if (test_and_set_bit(AS_MM_ALL_LOCKS, &mapping->flags))
> - BUG();
> + BUG_ON(test_and_set_bit(AS_MM_ALL_LOCKS, &mapping->flags));
> down_write_nest_lock(&mapping->i_mmap_rwsem, &mm->mmap_lock);
> }
> }
> @@ -3701,9 +3698,8 @@ static void vm_unlock_anon_vma(struct anon_vma *anon_vma)
> * can't change from under us until we release the
> * anon_vma->root->rwsem.
> */
> - if (!__test_and_clear_bit(0, (unsigned long *)
> - &anon_vma->root->rb_root.rb_root.rb_node))
> - BUG();
> + BUG_ON(!__test_and_clear_bit(0, (unsigned long *)
> + &anon_vma->root->rb_root.rb_root.rb_node));
> anon_vma_unlock_write(anon_vma);
> }
> }
> @@ -3716,9 +3712,7 @@ static void vm_unlock_mapping(struct address_space *mapping)
> * because we hold the mm_all_locks_mutex.
> */
> i_mmap_unlock_write(mapping);
> - if (!test_and_clear_bit(AS_MM_ALL_LOCKS,
> - &mapping->flags))
> - BUG();
> + BUG_ON(!test_and_clear_bit(AS_MM_ALL_LOCKS, &mapping->flags));
> }
> }
>
> --
> 2.29.GIT
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] mm/mmap: replace if (cond) BUG() with BUG_ON()
2021-01-06 4:28 ` Hugh Dickins
(?)
@ 2021-01-06 8:40 ` Alex Shi
-1 siblings, 0 replies; 21+ messages in thread
From: Alex Shi @ 2021-01-06 8:40 UTC (permalink / raw)
To: Hugh Dickins, Andrew Morton
Cc: Minchan Kim, Andrea Arcangeli, Michal Hocko, linux-mm, linux-kernel
在 2021/1/6 下午12:28, Hugh Dickins 写道:
> On Sat, 12 Dec 2020, Alex Shi wrote:
>>
>> I'm very sorry, a typo here. the patch should be updated:
>>
>> From ed4fa1c6d5bed5766c5f0c35af0c597855d7be06 Mon Sep 17 00:00:00 2001
>> From: Alex Shi <alex.shi@linux.alibaba.com>
>> Date: Fri, 11 Dec 2020 21:26:46 +0800
>> Subject: [PATCH] mm/mmap: replace if (cond) BUG() with BUG_ON()
>>
>> coccinelle reports some warnings:
>> WARNING: Use BUG_ON instead of if condition followed by BUG.
>>
>> It could be fixed by BUG_ON().
>>
>> Reported-by: abaci@linux.alibaba.com
>> Signed-off-by: Alex Shi <alex.shi@linux.alibaba.com>
>
> When diffing mmotm just now, I was sorry to find this: NAK.
>
> Alex, please consider why the authors of these lines (whom you
> did not Cc) chose to write them without BUG_ON(): it has always
> been preferred practice to use BUG_ON() on predicates, but not on
> functionally effective statements (sorry, I've forgotten the proper
> term: I'd say statements with side-effects, but here they are not
> just side-effects: they are their main purpose).
>
Right!
The original line may want to be done whenever the BUG() enabled, I
overlocked this points. Sorry! My fault!
Please revert them.
Thanks
Alex
> We prefer not to hide those away inside BUG macros: please fix your
> "abaci" to respect kernel style here - unless it turns out that the
> kernel has moved away from that, and it's me who's behind the times.
>
> Andrew, if you agree, please drop
> mm-mmap-replace-if-cond-bug-with-bug_on.patch
> from your stack.
>
> (And did Minchan really Ack it? I see an Ack from Minchan to a
> similar mm/zsmalloc patch: which surprises me, but is Minchan's
> business not mine; but that patch is not in mmotm.)
>
> On the whole, I think there are far too many patches submitted,
> where Developer B chooses to rewrite a line to their own preference,
> without respecting that Author A chose to write it in another way.
> That's great when it really does improve readability, but often not.
>
> Thanks,
> Hugh
>
>> Cc: Andrew Morton <akpm@linux-foundation.org>
>> Cc: linux-mm@kvack.org
>> Cc: linux-kernel@vger.kernel.org
>> ---
>> mm/mmap.c | 22 ++++++++--------------
>> 1 file changed, 8 insertions(+), 14 deletions(-)
>>
>> diff --git a/mm/mmap.c b/mm/mmap.c
>> index 8144fc3c5a78..107fa91bb59f 100644
>> --- a/mm/mmap.c
>> +++ b/mm/mmap.c
>> @@ -712,9 +712,8 @@ static void __insert_vm_struct(struct mm_struct *mm, struct vm_area_struct *vma)
>> struct vm_area_struct *prev;
>> struct rb_node **rb_link, *rb_parent;
>>
>> - if (find_vma_links(mm, vma->vm_start, vma->vm_end,
>> - &prev, &rb_link, &rb_parent))
>> - BUG();
>> + BUG_ON(find_vma_links(mm, vma->vm_start, vma->vm_end,
>> + &prev, &rb_link, &rb_parent));
>> __vma_link(mm, vma, prev, rb_link, rb_parent);
>> mm->map_count++;
>> }
>> @@ -3585,9 +3584,8 @@ static void vm_lock_anon_vma(struct mm_struct *mm, struct anon_vma *anon_vma)
>> * can't change from under us thanks to the
>> * anon_vma->root->rwsem.
>> */
>> - if (__test_and_set_bit(0, (unsigned long *)
>> - &anon_vma->root->rb_root.rb_root.rb_node))
>> - BUG();
>> + BUG_ON(__test_and_set_bit(0, (unsigned long *)
>> + &anon_vma->root->rb_root.rb_root.rb_node));
>> }
>> }
>>
>> @@ -3603,8 +3601,7 @@ static void vm_lock_mapping(struct mm_struct *mm, struct address_space *mapping)
>> * mm_all_locks_mutex, there may be other cpus
>> * changing other bitflags in parallel to us.
>> */
>> - if (test_and_set_bit(AS_MM_ALL_LOCKS, &mapping->flags))
>> - BUG();
>> + BUG_ON(test_and_set_bit(AS_MM_ALL_LOCKS, &mapping->flags));
>> down_write_nest_lock(&mapping->i_mmap_rwsem, &mm->mmap_lock);
>> }
>> }
>> @@ -3701,9 +3698,8 @@ static void vm_unlock_anon_vma(struct anon_vma *anon_vma)
>> * can't change from under us until we release the
>> * anon_vma->root->rwsem.
>> */
>> - if (!__test_and_clear_bit(0, (unsigned long *)
>> - &anon_vma->root->rb_root.rb_root.rb_node))
>> - BUG();
>> + BUG_ON(!__test_and_clear_bit(0, (unsigned long *)
>> + &anon_vma->root->rb_root.rb_root.rb_node));
>> anon_vma_unlock_write(anon_vma);
>> }
>> }
>> @@ -3716,9 +3712,7 @@ static void vm_unlock_mapping(struct address_space *mapping)
>> * because we hold the mm_all_locks_mutex.
>> */
>> i_mmap_unlock_write(mapping);
>> - if (!test_and_clear_bit(AS_MM_ALL_LOCKS,
>> - &mapping->flags))
>> - BUG();
>> + BUG_ON(!test_and_clear_bit(AS_MM_ALL_LOCKS, &mapping->flags));
>> }
>> }
>>
>> --
>> 2.29.GIT
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] mm/mmap: replace if (cond) BUG() with BUG_ON()
2021-01-06 4:28 ` Hugh Dickins
(?)
(?)
@ 2021-01-06 19:46 ` Andrew Morton
2021-01-06 20:09 ` Andrea Arcangeli
2021-01-06 20:10 ` Hugh Dickins
-1 siblings, 2 replies; 21+ messages in thread
From: Andrew Morton @ 2021-01-06 19:46 UTC (permalink / raw)
To: Hugh Dickins
Cc: Alex Shi, Minchan Kim, Andrea Arcangeli, Michal Hocko, linux-mm,
linux-kernel
On Tue, 5 Jan 2021 20:28:27 -0800 (PST) Hugh Dickins <hughd@google.com> wrote:
> Alex, please consider why the authors of these lines (whom you
> did not Cc) chose to write them without BUG_ON(): it has always
> been preferred practice to use BUG_ON() on predicates, but not on
> functionally effective statements (sorry, I've forgotten the proper
> term: I'd say statements with side-effects, but here they are not
> just side-effects: they are their main purpose).
>
> We prefer not to hide those away inside BUG macros
Should we change that? I find BUG_ON(something_which_shouldnt_fail())
to be quite natural and readable.
As are things like the existing
BUG_ON(mmap_read_trylock(mm));
BUG_ON(wb_domain_init(&global_wb_domain, GFP_KERNEL));
etc.
No strong opinion here, but is current mostly-practice really
useful?
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] mm/mmap: replace if (cond) BUG() with BUG_ON()
2021-01-06 19:46 ` Andrew Morton
@ 2021-01-06 20:09 ` Andrea Arcangeli
2021-01-06 20:18 ` Hugh Dickins
2021-01-06 20:10 ` Hugh Dickins
1 sibling, 1 reply; 21+ messages in thread
From: Andrea Arcangeli @ 2021-01-06 20:09 UTC (permalink / raw)
To: Andrew Morton
Cc: Hugh Dickins, Alex Shi, Minchan Kim, Michal Hocko, linux-mm,
linux-kernel
Hello,
On Wed, Jan 06, 2021 at 11:46:20AM -0800, Andrew Morton wrote:
> On Tue, 5 Jan 2021 20:28:27 -0800 (PST) Hugh Dickins <hughd@google.com> wrote:
>
> > Alex, please consider why the authors of these lines (whom you
> > did not Cc) chose to write them without BUG_ON(): it has always
> > been preferred practice to use BUG_ON() on predicates, but not on
> > functionally effective statements (sorry, I've forgotten the proper
> > term: I'd say statements with side-effects, but here they are not
> > just side-effects: they are their main purpose).
> >
> > We prefer not to hide those away inside BUG macros
>
> Should we change that? I find BUG_ON(something_which_shouldnt_fail())
> to be quite natural and readable.
>
> As are things like the existing
>
> BUG_ON(mmap_read_trylock(mm));
> BUG_ON(wb_domain_init(&global_wb_domain, GFP_KERNEL));
>
> etc.
>
>
> No strong opinion here, but is current mostly-practice really
> useful?
I'd be surprised if the kernel can boot with BUG_ON() defined as "do
{}while(0)" so I guess it doesn't make any difference.
I've no strong opinion either, but personally my views matches Hugh's
views on this. I certainly tried to stick to that in the past since I
find it cleaner if a bugcheck just "checks" and can be deleted at any
time without sudden breakage.
Said that I also guess we're in the minority.
Thanks,
Andrea
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH] mm/mmap: replace if (cond) BUG() with BUG_ON()
2021-01-06 20:09 ` Andrea Arcangeli
@ 2021-01-06 20:18 ` Hugh Dickins
0 siblings, 0 replies; 21+ messages in thread
From: Hugh Dickins @ 2021-01-06 20:18 UTC (permalink / raw)
To: Andrea Arcangeli
Cc: Andrew Morton, Hugh Dickins, Alex Shi, Minchan Kim, Michal Hocko,
linux-mm, linux-kernel
On Wed, 6 Jan 2021, Andrea Arcangeli wrote:
>
> I'd be surprised if the kernel can boot with BUG_ON() defined as "do
> {}while(0)" so I guess it doesn't make any difference.
I had been afraid of that too, when CONFIG_BUG is not set:
but I think it's actually "if (cond) do {} while (0)".
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH] mm/mmap: replace if (cond) BUG() with BUG_ON()
@ 2021-01-06 20:18 ` Hugh Dickins
0 siblings, 0 replies; 21+ messages in thread
From: Hugh Dickins @ 2021-01-06 20:18 UTC (permalink / raw)
To: Andrea Arcangeli
Cc: Andrew Morton, Hugh Dickins, Alex Shi, Minchan Kim, Michal Hocko,
linux-mm, linux-kernel
On Wed, 6 Jan 2021, Andrea Arcangeli wrote:
>
> I'd be surprised if the kernel can boot with BUG_ON() defined as "do
> {}while(0)" so I guess it doesn't make any difference.
I had been afraid of that too, when CONFIG_BUG is not set:
but I think it's actually "if (cond) do {} while (0)".
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH] mm/mmap: replace if (cond) BUG() with BUG_ON()
2021-01-06 20:18 ` Hugh Dickins
(?)
@ 2021-01-06 20:42 ` Andrew Morton
-1 siblings, 0 replies; 21+ messages in thread
From: Andrew Morton @ 2021-01-06 20:42 UTC (permalink / raw)
To: Hugh Dickins
Cc: Andrea Arcangeli, Alex Shi, Minchan Kim, Michal Hocko, linux-mm,
linux-kernel
On Wed, 6 Jan 2021 12:18:36 -0800 (PST) Hugh Dickins <hughd@google.com> wrote:
> On Wed, 6 Jan 2021, Andrea Arcangeli wrote:
> >
> > I'd be surprised if the kernel can boot with BUG_ON() defined as "do
> > {}while(0)" so I guess it doesn't make any difference.
>
> I had been afraid of that too, when CONFIG_BUG is not set:
> but I think it's actually "if (cond) do {} while (0)".
Yes, that is so. The thinking being that in most cases the compiler
will be smart enough to avoid generating any code for `cond' anyway.
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH] mm/mmap: replace if (cond) BUG() with BUG_ON()
2021-01-06 20:18 ` Hugh Dickins
(?)
(?)
@ 2021-01-07 17:28 ` Vlastimil Babka
2021-01-07 17:36 ` Andrea Arcangeli
-1 siblings, 1 reply; 21+ messages in thread
From: Vlastimil Babka @ 2021-01-07 17:28 UTC (permalink / raw)
To: Hugh Dickins, Andrea Arcangeli
Cc: Andrew Morton, Alex Shi, Minchan Kim, Michal Hocko, linux-mm,
linux-kernel
On 1/6/21 9:18 PM, Hugh Dickins wrote:
> On Wed, 6 Jan 2021, Andrea Arcangeli wrote:
>>
>> I'd be surprised if the kernel can boot with BUG_ON() defined as "do
>> {}while(0)" so I guess it doesn't make any difference.
>
> I had been afraid of that too, when CONFIG_BUG is not set:
> but I think it's actually "if (cond) do {} while (0)".
It's a maze of configs and arch-specific vs generic headers, but I do see this
in include/asm-generic/bug.h:
#else /* !CONFIG_BUG */
#ifndef HAVE_ARCH_BUG
#define BUG() do {} while (1)
#endif
So seems to me there *are* configurations possible where side-effects are indeed
thrown away, right?
WARN_ON is different as the result of the "inner" condition should be further
usable for constructing "outer" conditions:
(still in !CONFIG_BUG section)
#ifndef HAVE_ARCH_WARN_ON
#define WARN_ON(condition) ({
int __ret_warn_on = !!(condition);
unlikely(__ret_warn_on);
})
#endif
For completeness let's look at our own extensions when VM_DEBUG is disabled,
which is quite analogical to disabling CONFIG_BUG and thus it should better be
consistent with the generic stuff.
#define VM_BUG_ON(cond) BUILD_BUG_ON_INVALID(cond)
where BUILD_BUG_ON_INVALID generates no code, so it's consistent with BUG_ON()
and !CONFIG_BUG.
#define VM_WARN_ON(cond) BUILD_BUG_ON_INVALID(cond)
... well that's not consistent with WARN_ON. Hmm if you have asked me before I
checked, I would have said that it is, that I checked it already in the past
and/or there was some discussion already about it. Memory is failing me it
seems. We should better fix this?
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH] mm/mmap: replace if (cond) BUG() with BUG_ON()
2021-01-07 17:28 ` Vlastimil Babka
@ 2021-01-07 17:36 ` Andrea Arcangeli
2021-01-07 17:45 ` Vlastimil Babka
0 siblings, 1 reply; 21+ messages in thread
From: Andrea Arcangeli @ 2021-01-07 17:36 UTC (permalink / raw)
To: Vlastimil Babka
Cc: Hugh Dickins, Andrew Morton, Alex Shi, Minchan Kim, Michal Hocko,
linux-mm, linux-kernel
On Thu, Jan 07, 2021 at 06:28:29PM +0100, Vlastimil Babka wrote:
> On 1/6/21 9:18 PM, Hugh Dickins wrote:
> > On Wed, 6 Jan 2021, Andrea Arcangeli wrote:
> >>
> >> I'd be surprised if the kernel can boot with BUG_ON() defined as "do
> >> {}while(0)" so I guess it doesn't make any difference.
> >
> > I had been afraid of that too, when CONFIG_BUG is not set:
> > but I think it's actually "if (cond) do {} while (0)".
>
> It's a maze of configs and arch-specific vs generic headers, but I do see this
> in include/asm-generic/bug.h:
>
> #else /* !CONFIG_BUG */
> #ifndef HAVE_ARCH_BUG
> #define BUG() do {} while (1)
> #endif
>
> So seems to me there *are* configurations possible where side-effects are indeed
> thrown away, right?
But this not BUG_ON, and that is an infinite loop while(1), not an
optimization away as in while (0) that I was suggesting to just throw
away cond and make it a noop. BUG() is actually the thing to use to
move functional stuff out of BUG_ON so it's not going to be causing
issues if it just loops.
This overall feels mostly an aesthetically issue.
Thanks,
Andrea
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH] mm/mmap: replace if (cond) BUG() with BUG_ON()
2021-01-07 17:36 ` Andrea Arcangeli
@ 2021-01-07 17:45 ` Vlastimil Babka
0 siblings, 0 replies; 21+ messages in thread
From: Vlastimil Babka @ 2021-01-07 17:45 UTC (permalink / raw)
To: Andrea Arcangeli
Cc: Hugh Dickins, Andrew Morton, Alex Shi, Minchan Kim, Michal Hocko,
linux-mm, linux-kernel
On 1/7/21 6:36 PM, Andrea Arcangeli wrote:
> On Thu, Jan 07, 2021 at 06:28:29PM +0100, Vlastimil Babka wrote:
>> On 1/6/21 9:18 PM, Hugh Dickins wrote:
>> > On Wed, 6 Jan 2021, Andrea Arcangeli wrote:
>> >>
>> >> I'd be surprised if the kernel can boot with BUG_ON() defined as "do
>> >> {}while(0)" so I guess it doesn't make any difference.
>> >
>> > I had been afraid of that too, when CONFIG_BUG is not set:
>> > but I think it's actually "if (cond) do {} while (0)".
>>
>> It's a maze of configs and arch-specific vs generic headers, but I do see this
>> in include/asm-generic/bug.h:
>>
>> #else /* !CONFIG_BUG */
>> #ifndef HAVE_ARCH_BUG
>> #define BUG() do {} while (1)
>> #endif
>>
>> So seems to me there *are* configurations possible where side-effects are indeed
>> thrown away, right?
>
> But this not BUG_ON,
Oh, you're right, I got lost in the maze.
> and that is an infinite loop while(1), not an
And I overlooked that "1" too.
So that AFAICS means *both* VM_BUG_ON and VM_WARN_ON behave differently wrt
side-effects when disabled than BUG_ON and WARN_ON.
> optimization away as in while (0) that I was suggesting to just throw
> away cond and make it a noop. BUG() is actually the thing to use to
> move functional stuff out of BUG_ON so it's not going to be causing
> issues if it just loops.
>
> This overall feels mostly an aesthetically issue.
>
> Thanks,
> Andrea
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] mm/mmap: replace if (cond) BUG() with BUG_ON()
2021-01-06 19:46 ` Andrew Morton
@ 2021-01-06 20:10 ` Hugh Dickins
2021-01-06 20:10 ` Hugh Dickins
1 sibling, 0 replies; 21+ messages in thread
From: Hugh Dickins @ 2021-01-06 20:10 UTC (permalink / raw)
To: Andrew Morton
Cc: Hugh Dickins, Alex Shi, Minchan Kim, Andrea Arcangeli,
Michal Hocko, linux-mm, linux-kernel
On Wed, 6 Jan 2021, Andrew Morton wrote:
> On Tue, 5 Jan 2021 20:28:27 -0800 (PST) Hugh Dickins <hughd@google.com> wrote:
>
> > Alex, please consider why the authors of these lines (whom you
> > did not Cc) chose to write them without BUG_ON(): it has always
> > been preferred practice to use BUG_ON() on predicates, but not on
> > functionally effective statements (sorry, I've forgotten the proper
> > term: I'd say statements with side-effects, but here they are not
> > just side-effects: they are their main purpose).
> >
> > We prefer not to hide those away inside BUG macros
>
> Should we change that? I find BUG_ON(something_which_shouldnt_fail())
> to be quite natural and readable.
Fair enough. Whereas my mind tends to filter out the BUG lines when
skimming code, knowing they can be skipped, not needing that effort
to pull out what's inside them.
Perhaps I'm a relic and everyone else is with you: I can only offer
my own preference, which until now was supported by kernel practice.
>
> As are things like the existing
>
> BUG_ON(mmap_read_trylock(mm));
> BUG_ON(wb_domain_init(&global_wb_domain, GFP_KERNEL));
>
> etc.
People say "the exception proves the rule". Perhaps we should invite a
shower of patches to change those? (I'd prefer not, I'm no fan of churn.)
>
> No strong opinion here, but is current mostly-practice really
> useful?
You've seen my vote. Now let the games begin!
Hugh
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] mm/mmap: replace if (cond) BUG() with BUG_ON()
@ 2021-01-06 20:10 ` Hugh Dickins
0 siblings, 0 replies; 21+ messages in thread
From: Hugh Dickins @ 2021-01-06 20:10 UTC (permalink / raw)
To: Andrew Morton
Cc: Hugh Dickins, Alex Shi, Minchan Kim, Andrea Arcangeli,
Michal Hocko, linux-mm, linux-kernel
On Wed, 6 Jan 2021, Andrew Morton wrote:
> On Tue, 5 Jan 2021 20:28:27 -0800 (PST) Hugh Dickins <hughd@google.com> wrote:
>
> > Alex, please consider why the authors of these lines (whom you
> > did not Cc) chose to write them without BUG_ON(): it has always
> > been preferred practice to use BUG_ON() on predicates, but not on
> > functionally effective statements (sorry, I've forgotten the proper
> > term: I'd say statements with side-effects, but here they are not
> > just side-effects: they are their main purpose).
> >
> > We prefer not to hide those away inside BUG macros
>
> Should we change that? I find BUG_ON(something_which_shouldnt_fail())
> to be quite natural and readable.
Fair enough. Whereas my mind tends to filter out the BUG lines when
skimming code, knowing they can be skipped, not needing that effort
to pull out what's inside them.
Perhaps I'm a relic and everyone else is with you: I can only offer
my own preference, which until now was supported by kernel practice.
>
> As are things like the existing
>
> BUG_ON(mmap_read_trylock(mm));
> BUG_ON(wb_domain_init(&global_wb_domain, GFP_KERNEL));
>
> etc.
People say "the exception proves the rule". Perhaps we should invite a
shower of patches to change those? (I'd prefer not, I'm no fan of churn.)
>
> No strong opinion here, but is current mostly-practice really
> useful?
You've seen my vote. Now let the games begin!
Hugh
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] mm/mmap: replace if (cond) BUG() with BUG_ON()
2021-01-06 20:10 ` Hugh Dickins
(?)
@ 2021-01-07 8:33 ` Michal Hocko
-1 siblings, 0 replies; 21+ messages in thread
From: Michal Hocko @ 2021-01-07 8:33 UTC (permalink / raw)
To: Hugh Dickins
Cc: Andrew Morton, Alex Shi, Minchan Kim, Andrea Arcangeli, linux-mm,
linux-kernel
On Wed 06-01-21 12:10:30, Hugh Dickins wrote:
> On Wed, 6 Jan 2021, Andrew Morton wrote:
> > On Tue, 5 Jan 2021 20:28:27 -0800 (PST) Hugh Dickins <hughd@google.com> wrote:
> >
> > > Alex, please consider why the authors of these lines (whom you
> > > did not Cc) chose to write them without BUG_ON(): it has always
> > > been preferred practice to use BUG_ON() on predicates, but not on
> > > functionally effective statements (sorry, I've forgotten the proper
> > > term: I'd say statements with side-effects, but here they are not
> > > just side-effects: they are their main purpose).
> > >
> > > We prefer not to hide those away inside BUG macros
> >
> > Should we change that? I find BUG_ON(something_which_shouldnt_fail())
> > to be quite natural and readable.
>
> Fair enough. Whereas my mind tends to filter out the BUG lines when
> skimming code, knowing they can be skipped, not needing that effort
> to pull out what's inside them.
>
> Perhaps I'm a relic and everyone else is with you: I can only offer
> my own preference, which until now was supported by kernel practice.
I agree with Hugh. BUG_ON on something that is not a trivial predicate
makes the code slightly harder to follow.
I also do agree that accomodating the coding style to the existing code
is better as well because the resulting code is more compact.
In general I consider code transformations like this without a higher
goal that is stated explicitly a pointless churn which doesn't bring
much while it consumes a very scarce review bandwidth. Even when those
look trivial there is always a room to introduce silent breakage.
Be it a checkpatch or coccinelle the change shouldn't be based solely on
the script complains. Really, what is the point of changing an existing
if (cond) BUG into BUG_ON? Fewer lines? Taste? Code consistency?
--
Michal Hocko
SUSE Labs
^ permalink raw reply [flat|nested] 21+ messages in thread