From mboxrd@z Thu Jan 1 00:00:00 1970 Date: Thu, 10 May 2007 15:29:57 -0700 From: Andrew Morton Subject: Re: [PATCH] Bug in mm/thrash.c function grab_swap_token() Message-Id: <20070510152957.edb26df3.akpm@linux-foundation.org> In-Reply-To: <20070510122359.GA16433@srv1-m700-lanp.koti> References: <20070510122359.GA16433@srv1-m700-lanp.koti> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sender: owner-linux-mm@kvack.org Return-Path: To: mikukkon@iki.fi Cc: Mika Kukkonen , ashwin.chaugule@celunite.com, linux-mm@kvack.org, Rik van Riel List-ID: On Thu, 10 May 2007 15:24:00 +0300 Mika Kukkonen wrote: > Following bug was uncovered by compiling with '-W' flag: > > CC mm/thrash.o > mm/thrash.c: In function a??grab_swap_tokena??: > mm/thrash.c:52: warning: comparison of unsigned expression < 0 is always false > > Variable token_priority is unsigned, so decrementing first and then > checking the result does not work; fixed by reversing the test, patch > attached (compile tested only). > > I am not sure if likely() makes much sense in this new situation, but > I'll let somebody else to make a decision on that. > > Signed-off-by: Mika Kukkonen > > diff --git a/mm/thrash.c b/mm/thrash.c > index 9ef9071..c4c5205 100644 > --- a/mm/thrash.c > +++ b/mm/thrash.c > @@ -48,9 +48,8 @@ void grab_swap_token(void) > if (current_interval < current->mm->last_interval) > current->mm->token_priority++; > else { > - current->mm->token_priority--; > - if (unlikely(current->mm->token_priority < 0)) > - current->mm->token_priority = 0; > + if (likely(current->mm->token_priority > 0)) > + current->mm->token_priority--; > } > /* Check if we deserve the token */ > if (current->mm->token_priority > argh. This has potential to cause large changes in system performance. -- 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: email@kvack.org From mboxrd@z Thu Jan 1 00:00:00 1970 Subject: Re: [PATCH] Bug in mm/thrash.c function grab_swap_token() From: Ashwin Chaugule Reply-To: ashwin.chaugule@celunite.com In-Reply-To: <20070510152957.edb26df3.akpm@linux-foundation.org> References: <20070510122359.GA16433@srv1-m700-lanp.koti> <20070510152957.edb26df3.akpm@linux-foundation.org> Content-Type: text/plain; charset=UTF-8 Date: Fri, 11 May 2007 12:19:27 +0530 Message-Id: <1178866168.4497.6.camel@localhost.localdomain> Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: owner-linux-mm@kvack.org Return-Path: To: Andrew Morton Cc: mikukkon@iki.fi, Mika Kukkonen , linux-mm@kvack.org, Rik van Riel , ashwin.chaugule@gmail.com List-ID: On Thu, 2007-05-10 at 15:29 -0700, Andrew Morton wrote: > On Thu, 10 May 2007 15:24:00 +0300 > Mika Kukkonen wrote: > > > Following bug was uncovered by compiling with '-W' flag: > > > > CC mm/thrash.o > > mm/thrash.c: In function A?AcAcA?A!A?A?grab_swap_tokenA?AcAcA?A!AcA?Ac: > > mm/thrash.c:52: warning: comparison of unsigned expression < 0 is always false > > > > Variable token_priority is unsigned, so decrementing first and then > > checking the result does not work; fixed by reversing the test, patch > > attached (compile tested only). > > > > I am not sure if likely() makes much sense in this new situation, but > > I'll let somebody else to make a decision on that. > > > > Signed-off-by: Mika Kukkonen > > > > diff --git a/mm/thrash.c b/mm/thrash.c > > index 9ef9071..c4c5205 100644 > > --- a/mm/thrash.c > > +++ b/mm/thrash.c > > @@ -48,9 +48,8 @@ void grab_swap_token(void) > > if (current_interval < current->mm->last_interval) > > current->mm->token_priority++; > > else { > > - current->mm->token_priority--; > > - if (unlikely(current->mm->token_priority < 0)) > > - current->mm->token_priority = 0; > > + if (likely(current->mm->token_priority > 0)) > > + current->mm->token_priority--; > > } > > /* Check if we deserve the token */ > > if (current->mm->token_priority > > > argh. > > This has potential to cause large changes in system performance. I'm not sure how. Although, I think the logic still remains the same. IOW, if the prio decrements to zero, it will remain zero until it contends for token rapidly. The likely part is unnecessary. Thanks Mika. Let me submit this patch, coz I'd like to change my email address in thrash.c -- 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: email@kvack.org From mboxrd@z Thu Jan 1 00:00:00 1970 Received: by an-out-0708.google.com with SMTP id c10so209372ana for ; Fri, 11 May 2007 00:05:51 -0700 (PDT) Message-ID: <89af10f90705102358q58d4b07bmbaba1e511edd928b@mail.gmail.com> Date: Fri, 11 May 2007 12:28:51 +0530 From: "ashwin chaugule" Subject: Re: [PATCH] Bug in mm/thrash.c function grab_swap_token() In-Reply-To: <1178866168.4497.6.camel@localhost.localdomain> MIME-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Content-Disposition: inline References: <20070510122359.GA16433@srv1-m700-lanp.koti> <20070510152957.edb26df3.akpm@linux-foundation.org> <1178866168.4497.6.camel@localhost.localdomain> Sender: owner-linux-mm@kvack.org Return-Path: To: Andrew Morton Cc: mikukkon@iki.fi, Mika Kukkonen , linux-mm@kvack.org, Rik van Riel List-ID: This patch fixes a bug discovered by Mika Kukkonen in the swap token code. An unsigned int was being decremented and then checked for < 0. Signed-off-by: Ashwin Chaugule diff --git a/mm/thrash.c b/mm/thrash.c index 9ef9071..60f3344 100644 --- a/mm/thrash.c +++ b/mm/thrash.c @@ -8,7 +8,7 @@ * Simple token based thrashing protection, using the algorithm * described in: http://www.cs.wm.edu/~sjiang/token.pdf * - * Sep 2006, Ashwin Chaugule + * Sep 2006, Ashwin Chaugule * Improved algorithm to pass token: * Each task has a priority which is incremented if it contended * for the token in an interval less than its previous attempt. @@ -48,9 +48,8 @@ void grab_swap_token(void) if (current_interval < current->mm->last_interval) current->mm->token_priority++; else { - current->mm->token_priority--; - if (unlikely(current->mm->token_priority < 0)) - current->mm->token_priority = 0; + if (current->mm->token_priority > 0) + current->mm->token_priority--; } /* Check if we deserve the token */ if (current->mm->token_priority > -- 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: email@kvack.org From mboxrd@z Thu Jan 1 00:00:00 1970 Message-ID: <464771CA.3080906@redhat.com> Date: Sun, 13 May 2007 16:15:06 -0400 From: Rik van Riel MIME-Version: 1.0 Subject: Re: [PATCH] Bug in mm/thrash.c function grab_swap_token() References: <20070510122359.GA16433@srv1-m700-lanp.koti> <20070510152957.edb26df3.akpm@linux-foundation.org> <1178866168.4497.6.camel@localhost.localdomain> In-Reply-To: <1178866168.4497.6.camel@localhost.localdomain> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Sender: owner-linux-mm@kvack.org Return-Path: To: ashwin.chaugule@celunite.com Cc: Andrew Morton , mikukkon@iki.fi, Mika Kukkonen , linux-mm@kvack.org, ashwin.chaugule@gmail.com List-ID: Ashwin Chaugule wrote: > On Thu, 2007-05-10 at 15:29 -0700, Andrew Morton wrote: >> On Thu, 10 May 2007 15:24:00 +0300 >> Mika Kukkonen wrote: >> >>> Following bug was uncovered by compiling with '-W' flag: >>> >>> CC mm/thrash.o >>> mm/thrash.c: In function A?AcAcA?A!A?A?grab_swap_tokenA?AcAcA?A!AcA?Ac: >>> mm/thrash.c:52: warning: comparison of unsigned expression < 0 is always false >>> >>> Variable token_priority is unsigned, so decrementing first and then >>> checking the result does not work; fixed by reversing the test, patch >>> attached (compile tested only). >>> >>> I am not sure if likely() makes much sense in this new situation, but >>> I'll let somebody else to make a decision on that. >>> >>> Signed-off-by: Mika Kukkonen >>> >>> diff --git a/mm/thrash.c b/mm/thrash.c >>> index 9ef9071..c4c5205 100644 >>> --- a/mm/thrash.c >>> +++ b/mm/thrash.c >>> @@ -48,9 +48,8 @@ void grab_swap_token(void) >>> if (current_interval < current->mm->last_interval) >>> current->mm->token_priority++; >>> else { >>> - current->mm->token_priority--; >>> - if (unlikely(current->mm->token_priority < 0)) >>> - current->mm->token_priority = 0; >>> + if (likely(current->mm->token_priority > 0)) >>> + current->mm->token_priority--; >>> } >>> /* Check if we deserve the token */ >>> if (current->mm->token_priority > >> argh. >> >> This has potential to cause large changes in system performance. > > I'm not sure how. Although, I think the logic still remains the same. > IOW, if the prio decrements to zero, it will remain zero until it > contends for token rapidly. The problem is that the original code would decrement an unsigned variable beyond zero, underflowing to a number just under 2^32... That would make the process basically a permanent owner of the swap token. -- Politics is the struggle between those who want to make their country the best in the world, and those who believe it already is. Each group calls the other unpatriotic. -- 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: email@kvack.org From mboxrd@z Thu Jan 1 00:00:00 1970 Received: by an-out-0708.google.com with SMTP id c10so390663ana for ; Mon, 14 May 2007 00:38:16 -0700 (PDT) Message-ID: <89af10f90705140038u1a592909yf212294c4b28c967@mail.gmail.com> Date: Mon, 14 May 2007 13:08:16 +0530 From: "ashwin chaugule" Subject: Re: [PATCH] Bug in mm/thrash.c function grab_swap_token() In-Reply-To: <464771CA.3080906@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Content-Disposition: inline References: <20070510122359.GA16433@srv1-m700-lanp.koti> <20070510152957.edb26df3.akpm@linux-foundation.org> <1178866168.4497.6.camel@localhost.localdomain> <464771CA.3080906@redhat.com> Sender: owner-linux-mm@kvack.org Return-Path: To: Rik van Riel Cc: ashwin.chaugule@celunite.com, Andrew Morton , mikukkon@iki.fi, Mika Kukkonen , linux-mm@kvack.org List-ID: > >>> @@ -48,9 +48,8 @@ void grab_swap_token(void) > >>> if (current_interval < current->mm->last_interval) > >>> current->mm->token_priority++; > >>> else { > >>> - current->mm->token_priority--; > >>> - if (unlikely(current->mm->token_priority < 0)) > >>> - current->mm->token_priority = 0; > >>> + if (likely(current->mm->token_priority > 0)) > >>> + current->mm->token_priority--; > >>> } > >>> /* Check if we deserve the token */ > >>> if (current->mm->token_priority > > >> argh. > >> > >> This has potential to cause large changes in system performance. > > > > I'm not sure how. Although, I think the logic still remains the same. > > IOW, if the prio decrements to zero, it will remain zero until it > > contends for token rapidly. > > The problem is that the original code would decrement an > unsigned variable beyond zero, underflowing to a number > just under 2^32... > > That would make the process basically a permanent owner > of the swap token. Hm. Although, the probability of token_prio going below zero in the previous code was quite small. Nevertheless, I shall run the new fix through some tests and post the results asap. Cheers, Ashwin -- 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: email@kvack.org