mm: frontswap: fix a wrong if condition in frontswap_shrink

PATCH In review Zhenzhong Duan 2 participants 6 messages Sep 21, 2012 – Sep 29, 2012 Memory management

Share & tools

Zhenzhong Duan

author

2012-09-21 08:39 UTC #0 lore raw

PATCH 1/1 mm: frontswap: fix a wrong if condition in frontswap_shrink

pages_to_unuse is set to 0 to unuse all frontswap pages But that doesn't happen since a wrong condition in frontswap_shrink cancel it.

-v2: Add comment to explain return value of __frontswap_shrink, as suggested by Dan Carpenter, thanks

Signed-off-by Zhenzhong Duan <[email protected]>

mm/frontswap.c +7 -2

index 6b3e71a..e38fc39 100644
@@ -263,6 +263,11 @@ static int __frontswap_unuse_pages(unsigned long total, unsigned long *unused,
 	return ret;
 }
 
+/*
+ * Used to check if it's necessory and feasible to unuse pages.
+ * Return 1 when nothing to do, 0 when need to shink pages,
+ * error code when there is an error.
+ */
 static int __frontswap_shrink(unsigned long target_pages,
 				unsigned long *pages_to_unuse,
 				int *type)

@@ -275,7 +280,7 @@ static int __frontswap_shrink(unsigned long target_pages,
 	if (total_pages <= target_pages) {
 		/* Nothing to do */
 		*pages_to_unuse = 0;
-		return 0;
+		return 1;
 	}
 	total_pages_to_unuse = total_pages - target_pages;
 	return __frontswap_unuse_pages(total_pages_to_unuse, pages_to_unuse, type);

@@ -302,7 +307,7 @@ void frontswap_shrink(unsigned long target_pages)
 	spin_lock(&swap_lock);
 	ret = __frontswap_shrink(target_pages, &pages_to_unuse, &type);
 	spin_unlock(&swap_lock);
-	if (ret == 0 && pages_to_unuse)
+	if (ret == 0)
 		try_to_unuse(type, true, pages_to_unuse);
 	return;
 }

Signature · 7 lines

1.7.3

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Paul Bolle

2012-09-27 11:35 UTC #1 lore raw

Quoted 26 lines · Zhenzhong Duan

pages_to_unuse is set to 0 to unuse all frontswap pages But that doesn't happen since a wrong condition in frontswap_shrink cancel it.
-v2: Add comment to explain return value of __frontswap_shrink, as suggested by Dan Carpenter, thanks
Signed-off-by Zhenzhong Duan <[email protected]>
mm/frontswap.c +5 -0
index 6b3e71a..e38fc39 100644
@@ -263,6 +263,11 @@ static int __frontswap_unuse_pages(unsigned long total, unsigned long *unused,
 	return ret;
 }
 
+/*
+ * Used to check if it's necessory and feasible to unuse pages.
+ * Return 1 when nothing to do, 0 when need to shink pages,
+ * error code when there is an error.
+ */
 static int __frontswap_shrink(unsigned long target_pages,
 				unsigned long *pages_to_unuse,
 				int *type)

