On Saturday 11 March 2006 09:35, Andrew Morton wrote:
> Con Kolivas <[email protected]> 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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
