[PATCH] Re: kswapd

[PATCH] Re: kswapd

[Rik van Riel]

On Sun, 26 Mar 2000, Russell King wrote:

> I think I've solved (very dirtily) my kswapd problem

Your patch is the correct one. I've added an extra reschedule
point and cleaned up the code a little bit. I wonder who sent
the brown-paper-bag patch with the superfluous while loop to
Linus ...        (please raise your hand and/or buy rmk a beer)

Linus, could you please apply this patch ASAP? :)

regards,

Rik  (PS. I'm still planning to implement the VM changes I posted
to linux-mm earlier today, kswapd could be better and more efficient)
--
The Internet is not a network of computers. It is a network
of people. That is its real strength.

Wanna talk about the kernel?  irc.openprojects.net / #kernelnewbies
http://www.conectiva.com/		http://www.surriel.com/



--- linux-2.3.99-pre3/mm/vmscan.c.orig	Sat Mar 25 12:57:20 2000
+++ linux-2.3.99-pre3/mm/vmscan.c	Sun Mar 26 21:37:19 2000
@@ -499,19 +499,19 @@
 		 * the processes needing more memory will wake us
 		 * up on a more timely basis.
 		 */
-		do {
-			pgdat = pgdat_list;
-			while (pgdat) {
-				for (i = 0; i < MAX_NR_ZONES; i++) {
-					zone = pgdat->node_zones + i;
-					if ((!zone->size) || (!zone->zone_wake_kswapd))
-						continue;
-					do_try_to_free_pages(GFP_KSWAPD, zone);
-				}
-				pgdat = pgdat->node_next;
+		pgdat = pgdat_list;
+		while (pgdat) {
+			for (i = 0; i < MAX_NR_ZONES; i++) {
+				zone = pgdat->node_zones + i;
+				if (tsk->need_resched)
+					schedule();
+				if ((!zone->size) || (!zone->zone_wake_kswapd))
+					continue;
+				do_try_to_free_pages(GFP_KSWAPD, zone);
 			}
-			run_task_queue(&tq_disk);
-		} while (!tsk->need_resched);
+			pgdat = pgdat->node_next;
+		}
+		run_task_queue(&tq_disk);
 		tsk->state = TASK_INTERRUPTIBLE;
 		interruptible_sleep_on(&kswapd_wait);
 	}

--
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: [PATCH] Re: kswapd

[Kanoj Sarcar]

> 
> On Sun, 26 Mar 2000, Russell King wrote:
> 
> > I think I've solved (very dirtily) my kswapd problem
> 
> Your patch is the correct one. I've added an extra reschedule
> point and cleaned up the code a little bit. I wonder who sent
> the brown-paper-bag patch with the superfluous while loop to
> Linus ...        (please raise your hand and/or buy rmk a beer)

That would be me ...

What is the problem that your patch is fixing? Other than the 
while loop cosmetic changes, the only thing I see is to do 
with rescheduling (ie, do not invoke do_try_to_free_pages or
run_task_queue() if need_resched is set). Note that before this 
patch went in, as in 2.3.43 for example, the loop used to be:

                do {
                        /* kswapd is critical to provide GFP_ATOMIC
                           allocations (not GFP_HIGHMEM ones). */
                        if (nr_free_pages() - nr_free_highpages() >= freepages.high)
                                break;
                        if (!do_try_to_free_pages(GFP_KSWAPD, 0))
                                break;
                        run_task_queue(&tq_disk);
                } while (!tsk->need_resched);
                tsk->state = TASK_INTERRUPTIBLE;
                interruptible_sleep_on(&kswapd_wait);
        }

Which is the behavior I _tried_ to preserve. Oh, btw, I am not convinced
that you should do a run_task_queue after _all_ pgdats have been scanned,
rather than after each one.

Kanoj

