svn commit: r236744 - in projects/calloutng/sys: kern sys
Attilio Rao
attilio at freebsd.org
Sat Jun 9 15:55:02 UTC 2012
2012/6/9 Alexander Motin <mav at freebsd.org>:
> On 06/08/12 15:52, Attilio Rao wrote:
>>
>> 2012/6/8 Davide Italiano<davide at 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);
>>
>> 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.
>
>
> As opposite opinion, I don't like an idea of functions with both kinds of
> time arguments. It will break existing API/ABI (yes, rarely used, but
> already existing and documented) without giving any real benefits.
> Duplication of few code lines IMHO is not an argument at all if we are
> speaking about choosing API. Speaking about such a popular API as
> callout(9), I believe API breakage is just not an option.
I respectfully disagree.
We can break KPI between major version and this is not a problem. You
can consider to add some compatibility churn like this one for MFC, if
you need, but for HEAD we should really have cleaner code rather than
code duplication which brings to less mainteneability. For other
example of double-loaded arguments, please look at the interrupt
filter/interrupt threads interface.
I really think having a specific function just for passing timo/bt is
overkill. And you are really making a major change which is expected
to bring to changed KPI.
Attilio
--
Peace can only be achieved by understanding - A. Einstein
More information about the svn-src-projects
mailing list