mm/frontswap.c static int __frontswap_shrink(unsigned long target_pages, · line 282

Quoted 3 lines

mm/frontswap.c +0 -0

@@ -275,7 +280,7 @@ static int __frontswap_shrink(unsigned long target_pages,
 	if (total_pages <= target_pages) {
 		/* Nothing to do */
 		*pages_to_unuse = 0;

I think setting pages_to_unuse to zero here is not needed. It is initiated to zero in frontswap_shrink() and hasn't been touched since. See my patch at https://lkml.org/lkml/2012/9/27/250.

Quoted 6 lines

mm/frontswap.c +1 -1

-		return 0;
+		return 1;
 	}
 	total_pages_to_unuse = total_pages - target_pages;
 	return __frontswap_unuse_pages(total_pages_to_unuse, pages_to_unuse, type);

mm/frontswap.c void frontswap_shrink(unsigned long target_pages) · line 313

Quoted 8 lines

mm/frontswap.c +1 -1

@@ -302,7 +307,7 @@ void frontswap_shrink(unsigned long target_pages)
 	spin_lock(&swap_lock);
 	ret = __frontswap_shrink(target_pages, &pages_to_unuse, &type);
 	spin_unlock(&swap_lock);
-	if (ret == 0 && pages_to_unuse)
+	if (ret == 0)
 		try_to_unuse(type, true, pages_to_unuse);
 	return;
 }

Are you sure pages_to_unuse won't be zero here? I've stared quite a bit at __frontswap_unuse_pages() and it's not obvious pages_to_unuse (there also called unused) will never be zero when that function returns zero.

Paul Bolle

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Zhenzhong Duan

author

2012-09-28 03:44 UTC #2 lore raw

Quoted 10 lines · Paul Bolle

mm/frontswap.c static int __frontswap_shrink(unsigned long target_pages, · line 282
Quoted 5 lines · Zhenzhong Duan
mm/frontswap.c +0 -0
@@ -275,7 +280,7 @@ static int __frontswap_shrink(unsigned long target_pages,
  	if (total_pages<= target_pages) {
  		/* Nothing to do */
  		*pages_to_unuse = 0;
I think setting pages_to_unuse to zero here is not needed. It is initiated to zero in frontswap_shrink() and hasn't been touched since. See my patch at https://lkml.org/lkml/2012/9/27/250.

Yes, it's unneeded. But I didn't see warning as you said in above link when run 'make V=1 mm/frontswap.o'.

Quoted 10 lines

mm/frontswap.c +1 -2

-		return 0;
+		return 1;
  	}
  	total_pages_to_unuse = total_pages - target_pages;
  	return __frontswap_unuse_pages(total_pages_to_unuse, pages_to_unuse, type);

@@ -302,7 +307,7 @@ void frontswap_shrink(unsigned long target_pages)
  	spin_lock(&swap_lock);
  	ret = __frontswap_shrink(target_pages,&pages_to_unuse,&type);
  	spin_unlock(&swap_lock);
-	if (ret == 0&&  pages_to_unuse)

Quoted 8 lines

mm/frontswap.c void frontswap_shrink(unsigned long target_pages) · line 313
Quoted 4 lines
mm/frontswap.c +1 -0
+	if (ret == 0)
  		try_to_unuse(type, true, pages_to_unuse);
  	return;
  }
Are you sure pages_to_unuse won't be zero here? I've stared quite a bit at __frontswap_unuse_pages() and it's not obvious pages_to_unuse (there also called unused) will never be zero when that function returns zero.

pages_to_unuse==0 means all pages need to be unused.

zduan

Paul Bolle

2012-09-28 14:54 UTC #3 lore raw

Quoted 6 lines · Zhenzhong Duan

Quoted 3 lines · Paul Bolle
I think setting pages_to_unuse to zero here is not needed. It is initiated to zero in frontswap_shrink() and hasn't been touched since. See my patch at https://lkml.org/lkml/2012/9/27/250.
Yes, it's unneeded. But I didn't see warning as you said in above link when run 'make V=1 mm/frontswap.o'.

Not even before applying your patch? Anyhow, after applying your patch the warnings gone here too.

Quoted 18 lines

Quoted 10 lines
Quoted 10 lines
mm/frontswap.c +1 -2
-		return 0;
+		return 1;
  	}
  	total_pages_to_unuse = total_pages - target_pages;
  	return __frontswap_unuse_pages(total_pages_to_unuse, pages_to_unuse, type);
@@ -302,7 +307,7 @@ void frontswap_shrink(unsigned long target_pages)
  	spin_lock(&swap_lock);
  	ret = __frontswap_shrink(target_pages,&pages_to_unuse,&type);
  	spin_unlock(&swap_lock);
-	if (ret == 0&&  pages_to_unuse)
Quoted 8 lines
mm/frontswap.c void frontswap_shrink(unsigned long target_pages) · line 313
Quoted 4 lines
mm/frontswap.c +1 -0
+	if (ret == 0)
  		try_to_unuse(type, true, pages_to_unuse);
  	return;
  }
Are you sure pages_to_unuse won't be zero here? I've stared quite a bit at __frontswap_unuse_pages() and it's not obvious pages_to_unuse (there also called unused) will never be zero when that function returns zero.

Quoted 1 line

pages_to_unuse==0 means all pages need to be unused.

Ah, now I see. I was focusing on changing the code as little as possible and didn't realize that you actually wanted to change behavior here. Looking at it again this change makes sense (though I hardly understand frontswap, so I can't properly evaluate it). Anyhow, as I said, your patch also does what I care about - silence a warning - so we might as well forget about my patch.

Thanks,

Paul Bolle

Zhenzhong Duan

author

2012-09-29 02:54 UTC #4 lore raw

Quoted 9 lines · Paul Bolle

Quoted 6 lines · Zhenzhong Duan
Quoted 3 lines · Paul Bolle
I think setting pages_to_unuse to zero here is not needed. It is initiated to zero in frontswap_shrink() and hasn't been touched since. See my patch at https://lkml.org/lkml/2012/9/27/250.
Yes, it's unneeded. But I didn't see warning as you said in above link when run 'make V=1 mm/frontswap.o'.
Not even before applying your patch? Anyhow, after applying your patch the warnings gone here too.

I tested both cases, no warning, also didn't see -Wmaybe-uninitialized when make. My env is el5. gcc version 4.1.2 20080704 (Red Hat 4.1.2-52)
Maybe your gcc built in/implicit spec use that option?
thanks
zduan

Paul Bolle

2012-09-29 08:41 UTC #5 lore raw

Quoted 7 lines · Zhenzhong Duan

Quoted 2 lines · Paul Bolle
Not even before applying your patch? Anyhow, after applying your patch the warnings gone here too.
I tested both cases, no warning, also didn't see -Wmaybe-uninitialized when make.
My env is el5. gcc version 4.1.2 20080704 (Red Hat 4.1.2-52) Maybe your gcc built in/implicit spec use that option?

I simply use what (was and) is shipped by Fedora 17:

    $ sudo grep -w gcc /var/log/yum.log 
    Sep 12 11:45:54 Installed: gcc-4.7.0-5.fc17.x86_64
    Sep 27 13:54:24 Updated: gcc-4.7.2-2.fc17.x86_64

So I did my patch with a version of GCC's release 4.7.0, and tested your patch with a version of GCC's 4.7.2 release.

I don't think I tweaked any settings. Unless there are strong reasons to do otherwise, I try to use the tools shipped by Fedora in their default settings. (I'm not even sure there's a way to set one's GCC settings locally.)

Paul Bolle