> 
> Linus, could you please apply this patch ASAP? :)
> 
> regards,
> 
> Rik  (PS. I'm still planning to implement the VM changes I posted
> to linux-mm earlier today, kswapd could be better and more efficient)
> --
> The Internet is not a network of computers. It is a network
> of people. That is its real strength.
> 
> Wanna talk about the kernel?  irc.openprojects.net / #kernelnewbies
> http://www.conectiva.com/		http://www.surriel.com/
> 
> 
> 
> --- linux-2.3.99-pre3/mm/vmscan.c.orig	Sat Mar 25 12:57:20 2000
> +++ linux-2.3.99-pre3/mm/vmscan.c	Sun Mar 26 21:37:19 2000
> @@ -499,19 +499,19 @@
>  		 * the processes needing more memory will wake us
>  		 * up on a more timely basis.
>  		 */
> -		do {
> -			pgdat = pgdat_list;
> -			while (pgdat) {
> -				for (i = 0; i < MAX_NR_ZONES; i++) {
> -					zone = pgdat->node_zones + i;
> -					if ((!zone->size) || (!zone->zone_wake_kswapd))
> -						continue;
> -					do_try_to_free_pages(GFP_KSWAPD, zone);
> -				}
> -				pgdat = pgdat->node_next;
> +		pgdat = pgdat_list;
> +		while (pgdat) {
> +			for (i = 0; i < MAX_NR_ZONES; i++) {
> +				zone = pgdat->node_zones + i;
> +				if (tsk->need_resched)
> +					schedule();
> +				if ((!zone->size) || (!zone->zone_wake_kswapd))
> +					continue;
> +				do_try_to_free_pages(GFP_KSWAPD, zone);
>  			}
> -			run_task_queue(&tq_disk);
> -		} while (!tsk->need_resched);
> +			pgdat = pgdat->node_next;
> +		}
> +		run_task_queue(&tq_disk);
>  		tsk->state = TASK_INTERRUPTIBLE;
>  		interruptible_sleep_on(&kswapd_wait);
>  	}
> 
> --
> 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/
> 

--
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: [PATCH] Re: kswapd

[Rik van Riel]

On Sun, 26 Mar 2000, Kanoj Sarcar wrote:
> > On Sun, 26 Mar 2000, Russell King wrote:
> > 
> > > I think I've solved (very dirtily) my kswapd problem
> > 
> > Your patch is the correct one. I've added an extra reschedule
> > point and cleaned up the code a little bit. I wonder who sent
> > the brown-paper-bag patch with the superfluous while loop to
> > Linus ...        (please raise your hand and/or buy rmk a beer)
> 
> That would be me ...
> 
> What is the problem that your patch is fixing?

Removing the superfluous while loop.

Without my patch kswapd uses between 50 and 70% CPU time
in a particular workload. Now it uses between 3 and 5%.
Oh, and the latency problem probably has been fixed too...

cheers,

Rik
--
The Internet is not a network of computers. It is a network
of people. That is its real strength.

Wanna talk about the kernel?  irc.openprojects.net / #kernelnewbies
http://www.conectiva.com/		http://www.surriel.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: [PATCH] Re: kswapd

[Kanoj Sarcar]

> 
> On Sun, 26 Mar 2000, Kanoj Sarcar wrote:
> > > On Sun, 26 Mar 2000, Russell King wrote:
> > > 
> > > > I think I've solved (very dirtily) my kswapd problem
> > > 
> > > Your patch is the correct one. I've added an extra reschedule
> > > point and cleaned up the code a little bit. I wonder who sent
> > > the brown-paper-bag patch with the superfluous while loop to
> > > Linus ...        (please raise your hand and/or buy rmk a beer)
> > 
> > That would be me ...
> > 
> > What is the problem that your patch is fixing?
> 
> Removing the superfluous while loop.
> 

Maybe I am being stupid, but where is the superfluous loop? Remember,
as I just pointed out, 2.3.43 had the same loop, are you claiming it 
was buggy even then?

> Without my patch kswapd uses between 50 and 70% CPU time
> in a particular workload. Now it uses between 3 and 5%.

Can you explain how this is happening? I can see that in your patch,
kswapd does not go thru the loop if need_resched is set, but with
a single node, 3 zones, I would find it hard to explain such a 
difference.

> Oh, and the latency problem probably has been fixed too...

What latency problem? I still believe that the pre3 code is doing
the right thing, assuming 2.3.43 was doing the right thing.

Kanoj

> 
> cheers,
> 
> Rik
> --
> The Internet is not a network of computers. It is a network
> of people. That is its real strength.
> 
> Wanna talk about the kernel?  irc.openprojects.net / #kernelnewbies
> http://www.conectiva.com/		http://www.surriel.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: [PATCH] Re: kswapd

