lkml.org 
[lkml]   [2021]   [Jan]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] mm/mmap: replace if (cond) BUG() with BUG_ON()
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

\
 
 \ /
  Last update: 2021-01-06 05:31    [W:0.077 / U:0.604 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site