[PATCH 0/1] Refactoring wb_congested_put()

[PATCH 0/1] Refactoring wb_congested_put()

[Jules Irenge]

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

[PATCH 1/1] backing-dev: refactor wb_congested_put()

[Jules Irenge]

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(-)

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

Re: [PATCH 1/1] backing-dev: refactor wb_congested_put()

[Andrew Morton]

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)?

Re: [PATCH 1/1] backing-dev: refactor wb_congested_put()

[Matthew Wilcox]

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.

Re: [PATCH 1/1] backing-dev: refactor wb_congested_put()

[Andrew Morton]

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.

Re: [PATCH 1/1] backing-dev: refactor wb_congested_put()

[Jules Irenge]

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.

Re: [PATCH 1/1] backing-dev: refactor wb_congested_put()

[Jules Irenge]

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.