Re: Linux-2.4.1-pre11

Re: Linux-2.4.1-pre11

[Linus Torvalds]

[ Cc'd to linux-mm, because I'm trying to also explain what kind of code I
  like, and because the more people who understand how I judge the
  "goodness" of code, the happier I hope I will be. ]

On Sun, 28 Jan 2001, Marcelo Tosatti wrote:
> 
> Why you have not applied the conditional background page aging patch?

Because it's only performance, not stability, and every time I look at the
patch it looks ugly to me. Let me explain.

Quite frankly, I suspect I'd like the thing much more if it didn't have a
"background" flag, but a more specific thing. I hate overloading functions
with two things and then having flags that change the behaviour of them. I
like arguments that say "do THIS". I do not like arguments that say "I'm
THIS, do whatever you want that you think will satisfy me".

For example, I don't like how the current one has the "oneshot" parameter.
I'd much rather just have

	deactivated = refill_inactive_scan(count);

where the "count" would be the thing we ask for, and "deactivated" would
obviously be how many we got. No "background" or "oneshot". The "oneshot"
stuff in particular makes the

	while (refill_inactive_scan(DEF_PRIORITY, one)) ...

logic look completely buggered: first we ask for it to exit after having
found _one_, and then we have a loop that does this "count" times. Where's
the logic?

But at the same time, the "oneshot" parameter _does_ fulfill my need for
well-defined behaviour. It says "DO THIS!". Or at least it tries to. It
doesn't say "I like the color blue, so try to take that into account when
you do whatever you like to do".

Your "background" paramater makes the whole parameter pretty fluffy, in my
opinion.. Why do we exit early when '!background'? Where's the
_philosophy_ of the function? What do the arguments really mean? You can't
read the callers and understand what the callers really are trying to do..

Now, to me the "background" check should be in the caller. Th ecaller
knows that it's doing background activity, so the _caller_ should be the
one that say "when in the background, DO THIS!". Instead of letting the
function try to decide on its own what it is we want when we're in the
background.