[Eric W. Biederman]

kanoj@google.engr.sgi.com (Kanoj Sarcar) writes:

> What is the problem that your patch is fixing? 

> 
>                 do {
>                         /* kswapd is critical to provide GFP_ATOMIC
>                            allocations (not GFP_HIGHMEM ones). */
>                         if (nr_free_pages() - nr_free_highpages() >=
> freepages.high)
> 
>                                 break;
>                         if (!do_try_to_free_pages(GFP_KSWAPD, 0))
>                                 break;
>                         run_task_queue(&tq_disk);
>                 } while (!tsk->need_resched);
>                 tsk->state = TASK_INTERRUPTIBLE;
>                 interruptible_sleep_on(&kswapd_wait);
>         }

Hmm.  This loop runs until either 
(a) it has free pages or
(b) it has used up it's time slice.

> > --- linux-2.3.99-pre3/mm/vmscan.c.orig	Sat Mar 25 12:57:20 2000
> > +++ linux-2.3.99-pre3/mm/vmscan.c	Sun Mar 26 21:37:19 2000
> > @@ -499,19 +499,19 @@
> >  		 * the processes needing more memory will wake us
> >  		 * up on a more timely basis.
> >  		 */
> > -		do {
> > -			pgdat = pgdat_list;
> > -			while (pgdat) {
> > -				for (i = 0; i < MAX_NR_ZONES; i++) {
> > -					zone = pgdat->node_zones + i;
> > - if ((!zone->size) || (!zone->zone_wake_kswapd))
> 
> > -						continue;
> > -					do_try_to_free_pages(GFP_KSWAPD, zone);
> > -				}
> > -				pgdat = pgdat->node_next;
> > +		pgdat = pgdat_list;
> > +		while (pgdat) {
> > +			for (i = 0; i < MAX_NR_ZONES; i++) {
> > +				zone = pgdat->node_zones + i;
> > +				if (tsk->need_resched)
> > +					schedule();
> > +				if ((!zone->size) || (!zone->zone_wake_kswapd))
> > +					continue;
> > +				do_try_to_free_pages(GFP_KSWAPD, zone);
> >  			}
> > -			run_task_queue(&tq_disk);
> > -		} while (!tsk->need_resched);
> > +			pgdat = pgdat->node_next;
> > +		}
> > +		run_task_queue(&tq_disk);
> >  		tsk->state = TASK_INTERRUPTIBLE;
> >  		interruptible_sleep_on(&kswapd_wait);
> >  	}

The removed loop runs until a reschedule is needed.
Having enough memory isn't sufficient to get out of the loop.
So it can spin in run_task_queue(&tq_disk);

The added loop runs only while (pgdat).
Running out of time slice will put it to sleep now.

Eric
--
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: [PATCH] Re: kswapd

[Linus Torvalds]

On Sun, 26 Mar 2000, Kanoj Sarcar wrote:
> 
> What is the problem that your patch is fixing?

I agree with Rik's patch - the old behaviour kicked us out of the regular
loop whenever "need_resched" was set, and that is not necessarily a good
idea at all.

>From a conceptual standpoint, going to sleep when "need_resched" gets set
is not the right thing at all - the flag doesn't really have any bearing
on whether kswapd should sleep, it only has meaning from a scheduling
latency standpoint (ie "need_resched" does not mean "go to sleep", it
means "let somebody else run now" - different things).

On the other hand you're definitely right that this is not a new bug
introduced by you, Kanoj - this seems to be just a thinko that has been
there for a long long time. And I suspect I may have been the original
perpetrator of the crime.

The new code looks much saner: it reschedules when asked to, and it stops
looping when it makes sense (ie when there is no longer any reason to free
pages). Instead of mixing the two up.

		Linus

--
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: [PATCH] Re: kswapd

[Russell King]

Kanoj Sarcar writes:
> > Without my patch kswapd uses between 50 and 70% CPU time
> > in a particular workload. Now it uses between 3 and 5%.
> 
> Can you explain how this is happening? I can see that in your patch,
> kswapd does not go thru the loop if need_resched is set, but with
> a single node, 3 zones, I would find it hard to explain such a 
> difference.

