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

[mdf at FreeBSD.org]

On Thu, Feb 3, 2011 at 4:50 AM, John Baldwin <jhb at freebsd.org> wrote:
> 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).

Bruce correctly points out that the code doesn't work like I expect
with PREEMPTION, which most people will be running.

I'm thinking of adding a new per-thread field to record the last ticks
value that a voluntary mi_switch() was done, so that there's a
standard way of checking if a thread is being a hog; this will work
for both PREEMPTION and !PREEMPTION, and would be appropriate for the
places that previously used a counter.  (This would require
uio_yield() to be SW_VOL, but I can't see why it's not a voluntary
context switch anyways).

I'm happy to rename the functions (perhaps just yield_foo() rather
than foo_yield()?) and stop using uio_yield as the base name since
it's not a uio function.  I wanted to keep the uio_yield symbol to
preserve the KBI/KPI.

Any suggestions for names are welcome.

Thanks,
matthew