And conversely the "how many pages should be try to de-activate" logic
should be in "refill_inactive_scan". So to me, the following calling
convetion would make some amount of sense:

 - refill_inactive_scan() calling convention change: it should be

	int refill_inactive_scan(int nr_to_deactivate);

   and basically return the number of pages it de-activated, with the
   parameter being how many pages we _want_ to de-activate. We've already
   stopped using the priority (it's always DEF_PROPROTY), so we can drop
   that. And I'd like the other parameter to _mean_ something, if you see
   what I'm saying.

   "Try to scan for THIS many" is a meaning. "Try to scan in the
   background" doesn't really mean anything. What does "background" mean
   to the scanning logic? It means something to the caller, but not to the
   scanner.

 - kswapd, before it calls refill_inactive_scan(), would basically do
   something like

	nr = free_shortage() + inactive_shortage();
	if (nr > MAXSCAN)
		nr = MAXSCAN;
	if (nr || bg_page_aging)
		refill_inactive_scan(nr);

   which again has some _meaning_. You can point to it and say: we want to
   de-activate "nr" pages because we're short on memory. But even if the
   answer is "zero", we may want to do some aging in the background, so
   "refill_inactive_scan()" can know that a zero argument means that we
   don't _really_ want to deactivate anything.

   See how you can _read_ the calling code directly? Show the above five
   lines to a programmer who hasn't even seen what it is that
   "refill_inactive_scan()" actually does, and I bet he can guess what
   we're trying to do.

   That's what I mean with _meaning_. Which the current code lacks, and
   which your change makes even less of.

   And note how "refill_inactive_scan(0)" is automatically a logical
   special case. It tells refill_inactive_scan() that we don't actually
   want to _really_ deactivate anything, so we're obviously just doing the
   aging. 

 - refill_inactive() can do

	count -= refill_inactive_scan(count);

   instead of the current while-loop. Again, you can pretty much see from
   that one line what it tries to do.

Now, I'm not saying the above is how it must be done. The above is meant
more as an example of an interface and a logic that I can follow, and that
I think is more appropriate for programming. Programming is never about
asking the computer to do something and telling it what the constraints
are (a constraint would be "do this, but remember that we're a background
process"). Programming is about telling the computer what to do. You
should NOT say:

 "please scan some pages in the ackground"

but instead say

 "scan and age the active list, and deactive up to 5 pages"

You're the captain. Don't say "Please". Say "Make it so".

		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: Linux-2.4.1-pre11

[Ed Tomlinson]

Feedback is nice...

The way you would like refill_inactive defined raises a question or two.  The
patches now have it runing in two modes, which you object to.  The way I read
what you say the real way to fix this would be to refactor refill_inactive.

result = refill_inactive(count) 

which has one goal of getting count pages inactivated.  There should also be 
a function to do the bg_aging, something like.

result = balance_activates(background)

The goal of which is to ensure that for every page we activate we also age a 
page down.  This might move some pages to the inactive list and might trigger 
some swapouts.  

This could also be hidden in refill_inactive triggered by a count of zero but 
this implies that refill_inactive must have a side effect of decrementing the 
background counter which is not the nicest thing to do.

Comments?
Ed Tomlinson


On January 28, 2001 02:32 pm, Linus Torvalds wrote:
> [ Cc'd to linux-mm, because I'm trying to also explain what kind of code I
>   like, and because the more people who understand how I judge the
>   "goodness" of code, the happier I hope I will be. ]
>
> On Sun, 28 Jan 2001, Marcelo Tosatti wrote:
> > Why you have not applied the conditional background page aging patch?
>
> Because it's only performance, not stability, and every time I look at the
> patch it looks ugly to me. Let me explain.
>
> Quite frankly, I suspect I'd like the thing much more if it didn't have a
> "background" flag, but a more specific thing. I hate overloading functions
> with two things and then having flags that change the behaviour of them. I
> like arguments that say "do THIS". I do not like arguments that say "I'm
> THIS, do whatever you want that you think will satisfy me".
>
> For example, I don't like how the current one has the "oneshot" parameter.
> I'd much rather just have
>
> 	deactivated = refill_inactive_scan(count);
>
> where the "count" would be the thing we ask for, and "deactivated" would
> obviously be how many we got. No "background" or "oneshot". The "oneshot"
> stuff in particular makes the
>
> 	while (refill_inactive_scan(DEF_PRIORITY, one)) ...
>
> logic look completely buggered: first we ask for it to exit after having
> found _one_, and then we have a loop that does this "count" times. Where's
> the logic?
>
> But at the same time, the "oneshot" parameter _does_ fulfill my need for
> well-defined behaviour. It says "DO THIS!". Or at least it tries to. It
> doesn't say "I like the color blue, so try to take that into account when
> you do whatever you like to do".
>
> Your "background" paramater makes the whole parameter pretty fluffy, in my
> opinion.. Why do we exit early when '!background'? Where's the
> _philosophy_ of the function? What do the arguments really mean? You can't
> read the callers and understand what the callers really are trying to do..
>
> Now, to me the "background" check should be in the caller. Th ecaller
> knows that it's doing background activity, so the _caller_ should be the
> one that say "when in the background, DO THIS!". Instead of letting the
> function try to decide on its own what it is we want when we're in the
> background.
>
> And conversely the "how many pages should be try to de-activate" logic
> should be in "refill_inactive_scan". So to me, the following calling
> convetion would make some amount of sense:
>
>  - refill_inactive_scan() calling convention change: it should be
>
> 	int refill_inactive_scan(int nr_to_deactivate);
>
>    and basically return the number of pages it de-activated, with the
>    parameter being how many pages we _want_ to de-activate. We've already
>    stopped using the priority (it's always DEF_PROPROTY), so we can drop
>    that. And I'd like the other parameter to _mean_ something, if you see
>    what I'm saying.
>
>    "Try to scan for THIS many" is a meaning. "Try to scan in the
>    background" doesn't really mean anything. What does "background" mean
>    to the scanning logic? It means something to the caller, but not to the
>    scanner.
>
>  - kswapd, before it calls refill_inactive_scan(), would basically do
>    something like
>
> 	nr = free_shortage() + inactive_shortage();
> 	if (nr > MAXSCAN)
> 		nr = MAXSCAN;
> 	if (nr || bg_page_aging)
> 		refill_inactive_scan(nr);
>
>    which again has some _meaning_. You can point to it and say: we want to
>    de-activate "nr" pages because we're short on memory. But even if the
>    answer is "zero", we may want to do some aging in the background, so
>    "refill_inactive_scan()" can know that a zero argument means that we
>    don't _really_ want to deactivate anything.
>
>    See how you can _read_ the calling code directly? Show the above five
>    lines to a programmer who hasn't even seen what it is that
>    "refill_inactive_scan()" actually does, and I bet he can guess what
>    we're trying to do.
>
>    That's what I mean with _meaning_. Which the current code lacks, and
>    which your change makes even less of.
>
>    And note how "refill_inactive_scan(0)" is automatically a logical
>    special case. It tells refill_inactive_scan() that we don't actually
>    want to _really_ deactivate anything, so we're obviously just doing the
>    aging.
>
>  - refill_inactive() can do
>
> 	count -= refill_inactive_scan(count);
>
>    instead of the current while-loop. Again, you can pretty much see from
>    that one line what it tries to do.
>
> Now, I'm not saying the above is how it must be done. The above is meant
> more as an example of an interface and a logic that I can follow, and that
> I think is more appropriate for programming. Programming is never about
> asking the computer to do something and telling it what the constraints
> are (a constraint would be "do this, but remember that we're a background
> process"). Programming is about telling the computer what to do. You
> should NOT say:
>
>  "please scan some pages in the ackground"
>
> but instead say
>
>  "scan and age the active list, and deactive up to 5 pages"
>
> You're the captain. Don't say "Please". Say "Make it so".
>
> 		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/
--
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: Linux-2.4.1-pre11

[Rik van Riel]

On Sun, 28 Jan 2001, Linus Torvalds wrote:

> Quite frankly, I suspect I'd like the thing much more if it
> didn't have a "background" flag, but a more specific thing.

> For example, I don't like how the current one has the "oneshot" parameter.
> I'd much rather just have
> 
> 	deactivated = refill_inactive_scan(count);

I have this buried somewhere in the 3 weeks of code I wrote
while at linux.conf.au. I'll dig up some parts of my code and
I'll give it to you ASAP.

>  - refill_inactive_scan() calling convention change: it should be
> 
> 	int refill_inactive_scan(int nr_to_deactivate);
> 
>    and basically return the number of pages it de-activated, with the
>    parameter being how many pages we _want_ to de-activate.

One moment please, I'll make you a patch.  ;)

>  - kswapd, before it calls refill_inactive_scan(), would basically do
>    something like
> 
> 	nr = free_shortage() + inactive_shortage();
> 	if (nr > MAXSCAN)
> 		nr = MAXSCAN;
> 	if (nr || bg_page_aging)
> 		refill_inactive_scan(nr);
> 
>    which again has some _meaning_. You can point to it and say: we want to
>    de-activate "nr" pages because we're short on memory. But even if the
>    answer is "zero", we may want to do some aging in the background, so
>    "refill_inactive_scan()" can know that a zero argument means that we
>    don't _really_ want to deactivate anything.

Wouldn't that be:
	if (nr < MINSCAN)
		nr = MINSCAN;

Btw, if we fix the inactive_shortage() function to take
per-zone inactive shortage into account, we can just skip
scanning if we only have a free shortage but no inactive
shortage.

Page_launder() then needs to be changed a bit too. If it's
called from a user process, it can have the same behaviour
it has now.

OTOH, when it's called from kswapd, kswapd should just flush
as many pages as needed to make the free shortage go away.

Suppose you have a system with 500 kB free shortage. In the
current setup, kswapd will flush MAX_LAUNDER pages and go to
sleep. An alternative would be to let kswapd not go to sleep
when we have a free shortage, but the result of this would
be that kswapd will keep submitting IO until the first 500 kB
is flushed (also wrong).

Only when kswapd submits 500 kB of dirty pages for IO in the
launder loop, everything will work out (didn't submit too much
IO, get the amount of free pages we need).

>  - refill_inactive() can do
> 
> 	count -= refill_inactive_scan(count);

Have you been reading the (partly messy) big snapshot patch
I posted on january 15th? ;)


cheers,

Rik
--
Virtual memory is like a game you can't win;
However, without VM there's truly nothing to lose...

		http://www.surriel.com/
http://www.conectiva.com/	http://distro.conectiva.com.br/

--
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: Linux-2.4.1-pre11

[Marcelo Tosatti]

On Mon, 29 Jan 2001, Rik van Riel wrote:

> Wouldn't that be:
> 	if (nr < MINSCAN)
> 		nr = MINSCAN;
> 
> Btw, if we fix the inactive_shortage() function to take
> per-zone inactive shortage into account, we can just skip
> scanning if we only have a free shortage but no inactive
> shortage.

inactive_shortage() already takes this into account. 

> Page_launder() then needs to be changed a bit too. If it's
> called from a user process, it can have the same behaviour
> it has now.

Btw, look at this part of code from kswapd: 

                /* 
                 * We go to sleep if either the free page shortage
                 * or the inactive page shortage is gone. We do this
                 * because:
                 * 1) we need no more free pages   or
                 * 2) the inactive pages need to be flushed to disk,
                 *    it wouldn't help to eat CPU time now ...
                 *
                 * We go to sleep for one second, but if it's needed
                 * we'll be woken up earlier...
                 */
                if (!free_shortage() || !inactive_shortage()) {
                        interruptible_sleep_on_timeout(&kswapd_wait, HZ);


kswapd goes to sleep if there is no free shortage, even if the inactive
list is under shortage.

Why not refill the inactive list when the inactive list is under
shortage? :)


--
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: Linux-2.4.1-pre11

[Rik van Riel]

On Mon, 29 Jan 2001, Marcelo Tosatti wrote:

> Btw, look at this part of code from kswapd: 
> 
>                  * 1) we need no more free pages   or
>                  * 2) the inactive pages need to be flushed to disk,
>                  *    it wouldn't help to eat CPU time now ...

>                 if (!free_shortage() || !inactive_shortage()) {
>                         interruptible_sleep_on_timeout(&kswapd_wait, HZ);
> 
> kswapd goes to sleep if there is no free shortage, even if the
> inactive list is under shortage.
> 
> Why not refill the inactive list when the inactive list is under
> shortage? :)

At this point, we already scanned the active list and we
know we're not getting any more aging information at this
point.

In this case, it might be better to leave the active pages
alone for a while and give userland a chance to use the
pages so we can get some aging information the next time we
scan the list.

regards,

Rik
--
Virtual memory is like a game you can't win;
However, without VM there's truly nothing to lose...

		http://www.surriel.com/
http://www.conectiva.com/	http://distro.conectiva.com.br/



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