On Thu, Feb 3, 2011 at 4:50 AM, John Baldwin <j...@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 <m...@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 _______________________________________________ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"