Re: [PATCH] mm: Implement swap prefetching tweaks

[Con Kolivas <kernel@kolivas.org>]

On Saturday 11 March 2006 09:35, Andrew Morton wrote:
> Con Kolivas <kernel@kolivas.org> wrote:
> > +	/*
> > +	 * get_page_state is super expensive so we only perform it every
> > +	 * SWAP_CLUSTER_MAX prefetched_pages.
>
> nr_running() is similarly expensive btw.

Yes which is why I do it just as infrequently as get_page_state.
>
> > 	 * We also test if we're the only
> > +	 * task running anywhere. We want to have as little impact on all
> > +	 * resources (cpu, disk, bus etc). As this iterates over every cpu
> > +	 * we measure this infrequently.
> > +	 */
> > +	if (!(sp_stat.prefetched_pages % SWAP_CLUSTER_MAX)) {
> > +		unsigned long cpuload = nr_running();
> > +
> > +		if (cpuload > 1)
> > +			goto out;
>
> Sorry, this is just wrong.  If swap prefetch is useful then it's also
> useful if some task happens to be sitting over in the corner calculating
> pi.
>
> What's the actual problem here?  Someone's 3d game went blippy?  Why?  How
> much?  Are we missing a cond_resched()?

No, it's pretty easy to reproduce, kprefetchd sits there in uninterruptible 
sleep with one cpu on SMP pegged at 100% iowait due to it. This tends to have 
noticeable effects everywhere on HT or SMP. On UP the yielding helped it but 
even then it still causes blips. How much? Well to be honest it's noticeable 
a shipload. Running a game, any game, that uses 100% (and most fancy games 
do) causes stuttering on audio, pauses and so on. This is evident on linux 
native games, games under emulators or qemu and so on. That iowait really 
hurts, and tweaking just priority doesn't help it in any way.

With this change it's much more polite and takes a bit longer to complete 
prefetching but is still effective while no longer being noticeable.

> > +		cpuload += nr_uninterruptible();
> > +		if (cpuload > 1)
> > +			goto out;
>
> Not sure about this either.

Same as above. It's the tasks in uninterruptible sleep that cause the most 
harm. I do it sequentially simply because nr_running() is more likely to be 
>1 than the sum total, and I'd prefer not to do nr_uninterruptible() unless 
it's necessary. Both of these are actually done lockless though.

> > +		if (ns->last_free) {
> > +			if (ns->current_free + SWAP_CLUSTER_MAX <
> > +				ns->last_free) {
> > +					ns->last_free = ns->current_free;
> >  					node_clear(node,
> >  						sp_stat.prefetch_nodes);
> >  					continue;
> >  			}
> >  		} else
>
> That has an extra tabstop.

Hrm. 3 years on and I still make basi style mistakes.

Cheers,
Con