kswapd was running until it had a quantum, whether or not it had to
free any more pages.

> > Oh, and the latency problem probably has been fixed too...
> 
> What latency problem? I still believe that the pre3 code is doing
> the right thing, assuming 2.3.43 was doing the right thing.

Check out the linux-kernel archives, specifically for my mails about
NFS/sound problems - it is not good for processes (eg, rpciod) to be
starved of CPU time up to 200-400ms.  It was basically impossible to
play mp3s without the playback basically "stopping" for that period.

(without the patch in, I see kswapd using 200ms-400ms.  With the patch
in, it only uses >10ms very very rarely).
   _____
  |_____| ------------------------------------------------- ---+---+-
  |   |         Russell King        rmk@arm.linux.org.uk      --- ---
  | | | |   http://www.arm.linux.org.uk/~rmk/aboutme.html    /  /  |
  | +-+-+                                                     --- -+-
  /   |               THE developer of ARM Linux              |+| /|\
 /  | | |                                                     ---  |
    +-+-+ -------------------------------------------------  /\\\  |
--
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: [PATCH] Re: kswapd

[Kanoj Sarcar]

> 
> On the other hand you're definitely right that this is not a new bug
> introduced by you, Kanoj - this seems to be just a thinko that has been

Whew, as long as I can keep that beer I was going to send to Russell on
Rik's suggestion for myself! :-)

On a more serious note, I know too little about the application load 
that Rik/Russell is talking about to understand what's going on, but
I have the vague suspicion that Rik's patch is just a part fix to the 
problem, and that maybe we might be doing too many kswapd wakes ups
via the balancing code. 

This is my reasoning: Rik's patch makes it so that before kswapd 
undertakes heavy weight work, it yields the cpu ... then it checks
whether it has to do the work (via zone_wake_kswapd). This is the
only difference over pre3. If this is improving things a lot, that
makes me believe that the memory-low condition is subsiding (pages
are being freed up) just after kswapd has yielded, and before it
gets scheduled onto the cpu again. This depends on the app and its
priority too, I guess. If there is an app load where the pages are
not freed, Rik's patch would _probably_ not be able to help. Its 
better than nothing, but if you wanted to do the best you could, you
need to add more yield points into the body of kswapd code (which 
I suspect will not be free of side effects possibly). 

The other part about possibly doing too many kswapd wakeups is 
just a hunch, not directly related to this patch. I will forward
a balancing patch on the mailing lists next for people to try.

Kanoj
--
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: [PATCH] Re: kswapd

[Rik van Riel]

On Mon, 27 Mar 2000, Kanoj Sarcar wrote:

> This is my reasoning: Rik's patch makes it so that before kswapd 
> undertakes heavy weight work, it yields the cpu ... then it checks
> whether it has to do the work (via zone_wake_kswapd). This is the
> only difference over pre3. If this is improving things a lot, that
> makes me believe that the memory-low condition is subsiding (pages
> are being freed up) just after kswapd has yielded, and before it
> gets scheduled onto the cpu again.

This is not the case. CPU system time has decreased from 70%
to 7% with the patch in. Kswapd uses about 5 of those 7%, so
it is true that some of the work is done by the applications,
but overall system time has decreased a lot.

> This depends on the app and its priority too, I guess. If there
> is an app load where the pages are not freed, Rik's patch would
> _probably_ not be able to help.

In this case, nothing would help (except having your low-latency
app completely free of memory allocations).

> Its better than nothing, but if you wanted to do the best you
> could, you need to add more yield points into the body of kswapd
> code (which I suspect will not be free of side effects
> possibly).

One resched every do_try_to_free_pages() seems to be enough
for most things, but it should be relatively easy to build
some reschedule points into do_try_to_free_pages() (which
can _only_ be called if current->state == TASK_RUNNING!!).

> The other part about possibly doing too many kswapd wakeups is 
> just a hunch, not directly related to this patch. I will forward
> a balancing patch on the mailing lists next for people to try.

Probably some hysteresis problem. Using all three freepages
thresholds (min, low and high) solved the problem for zoneless
MM, so it should probably work for a zoned VM too.

regards,

Rik
--
The Internet is not a network of computers. It is a network
of people. That is its real strength.

