LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH 0/1] Refactoring wb_congested_put()
@ 2020-03-12  0:21 Jules Irenge
  2020-03-12  0:21 ` [PATCH 1/1] backing-dev: refactor wb_congested_put() Jules Irenge
  0 siblings, 1 reply; 6+ messages in thread
From: Jules Irenge @ 2020-03-12  0:21 UTC (permalink / raw)
  To: boqun.feng; +Cc: linux-mm, akpm, linux-kernel

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 1/1] backing-dev: refactor wb_congested_put()
  2020-03-12  0:21 [PATCH 0/1] Refactoring wb_congested_put() Jules Irenge
@ 2020-03-12  0:21 ` Jules Irenge
  2020-03-12  0:59   ` Andrew Morton
  0 siblings, 1 reply; 6+ messages in thread
From: Jules Irenge @ 2020-03-12  0:21 UTC (permalink / raw)
  To: boqun.feng; +Cc: linux-mm, akpm, linux-kernel

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/1] backing-dev: refactor wb_congested_put()
  2020-03-12  0:21 ` [PATCH 1/1] backing-dev: refactor wb_congested_put() Jules Irenge
@ 2020-03-12  0:59   ` Andrew Morton
  2020-03-12  2:29     ` Matthew Wilcox
  0 siblings, 1 reply; 6+ messages in thread
From: Andrew Morton @ 2020-03-12  0:59 UTC (permalink / raw)
  To: Jules Irenge; +Cc: boqun.feng, linux-mm, linux-kernel

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/1] backing-dev: refactor wb_congested_put()
  2020-03-12  0:59   ` Andrew Morton
@ 2020-03-12  2:29     ` Matthew Wilcox
  2020-03-12  3:00       ` Andrew Morton
  0 siblings, 1 reply; 6+ messages in thread
From: Matthew Wilcox @ 2020-03-12  2:29 UTC (permalink / raw)
  To: Andrew Morton; +Cc: Jules Irenge, boqun.feng, linux-mm, linux-kernel

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.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/1] backing-dev: refactor wb_congested_put()
  2020-03-12  2:29     ` Matthew Wilcox
@ 2020-03-12  3:00       ` Andrew Morton
  2020-03-12  9:56         ` Jules Irenge
  0 siblings, 1 reply; 6+ messages in thread
From: Andrew Morton @ 2020-03-12  3:00 UTC (permalink / raw)
  To: Matthew Wilcox; +Cc: Jules Irenge, boqun.feng, linux-mm, linux-kernel

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.


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/1] backing-dev: refactor wb_congested_put()
  2020-03-12  3:00       ` Andrew Morton
@ 2020-03-12  9:56         ` Jules Irenge
  0 siblings, 0 replies; 6+ messages in thread
From: Jules Irenge @ 2020-03-12  9:56 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Matthew Wilcox, Jules Irenge, boqun.feng, linux-mm, linux-kernel



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.

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2020-03-12  9:56 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-12  0:21 [PATCH 0/1] Refactoring wb_congested_put() Jules Irenge
2020-03-12  0:21 ` [PATCH 1/1] backing-dev: refactor wb_congested_put() Jules Irenge
2020-03-12  0:59   ` Andrew Morton
2020-03-12  2:29     ` Matthew Wilcox
2020-03-12  3:00       ` Andrew Morton
2020-03-12  9:56         ` Jules Irenge

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).