page_launder() bug

page_launder() bug

[BERECZ Szabolcs]

Hi!

there is a bug in page_launder introduced with kernel 2.4.3-ac12.
if the swapfile is on a filesystem, then after swapping out some
pages, the system locks up. sometimes it writes an oops message.
I don't know exactly what's the problem, but with the attached
patch it works. (this is just a reverse patch to go back to
pre 2.4.3-ac12, and it's against 2.4.4-ac5)
please fix the bug, or apply this patch until fixed properly.

a question:
why don't you include lkcd or something like that in the
mainstream kernel? it would be much easier to save those annoying
oopses :)

Bye,
Szabi


diff -Nur linux/mm/vmscan.c linux.swapfix/mm/vmscan.c
--- linux/mm/vmscan.c	Sun May  6 15:59:22 2001
+++ linux.swapfix/mm/vmscan.c	Sun May  6 16:07:09 2001
@@ -448,15 +448,9 @@
 	maxscan = nr_inactive_dirty_pages;
 	while ((page_lru = inactive_dirty_list.prev) != &inactive_dirty_list &&
 				maxscan-- > 0) {
-		int dead_swap_page;
-
 		page = list_entry(page_lru, struct page, lru);
 		zone = page->zone;

-		dead_swap_page =
-			(PageSwapCache(page) &&
-			 page_count(page) == (1 + !!page->buffers));
-
 		/* Wrong page on list?! (list corruption, should not happen) */
 		if (!PageInactiveDirty(page)) {
 			printk("VM: page_launder, wrong page on list.\n");
@@ -467,10 +461,9 @@
 		}

 		/* Page is or was in use?  Move it to the active list. */
-		if (!dead_swap_page &&
-		    (PageTestandClearReferenced(page) || page->age > 0 ||
-		     (!page->buffers && page_count(page) > 1) ||
-		     page_ramdisk(page))) {
+		if (PageTestandClearReferenced(page) || page->age > 0 ||
+				(!page->buffers && page_count(page) > 1) ||
+				page_ramdisk(page)) {
 			del_page_from_inactive_dirty_list(page);
 			add_page_to_active_list(page);
 			continue;
@@ -512,11 +505,8 @@
 			if (!writepage)
 				goto page_active;

-			/* First time through? Move it to the back of the list,
-			 * but not if it is a dead swap page. We want to reap
-			 * those as fast as possible.
-			 */
-			if (!launder_loop && !dead_swap_page) {
+			/* First time through? Move it to the back of the list */
+			if (!launder_loop) {
 				list_del(page_lru);
 				list_add(page_lru, &inactive_dirty_list);
 				UnlockPage(page);

--
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.eu.org/Linux-MM/

Re: page_launder() bug

[Jonathan Morton]

>-			 page_count(page) == (1 + !!page->buffers));

Two inversions in a row?  I'd like to see that made more explicit,
otherwise it looks like a bug to me.  Of course, if it IS a bug...

--------------------------------------------------------------
from:     Jonathan "Chromatix" Morton
mail:     chromi@cyberspace.org  (not for attachments)
big-mail: chromatix@penguinpowered.com
uni-mail: j.d.morton@lancaster.ac.uk

The key to knowledge is not to rely on people to teach you it.

Get VNC Server for Macintosh from http://www.chromatix.uklinux.net/vnc/

-----BEGIN GEEK CODE BLOCK-----
Version 3.12
GCS$/E/S dpu(!) s:- a20 C+++ UL++ P L+++ E W+ N- o? K? w--- O-- M++$ V? PS
PE- Y+ PGP++ t- 5- X- R !tv b++ DI+++ D G e+ h+ r++ y+(*)
-----END GEEK CODE BLOCK-----


--
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.eu.org/Linux-MM/

Re: page_launder() bug

[BERECZ Szabolcs]

Hi!

On Sun, 6 May 2001, Jonathan Morton wrote:

> >-			 page_count(page) == (1 + !!page->buffers));
>
> Two inversions in a row?  I'd like to see that made more explicit,
> otherwise it looks like a bug to me.  Of course, if it IS a bug...
it's not a bug.
if page->buffers is zero, than the page_count(page) is 1, and if
page->buffers is other than zero, page_count(page) is 2.
so it checks if page is really used by something.
maybe this last line is not true, but the !!page->buffers is not a bug.

Bye,
Szabi


--
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.eu.org/Linux-MM/

Re: page_launder() bug

[David S. Miller]

Jonathan Morton writes:
 > >-			 page_count(page) == (1 + !!page->buffers));
 > 
 > Two inversions in a row?

It is the most straightforward way to make a '1' or '0'
integer from the NULL state of a pointer.

Later,
David S. Miller
davem@redhat.com
--
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.eu.org/Linux-MM/

Re: page_launder() bug

[Aaron Lehmann]

On Sun, May 06, 2001 at 09:55:26PM -0700, David S. Miller wrote:
> 
> Jonathan Morton writes:
>  > >-			 page_count(page) == (1 + !!page->buffers));
>  > Two inversions in a row?
> It is the most straightforward way to make a '1' or '0'
> integer from the NULL state of a pointer.

page_count(page) == (1 + (page->buffers != 0));

--
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.eu.org/Linux-MM/

Re: page_launder() bug

[Tobias Ringstrom]

On Sun, 6 May 2001, David S. Miller wrote:
> It is the most straightforward way to make a '1' or '0'
> integer from the NULL state of a pointer.

But is it really specified in the C "standards" to be exctly zero or one,
and not zero and non-zero?

IMHO, the ?: construct is way more readable and reliable.

/Tobias

--
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.eu.org/Linux-MM/

Re: page_launder() bug

[David S. Miller]

Tobias Ringstrom writes:
 > But is it really specified in the C "standards" to be exctly zero or one,
 > and not zero and non-zero?

I'm pretty sure it does.

 > IMHO, the ?: construct is way more readable and reliable.

Well identical code has been there for several months just a few lines
away.

I've seen this idiom used in many places (even the GCC sources :-),
so I'm rather surprised people are seeing it for the first time.

Later,
David S. Miller
davem@redhat.com
--
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.eu.org/Linux-MM/

Re: page_launder() bug

[Tobias Ringstrom]

On Mon, 7 May 2001, David S. Miller wrote:
>
> I've seen this idiom used in many places (even the GCC sources :-),
> so I'm rather surprised people are seeing it for the first time.

Or perhaps some of us have seen it one time too many?  :-)

