* [PATCH -v2] mm: frontswap: fix a wrong if condition in frontswap_shrink
@ 2012-09-21 8:40 Zhenzhong Duan
2012-09-27 11:35 ` Paul Bolle
0 siblings, 1 reply; 6+ messages in thread
From: Zhenzhong Duan @ 2012-09-21 8:40 UTC (permalink / raw)
To: linux-kernel@vger.kernel.org, linux-mm
Cc: Dan Magenheimer, Konrad Rzeszutek Wilk, levinsasha928, Feng Jin,
dan.carpenter, 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 <zhenzhong.duan@oracle.com>
diff --git a/mm/frontswap.c b/mm/frontswap.c
index 6b3e71a..e38fc39 100644
--- a/mm/frontswap.c
+++ b/mm/frontswap.c
@@ -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;
}
--
1.7.3
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH -v2] mm: frontswap: fix a wrong if condition in frontswap_shrink
2012-09-21 8:40 [PATCH -v2] mm: frontswap: fix a wrong if condition in frontswap_shrink Zhenzhong Duan
@ 2012-09-27 11:35 ` Paul Bolle
2012-09-28 3:43 ` Zhenzhong Duan
0 siblings, 1 reply; 6+ messages in thread
From: Paul Bolle @ 2012-09-27 11:35 UTC (permalink / raw)
To: zhenzhong.duan
Cc: linux-kernel@vger.kernel.org, linux-mm, Dan Magenheimer,
Konrad Rzeszutek Wilk, levinsasha928, Feng Jin, dan.carpenter
On Fri, 2012-09-21 at 16:40 +0800, Zhenzhong Duan wrote:
> 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 <zhenzhong.duan@oracle.com>
>
> diff --git a/mm/frontswap.c b/mm/frontswap.c
> index 6b3e71a..e38fc39 100644
> --- a/mm/frontswap.c
> +++ b/mm/frontswap.c
> @@ -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;
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.
> - 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;
> }
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 majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH -v2] mm: frontswap: fix a wrong if condition in frontswap_shrink
2012-09-27 11:35 ` Paul Bolle
@ 2012-09-28 3:43 ` Zhenzhong Duan
2012-09-28 14:54 ` Paul Bolle
0 siblings, 1 reply; 6+ messages in thread
From: Zhenzhong Duan @ 2012-09-28 3:43 UTC (permalink / raw)
To: Paul Bolle
Cc: linux-kernel@vger.kernel.org, linux-mm, Dan Magenheimer,
Konrad Rzeszutek Wilk, levinsasha928, Feng Jin, dan.carpenter
On 2012-09-27 19:35, Paul Bolle wrote:
> On Fri, 2012-09-21 at 16:40 +0800, Zhenzhong Duan wrote:
>>
>> @@ -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'.
>> - 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;
>> }
>
> 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
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH -v2] mm: frontswap: fix a wrong if condition in frontswap_shrink
2012-09-28 3:43 ` Zhenzhong Duan
@ 2012-09-28 14:54 ` Paul Bolle
2012-09-29 2:54 ` Zhenzhong Duan
0 siblings, 1 reply; 6+ messages in thread
From: Paul Bolle @ 2012-09-28 14:54 UTC (permalink / raw)
To: zhenzhong.duan
Cc: linux-kernel@vger.kernel.org, linux-mm, Dan Magenheimer,
Konrad Rzeszutek Wilk, levinsasha928, Feng Jin, dan.carpenter
On Fri, 2012-09-28 at 11:43 +0800, Zhenzhong Duan wrote:
> On 2012-09-27 19:35, Paul Bolle wrote:
> > 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.
> >> - 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;
> >> }
> >
> > 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.
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
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH -v2] mm: frontswap: fix a wrong if condition in frontswap_shrink
2012-09-28 14:54 ` Paul Bolle
@ 2012-09-29 2:54 ` Zhenzhong Duan
2012-09-29 8:41 ` Paul Bolle
0 siblings, 1 reply; 6+ messages in thread
From: Zhenzhong Duan @ 2012-09-29 2:54 UTC (permalink / raw)
To: Paul Bolle
Cc: linux-kernel@vger.kernel.org, linux-mm, Dan Magenheimer,
Konrad Rzeszutek Wilk, levinsasha928, Feng Jin, dan.carpenter
On 2012-09-28 22:54, Paul Bolle wrote:
> On Fri, 2012-09-28 at 11:43 +0800, Zhenzhong Duan wrote:
>> On 2012-09-27 19:35, Paul Bolle wrote:
>>> 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
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH -v2] mm: frontswap: fix a wrong if condition in frontswap_shrink
2012-09-29 2:54 ` Zhenzhong Duan
@ 2012-09-29 8:41 ` Paul Bolle
0 siblings, 0 replies; 6+ messages in thread
From: Paul Bolle @ 2012-09-29 8:41 UTC (permalink / raw)
To: zhenzhong.duan
Cc: linux-kernel@vger.kernel.org, linux-mm, Dan Magenheimer,
Konrad Rzeszutek Wilk, levinsasha928, Feng Jin, dan.carpenter
On Sat, 2012-09-29 at 10:54 +0800, Zhenzhong Duan wrote:
> On 2012-09-28 22:54, Paul Bolle wrote:
> > 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
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-09-29 8:41 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-09-21 8:40 [PATCH -v2] mm: frontswap: fix a wrong if condition in frontswap_shrink Zhenzhong Duan
2012-09-27 11:35 ` Paul Bolle
2012-09-28 3:43 ` Zhenzhong Duan
2012-09-28 14:54 ` Paul Bolle
2012-09-29 2:54 ` Zhenzhong Duan
2012-09-29 8:41 ` Paul Bolle
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).