| Message ID | 1607743586-80303-2-git-send-email-alex.shi@linux.alibaba.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm/mmap: replace if (cond) BUG() with BUG_ON() | expand |
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> 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)); } }
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
在 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
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?
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
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
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)".
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.
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?
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?
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
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 >
diff --git a/mm/mmap.c b/mm/mmap.c index 8144fc3c5a78..2dab93dedae6 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(); + BUF_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)); } }
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> 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(-)