Wanna talk about the kernel?  irc.openprojects.net / #kernelnewbies
http://www.conectiva.com/		http://www.surriel.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: [PATCH] Re: kswapd

[Linus Torvalds]

On Mon, 27 Mar 2000, Kanoj Sarcar wrote:
> 
> This is my reasoning: Rik's patch makes it so that before kswapd 
> undertakes heavy weight work, it yields the cpu ... then it checks
> whether it has to do the work (via zone_wake_kswapd). This is the
> only difference over pre3.

No, there's another difference: pre3 will loop forever, even if there is
nothing to do - until something comes up that needs scheduling. Basically,
the pre3 loop boils down to

	do {
		/* not interesting */
	} while (!tsk->need_resched);

when there is enough memory.

Which obviously causes excessive CPU to be wasted.

NOTE! The "obviously" is a bit strong. What happens is that kswapd is only
woken up when needed, so most of the time it is sleeping. It's only when
it is woken up and when it has done its work when the loop turns into a
CPU-burner, but it can easily mean that kswapd will just spend CPU time
for no good reason until its time-slice is exhausted.

So think of the bug as "kswapd will waste the final part of its timeslice
doing nothing useful".

		Linus

--
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: [PATCH] Re: kswapd

[Mark Hahn]

> So think of the bug as "kswapd will waste the final part of its timeslice
> doing nothing useful".

yes!  should it not look at the return from try_to_free_pages 
to find out whether further looping is needed?  or something based
on the current free pages level, hopefully with hysteresis like 
Rik mentioned?

regards, mark hahn.

--
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: [PATCH] Re: kswapd

[Linus Torvalds]

On Mon, 27 Mar 2000, Mark Hahn wrote:
> 
> yes!  should it not look at the return from try_to_free_pages 
> to find out whether further looping is needed?  or something based
> on the current free pages level, hopefully with hysteresis like 
> Rik mentioned?

It does do that - that's what the "kswapd_wake_up" flag does for the loop.
The problem was that the loop termination logic was a bit topsy-turvy.

		Linus

--
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: [PATCH] Re: kswapd

[Rik van Riel]

On Mon, 27 Mar 2000, Mark Hahn wrote:

> > So think of the bug as "kswapd will waste the final part of its timeslice
> > doing nothing useful".
> 
> yes!  should it not look at the return from try_to_free_pages 
> to find out whether further looping is needed? 
> or something based on the current free pages level, hopefully
> with hysteresis like Rik mentioned?

It is looking at the current free page levels, on a zone-by-zone
basis. Looking at the return value of try_to_free_pages() doesn't
make much sense IMHO because that just means that normal processes
will be doing the heavy work instead of kswapd (leading to poor
interactive response and other trouble).

regards,

Rik
--
The Internet is not a network of computers. It is a network
of people. That is its real strength.

Wanna talk about the kernel?  irc.openprojects.net / #kernelnewbies
http://www.conectiva.com/		http://www.surriel.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: [PATCH] Re: kswapd

[Russell King]

Kanoj Sarcar writes:
> On a more serious note, I know too little about the application load 
> that Rik/Russell is talking about to understand what's going on, but
> I have the vague suspicion that Rik's patch is just a part fix to the 
> problem, and that maybe we might be doing too many kswapd wakes ups
> via the balancing code. 

My situation is very simple - one copy of xaudio playing mp3s.  Nothing
else.  No swapping.  No nothing.  Just one xaudio (and the occasional
cron and atd running) playing mp3s from a NFS server.  Load average < 1.
   _____
  |_____| ------------------------------------------------- ---+---+-
  |   |         Russell King        rmk@arm.linux.org.uk      --- ---
  | | | |   http://www.arm.linux.org.uk/~rmk/aboutme.html    /  /  |
  | +-+-+                                                     --- -+-
  /   |               THE developer of ARM Linux              |+| /|\
 /  | | |                                                     ---  |
    +-+-+ -------------------------------------------------  /\\\  |
--
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: [PATCH] Re: kswapd

[Kanoj Sarcar]

> 
> So think of the bug as "kswapd will waste the final part of its timeslice
> doing nothing useful".
>

Got it ... Thanks.

Makes you wonder whether the resched checking should be done for each zone, 
or once for the entire pgdat.

Kanoj
--
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/