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:

quoted

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/