Skip to contentSkip to conversation

Refactoring wb_congested_put()

PATCH In review Jules Irenge 3 participants 6 messages Mar 12, 2020

Subsystems (2)
Memory management MM misc
lore
Share & tools
lore

Jules Irenge

author
#0 lore raw

cover Refactoring wb_congested_put()

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(-)
Signature · 1 line
2.24.1
author
#1 lore raw

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

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(-)
mm/backing-dev.c +13 -10
index 62f05f605fb5..9d47c0b6a42c 100644
@@ -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)
Signature · 1 line
2.24.1
maintainer
#2 lore raw

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

Quoted 35 lines · 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()

...

mm/backing-dev.c +8 -7
@@ -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,

mm/backing-dev.c void wb_congested_put(struct bdi_writeback_congested *congested) · line 482

Quoted 10 lines
mm/backing-dev.c +5 -3
+				 &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)?

#3 lore raw

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

Quoted 5 lines · Andrew Morton

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.

maintainer
#4 lore raw

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

Quoted 8 lines · On Wed, 11 Mar 2020 19:29:48 -0700 Matthew Wilcox <[email protected]> wrote:
Quoted 5 lines · Andrew Morton

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.

Quoted 3 lines

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.

author
#5 lore raw

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

Quoted 11 lines · Andrew Morton
Quoted 8 lines · On Wed, 11 Mar 2020 19:29:48 -0700 Matthew Wilcox <[email protected]> wrote:
Quoted 5 lines · Andrew Morton

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.

Quoted 10 lines

I can well imagine.

Quoted 3 lines

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.