svn commit: r218195 - in head/sys: amd64/amd64 arm/arm i386/i386 ia64/ia64 kern mips/mips powerpc/powerpc sparc64/sparc64 sun4v/sun4v sys ufs/ffs

[John Baldwin]

On Thursday, February 03, 2011 2:47:20 am Juli Mallett wrote:
> On Wed, Feb 2, 2011 at 08:35, Matthew D Fleming <mdf at freebsd.org> wrote:
> > Author: mdf
> > Date: Wed Feb  2 16:35:10 2011
> > New Revision: 218195
> > URL: http://svn.freebsd.org/changeset/base/218195
> >
> > Log:
> >  Put the general logic for being a CPU hog into a new function
> >  should_yield().  Use this in various places.  Encapsulate the common
> >  case of check-and-yield into a new function maybe_yield().
> >
> >  Change several checks for a magic number of iterations to use
> >  should_yield() instead.
> 
> First off, I admittedly don't know or care very much about this area,
> but this commit stood out to me and I had a few minor concerns.
> 
> I'm slightly uncomfortable with the flat namespace here.  It isn't
> obvious from the names that maybe_yield() and should_yield() relate
> only to uio_yield() and not other types of yielding (from DELAY() to
> cpu_idle() to sched_yield().)  The other problematic element here is
> that "maybe_yield" and "should_yield" could quite reasonably be
> variables or functions in existing code in the kernel, and although we
> don't try to protect against changes that could cause such collisions,
> we shouldn't do them gratuitously, and there's even something that
> seems aesthetically off about these; they seem...informal, even
> Linuxy.  I think names like uio_should_yield() and uio_maybe_yield()
> wouldn't have nearly as much of a problem, since the context of the
> question of "should" is isolated to uio operations rather than, say,
> whether the scheduler would *like* for us, as the running thread, to
> yield, or other considerations that may be more general.

I mostly agree, but these checks are no longer specific to uio.  Matt used 
them to replace many ad-hoc checks using counters with hardcoded maximums in 
places like softupdates, etc.

I don't have any good suggestions for what else you would call these.  I'm not 
sure 'sched_amcpuhog() or sched_hoggingcpu()' are really better (and these are 
not scheduler dependent, so sched_ would probably not be a good prefix).

-- 
John Baldwin