| Message ID | 20200312002156.49023-2-jbi.octave@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Refactoring wb_congested_put() | expand |
On Thu, 12 Mar 2020 00:21:56 +0000 Jules Irenge <jbi.octave@gmail.com> wrote: > wb_congested_put() was written in such a way that made it difficult > for Sparse tool not to complain > Expanding the function locking block in the if statement improves on > the readability of the code. Rewritting it comes with one add-on: > > It fixes a warning reported by Sparse tool at wb_congested_put() > > warning: context imbalance in wb_congested_put() - unexpected unlock > > Refactor the function wb_congested_put() > > ... > > --- a/mm/backing-dev.c > +++ b/mm/backing-dev.c > @@ -464,18 +464,21 @@ void wb_congested_put(struct bdi_writeback_congested *congested) > { > unsigned long flags; > > - if (!refcount_dec_and_lock_irqsave(&congested->refcnt, &cgwb_lock, &flags)) > - return; > - > + if (!refcount_dec_not_one(&congested->refcnt)) { > + spin_lock_irqsave(&cgwb_lock, flags); > + if (!refcount_dec_and_test(&congested->refcnt)) { > + spin_unlock_irqrestore(&cgwb_lock, flags); > + return; > + } > /* bdi might already have been destroyed leaving @congested unlinked */ > - if (congested->__bdi) { > - rb_erase(&congested->rb_node, > - &congested->__bdi->cgwb_congested_tree); > - congested->__bdi = NULL; > + if (congested->__bdi) { > + rb_erase(&congested->rb_node, > + &congested->__bdi->cgwb_congested_tree); > + congested->__bdi = NULL; > + } > + spin_unlock_irqrestore(&cgwb_lock, flags); > + kfree(congested); > } > - > - spin_unlock_irqrestore(&cgwb_lock, flags); > - kfree(congested); > } hm, it's hard to get excited over this. Open-coding the refcount_dec_and_lock_irqsave() internals at a callsite in order to make sparse happy. Is there some other way, using __acquires (for example)?
On Wed, Mar 11, 2020 at 05:59:19PM -0700, Andrew Morton wrote: > hm, it's hard to get excited over this. Open-coding the > refcount_dec_and_lock_irqsave() internals at a callsite in order to > make sparse happy. > > Is there some other way, using __acquires (for example)? sparse is really bad at conditional lock acquisition. we have similar problems over the vfs. but we shouldn't be obfuscating our code to make the tool happy.
On Wed, 11 Mar 2020 19:29:48 -0700 Matthew Wilcox <willy@infradead.org> wrote: > On Wed, Mar 11, 2020 at 05:59:19PM -0700, Andrew Morton wrote: > > hm, it's hard to get excited over this. Open-coding the > > refcount_dec_and_lock_irqsave() internals at a callsite in order to > > make sparse happy. > > > > Is there some other way, using __acquires (for example)? > > sparse is really bad at conditional lock acquisition. I can well imagine. > we have similar > problems over the vfs. but we shouldn't be obfuscating our code to make > the tool happy. Perhaps sparse needs a way of being directed to suppress checking across a particular function.
On Wed, 11 Mar 2020, Andrew Morton wrote: > On Wed, 11 Mar 2020 19:29:48 -0700 Matthew Wilcox <willy@infradead.org> wrote: > > > On Wed, Mar 11, 2020 at 05:59:19PM -0700, Andrew Morton wrote: > > > hm, it's hard to get excited over this. Open-coding the > > > refcount_dec_and_lock_irqsave() internals at a callsite in order to > > > make sparse happy. > > > > > > Is there some other way, using __acquires (for example)? > > > > sparse is really bad at conditional lock acquisition. > > I can well imagine. > > > we have similar > > problems over the vfs. but we shouldn't be obfuscating our code to make > > the tool happy. > > Perhaps sparse needs a way of being directed to suppress checking > across a particular function. > > Thanks for the feedback, maybe this is a limitation for Sparse. I have experienced quite often this problem.
diff --git a/mm/backing-dev.c b/mm/backing-dev.c index 62f05f605fb5..9d47c0b6a42c 100644 --- a/mm/backing-dev.c +++ b/mm/backing-dev.c @@ -464,18 +464,21 @@ void wb_congested_put(struct bdi_writeback_congested *congested) { unsigned long flags; - if (!refcount_dec_and_lock_irqsave(&congested->refcnt, &cgwb_lock, &flags)) - return; - + if (!refcount_dec_not_one(&congested->refcnt)) { + spin_lock_irqsave(&cgwb_lock, flags); + if (!refcount_dec_and_test(&congested->refcnt)) { + spin_unlock_irqrestore(&cgwb_lock, flags); + return; + } /* bdi might already have been destroyed leaving @congested unlinked */ - if (congested->__bdi) { - rb_erase(&congested->rb_node, - &congested->__bdi->cgwb_congested_tree); - congested->__bdi = NULL; + if (congested->__bdi) { + rb_erase(&congested->rb_node, + &congested->__bdi->cgwb_congested_tree); + congested->__bdi = NULL; + } + spin_unlock_irqrestore(&cgwb_lock, flags); + kfree(congested); } - - spin_unlock_irqrestore(&cgwb_lock, flags); - kfree(congested); } static void cgwb_release_workfn(struct work_struct *work)
wb_congested_put() was written in such a way that made it difficult for Sparse tool not to complain Expanding the function locking block in the if statement improves on the readability of the code. Rewritting it comes with one add-on: It fixes a warning reported by Sparse tool at wb_congested_put() warning: context imbalance in wb_congested_put() - unexpected unlock Refactor the function wb_congested_put() Signed-off-by: Jules Irenge <jbi.octave@gmail.com> --- mm/backing-dev.c | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-)