* Re: Linux-2.4.1-pre11
2001-01-28 19:32 ` Linux-2.4.1-pre11 Linus Torvalds
@ 2001-01-29 4:29 ` Ed Tomlinson
2001-01-29 19:30 ` Linux-2.4.1-pre11 Rik van Riel
1 sibling, 0 replies; 5+ messages in thread
From: Ed Tomlinson @ 2001-01-29 4:29 UTC (permalink / raw)
To: Linus Torvalds, Marcelo Tosatti; +Cc: linux-mm
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/
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Linux-2.4.1-pre11
2001-01-28 19:32 ` Linux-2.4.1-pre11 Linus Torvalds
2001-01-29 4:29 ` Linux-2.4.1-pre11 Ed Tomlinson
@ 2001-01-29 19:30 ` Rik van Riel
2001-01-29 18:24 ` Linux-2.4.1-pre11 Marcelo Tosatti
1 sibling, 1 reply; 5+ messages in thread
From: Rik van Riel @ 2001-01-29 19:30 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Marcelo Tosatti, linux-mm
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/
^ permalink raw reply [flat|nested] 5+ messages in thread