Hi
I expanded the lock function refcount_dec_and_lock_irqsave(&congested->refcnt, &cgwb_lock, &flags)
and rearrange the code so that it can look simple and Sparse will not throw a warning.
Please feel free to leave me any feedback
Thanks
Jules Irenge (1):
backing-dev: refactor wb_congested_put()
mm/backing-dev.c | 23 +++++++++++++----------
1 file changed, 13 insertions(+), 10 deletions(-)
--
2.24.1
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 <[email protected]>
---
mm/backing-dev.c | 23 +++++++++++++----------
1 file changed, 13 insertions(+), 10 deletions(-)
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)
--
2.24.1
On Thu, 12 Mar 2020 00:21:56 +0000 Jules Irenge <[email protected]> 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 <[email protected]> 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 <[email protected]> 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.