Re: Linux-2.4.1-pre11

[Ed Tomlinson <tomlins@cam.org>]

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/