Date: Fri, 8 Jun 2012 14:13:22 +0100 From: Attilio Rao <attilio@freebsd.org> To: Davide Italiano <davide@freebsd.org> Cc: svn-src-projects@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r236744 - in projects/calloutng/sys: kern sys Message-ID: <CAJ-FndC8-NfkLGLGaaOnR1_HGHyDNHxEKYNp3a2FggYtqZ%2B7nA@mail.gmail.com> In-Reply-To: <CACYV=-FD6qNAK_y532y3jQQhCh7Ct7venkEsxYmFBEiqZMPnyw@mail.gmail.com> References: <201206081153.q58BrqG2056771@svn.freebsd.org> <CAJ-FndB7QPRnmqrwyR9ixF9Jom8n7P1OMs=rczOKhmRxt08m3g@mail.gmail.com> <CACYV=-FD6qNAK_y532y3jQQhCh7Ct7venkEsxYmFBEiqZMPnyw@mail.gmail.com>
index | next in thread | previous in thread | raw e-mail
2012/6/8 Davide Italiano <davide@freebsd.org>: > On Fri, Jun 8, 2012 at 2:52 PM, Attilio Rao <attilio@freebsd.org> wrote: >> 2012/6/8 Davide Italiano <davide@freebsd.org>: >>> Author: davide >>> Date: Fri Jun 8 11:53:51 2012 >>> New Revision: 236744 >>> URL: http://svn.freebsd.org/changeset/base/236744 >>> >>> Log: >>> Add (experimentally) a function to the sleepqueue(9) KPI >>> sleepq_set_timeout_bt() in which the timeout may be specified in terms >>> of bintime rather than ticks, and which takes advantage of the new >>> precision capabilities of the callout subsystem. >>> >>> Modify the kern_nanosleep() function so that it may rely on >>> sleepq_set_timeout_bt() rather than tsleep(). >>> >>> Modified: >>> projects/calloutng/sys/kern/kern_time.c >>> projects/calloutng/sys/kern/subr_sleepqueue.c >>> projects/calloutng/sys/sys/sleepqueue.h >>> >>> Modified: projects/calloutng/sys/kern/kern_time.c >>> ============================================================================== >>> --- projects/calloutng/sys/kern/kern_time.c Fri Jun 8 11:40:30 2012 (r236743) >>> +++ projects/calloutng/sys/kern/kern_time.c Fri Jun 8 11:53:51 2012 (r236744) >>> @@ -43,6 +43,7 @@ __FBSDID("$FreeBSD$"); >>> #include <sys/resourcevar.h> >>> #include <sys/signalvar.h> >>> #include <sys/kernel.h> >>> +#include <sys/sleepqueue.h> >>> #include <sys/syscallsubr.h> >>> #include <sys/sysctl.h> >>> #include <sys/sysent.h> >>> @@ -352,37 +353,40 @@ static int nanowait; >>> int >>> kern_nanosleep(struct thread *td, struct timespec *rqt, struct timespec *rmt) >>> { >>> - struct timespec ts, ts2, ts3; >>> - struct timeval tv; >>> - int error; >>> + struct timespec ts; >>> + struct bintime bt, bt2, tmp; >>> + int catch = 0, error; >>> >>> if (rqt->tv_nsec < 0 || rqt->tv_nsec >= 1000000000) >>> return (EINVAL); >>> if (rqt->tv_sec < 0 || (rqt->tv_sec == 0 && rqt->tv_nsec == 0)) >>> return (0); >>> - getnanouptime(&ts); >>> - timespecadd(&ts, rqt); >>> - TIMESPEC_TO_TIMEVAL(&tv, rqt); >>> + binuptime(&bt); >>> + timespec2bintime(rqt, &tmp); >>> + bintime_add(&bt,&tmp); >>> for (;;) { >>> - error = tsleep(&nanowait, PWAIT | PCATCH, "nanslp", >>> - tvtohz(&tv)); >>> - getnanouptime(&ts2); >>> - if (error != EWOULDBLOCK) { >>> - if (error == ERESTART) >>> - error = EINTR; >>> - if (rmt != NULL) { >>> - timespecsub(&ts, &ts2); >>> - if (ts.tv_sec < 0) >>> - timespecclear(&ts); >>> - *rmt = ts; >>> - } >>> + sleepq_lock(&nanowait); >>> + sleepq_add(&nanowait, NULL, "nanslp", PWAIT | PCATCH, 0); >>> + sleepq_set_timeout_bt(&nanowait,bt); >>> + error = sleepq_timedwait_sig(&nanowait,catch); >>> + binuptime(&bt2); >>> + if (catch) { >>> + if (error != EWOULDBLOCK) { >>> + if (error == ERESTART) >>> + error = EINTR; >>> + if (rmt != NULL) { >>> + tmp = bt; >>> + bintime_sub(&tmp, &bt2); >>> + bintime2timespec(&tmp,&ts); >>> + if (ts.tv_sec < 0) >>> + timespecclear(&ts); >>> + *rmt = ts; >>> + } >>> return (error); >>> + } >>> } >>> - if (timespeccmp(&ts2, &ts, >=)) >>> + if (bintime_cmp(&bt2, &bt, >=)) >>> return (0); >>> - ts3 = ts; >>> - timespecsub(&ts3, &ts2); >>> - TIMESPEC_TO_TIMEVAL(&tv, &ts3); >>> } >>> } >>> >>> >>> Modified: projects/calloutng/sys/kern/subr_sleepqueue.c >>> ============================================================================== >>> --- projects/calloutng/sys/kern/subr_sleepqueue.c Fri Jun 8 11:40:30 2012 (r236743) >>> +++ projects/calloutng/sys/kern/subr_sleepqueue.c Fri Jun 8 11:53:51 2012 (r236744) >>> @@ -361,6 +361,22 @@ sleepq_add(void *wchan, struct lock_obje >>> * Sets a timeout that will remove the current thread from the specified >>> * sleep queue after timo ticks if the thread has not already been awakened. >>> */ >>> +void >>> +sleepq_set_timeout_bt(void *wchan, struct bintime bt) >>> +{ >>> + >>> + struct sleepqueue_chain *sc; >>> + struct thread *td; >>> + >>> + td = curthread; >>> + sc = SC_LOOKUP(wchan); >>> + mtx_assert(&sc->sc_lock, MA_OWNED); >>> + MPASS(TD_ON_SLEEPQ(td)); >>> + MPASS(td->td_sleepqueue == NULL); >>> + MPASS(wchan != NULL); >>> + callout_reset_bt_on(&td->td_slpcallout, bt, sleepq_timeout, td, PCPU_GET(cpuid)); >>> +} >>> + >> >> For this, I'd rather prefer that you patch sleepq_set_timeout() directly to be: >> void sleepq_set_timeout(void *wchan, int timo, struct bintime *bt);u >> >> Then, if you pass a NULL ptr to bt the 'timo' is used, otherwise bt is >> given preference in the logic. You will need to patch the current few >> callers of sleepq_set_timo() to get NULL, but it is a small patch. >> I would really like that you do the same also in the callout KPI, >> possibly, because this avoids a lot of code duplication. >> >> Attilio >> >> >> -- >> Peace can only be achieved by understanding - A. Einstein > > Attilio, > I agree with you about the few clients of sleepq_set_timeout(), and I > may change it easily. > About the callout_* KPI, if you refer to the recently added > callout_reset_bt_on() function, I have some concerns. > Right now, the number of consumers of callout_reset() or > callout_reset_on() is a lot bigger than the one of > sleepq_set_timeout() to consider a change, at least from my point of > view. > Moreover, differently from previous case, callout_reset_bt_on() > doesn't duplicate code because I've moved the old code there and made > callout_reset_on() a wrapper in which conversion is made and > callout_reset_bt_on() is called. Ok, for the moment just stick with the sleepq change. About the callout KPI we will see later on once you have a commit candidate. Attilio -- Peace can only be achieved by understanding - A. Einsteinhelp
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAJ-FndC8-NfkLGLGaaOnR1_HGHyDNHxEKYNp3a2FggYtqZ%2B7nA>