/Tobias

--
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.eu.org/Linux-MM/

Re: page_launder() bug

[Alan Cox]

> > It is the most straightforward way to make a '1' or '0'
> > integer from the NULL state of a pointer.
> 
> But is it really specified in the C "standards" to be exctly zero or one,
> and not zero and non-zero?

Yes. (Fortunately since when this argument occurred Linus said he would eat
his underpants if he was wrong)

Alan

--
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.eu.org/Linux-MM/

Re: page_launder() bug

[Daniel Phillips]

On Monday 07 May 2001 08:26, Tobias Ringstrom wrote:
> On Sun, 6 May 2001, David S. Miller wrote:
> > It is the most straightforward way to make a '1' or '0'
> > integer from the NULL state of a pointer.
>
> But is it really specified in the C "standards" to be exctly zero or
> one, and not zero and non-zero?

Yes, and if we did not have this stupid situation where the C language 
standard is not freely available online then you would not have had to 
ask.</rant>

> IMHO, the ?: construct is way more readable and reliable.

There is no difference in reliability.  Readability is a matter of 
opinion - my opinion is that they are equally readable.  To its credit, 
gcc produces the same ia32 code in either case:

	int foo = 999;
	return 1 + !!foo;

<main+6>:	movl   $0x3e7,0xfffffffc(%ebp)
<main+13>:	cmpl   $0x0,0xfffffffc(%ebp)
<main+17>:	je     0x80483e0 <main+32>
<main+19>:	mov    $0x2,%eax
<main+24>:	jmp    0x80483e5 <main+37>
<main+26>:	lea    0x0(%esi),%esi
<main+32>:	mov    $0x1,%eax
<main+37>:	mov    %eax,%eax

	int foo = 999;
	return foo? 2: 1;

<main+6>:	movl   $0x3e7,0xfffffffc(%ebp)
<main+13>:	cmpl   $0x0,0xfffffffc(%ebp)
<main+17>:	je     0x80483e0 <main+32>
<main+19>:	mov    $0x2,%eax
<main+24>:	jmp    0x80483e5 <main+37>
<main+26>:	lea    0x0(%esi),%esi
<main+32>:	mov    $0x1,%eax
<main+37>:	mov    %eax,%eax

--
Daniel
--
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.eu.org/Linux-MM/

Re: page_launder() bug

[Horst von Brand]

"David S. Miller" <davem@redhat.com> said:
> Jonathan Morton writes:
>  > >-			 page_count(page) == (1 + !!page->buffers));
>  > 
>  > Two inversions in a row?
> 
> It is the most straightforward way to make a '1' or '0'
> integer from the NULL state of a pointer.

IMVHO, it is clearer to write:

  page_count(page) == 1 + (page->buffers != NULL)

At least, the original poster wouldn't have wondered, and I wouldn't have
had to think a bit to find out what it meant... If gcc generates worse code
for this, it should be fixed.
-- 
Dr. Horst H. von Brand                       mailto:vonbrand@inf.utfsm.cl
Departamento de Informatica                     Fono: +56 32 654431
Universidad Tecnica Federico Santa Maria              +56 32 654239
Casilla 110-V, Valparaiso, Chile                Fax:  +56 32 797513
--
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.eu.org/Linux-MM/

Re: page_launder() bug

[Rusty Russell]

In message <15094.10942.592911.70443@pizda.ninka.net> you write:
> 
> Jonathan Morton writes:
>  > >-			 page_count(page) == (1 + !!page->buffers));
>  > 
>  > Two inversions in a row?
> 
> It is the most straightforward way to make a '1' or '0'
> integer from the NULL state of a pointer.

Overall, I'd have to say that this:

-		dead_swap_page =
-			(PageSwapCache(page) &&
-			 page_count(page) == (1 + !!page->buffers));
-

Is nicer as:

		int dead_swap_page = 0;

		if (PageSwapCache(page)
		    && page_count(page) == (page->buffers ? 1 : 2))
			dead_swap_page = 1;

After all, the second is what the code *means* (1 and 2 are magic
numbers).

That said, anyone who doesn't understand the former should probably
get some more C experience before commenting on others' code...

Rusty.
--
Premature optmztion is rt of all evl. --DK
--
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.eu.org/Linux-MM/

Re: page_launder() bug

[Jonathan Morton]

>That said, anyone who doesn't understand the former should probably
>get some more C experience before commenting on others' code...

I understood it, but it looked very much like a typo.

--------------------------------------------------------------
from:     Jonathan "Chromatix" Morton
mail:     chromi@cyberspace.org  (not for attachments)
big-mail: chromatix@penguinpowered.com
uni-mail: j.d.morton@lancaster.ac.uk

The key to knowledge is not to rely on people to teach you it.

Get VNC Server for Macintosh from http://www.chromatix.uklinux.net/vnc/

-----BEGIN GEEK CODE BLOCK-----
Version 3.12
GCS$/E/S dpu(!) s:- a20 C+++ UL++ P L+++ E W+ N- o? K? w--- O-- M++$ V? PS
PE- Y+ PGP++ t- 5- X- R !tv b++ DI+++ D G e+ h+ r++ y+(*)
-----END GEEK CODE BLOCK-----


--
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.eu.org/Linux-MM/

Re: page_launder() bug

[Martin Dalecki]

Rusty Russell wrote:
> 
> In message <15094.10942.592911.70443@pizda.ninka.net> you write:
> >
> > Jonathan Morton writes:
> >  > >-                  page_count(page) == (1 + !!page->buffers));
> >  >
> >  > Two inversions in a row?
> >
> > It is the most straightforward way to make a '1' or '0'
> > integer from the NULL state of a pointer.
> 
> Overall, I'd have to say that this:
> 
> -               dead_swap_page =
> -                       (PageSwapCache(page) &&
> -                        page_count(page) == (1 + !!page->buffers));
> -
> 
> Is nicer as:
> 
>                 int dead_swap_page = 0;
> 
>                 if (PageSwapCache(page)
>                     && page_count(page) == (page->buffers ? 1 : 2))
>                         dead_swap_page = 1;
> 
> After all, the second is what the code *means* (1 and 2 are magic
> numbers).
> 
> That said, anyone who doesn't understand the former should probably
> get some more C experience before commenting on others' code...

Basically Amen.

But there are may be better chances that the compiler does do
better job at branch prediction in the second case? 
Wenn anyway objdump -S should show it...
--
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.eu.org/Linux-MM/