Skip site navigation (1)Skip section navigation (2) 
Date:      Fri, 8 Jun 2012 15:08:24 +0200
From:      Davide Italiano <davide@freebsd.org>
To:        Attilio Rao <attilio@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:  <CACYV=-FD6qNAK_y532y3jQQhCh7Ct7venkEsxYmFBEiqZMPnyw@mail.gmail.com>
In-Reply-To: <CAJ-FndB7QPRnmqrwyR9ixF9Jom8n7P1OMs=rczOKhmRxt08m3g@mail.gmail.com>
References:  <201206081153.q58BrqG2056771@svn.freebsd.org> <CAJ-FndB7QPRnmqrwyR9ixF9Jom8n7P1OMs=rczOKhmRxt08m3g@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help 
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 =A08 11:53:51 2012
>> New Revision: 236744
>> URL: http://svn.freebsd.org/changeset/base/236744
>>
>> Log:
>> =A0Add (experimentally) a function to the sleepqueue(9) KPI
>> =A0sleepq_set_timeout_bt() in which the timeout may be specified in term=
s
>> =A0of bintime rather than ticks, and which takes advantage of the new
>> =A0precision capabilities of the callout subsystem.
>>
>> =A0Modify the kern_nanosleep() function so that it may rely on
>> =A0sleepq_set_timeout_bt() rather than tsleep().
>>
>> Modified:
>> =A0projects/calloutng/sys/kern/kern_time.c
>> =A0projects/calloutng/sys/kern/subr_sleepqueue.c
>> =A0projects/calloutng/sys/sys/sleepqueue.h
>>
>> Modified: projects/calloutng/sys/kern/kern_time.c
>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D
>> --- projects/calloutng/sys/kern/kern_time.c =A0 =A0 Fri Jun =A08 11:40:3=
0 2012 =A0 =A0 =A0 =A0(r236743)
>> +++ projects/calloutng/sys/kern/kern_time.c =A0 =A0 Fri Jun =A08 11:53:5=
1 2012 =A0 =A0 =A0 =A0(r236744)
>> @@ -43,6 +43,7 @@ __FBSDID("$FreeBSD$");
>> =A0#include <sys/resourcevar.h>
>> =A0#include <sys/signalvar.h>
>> =A0#include <sys/kernel.h>
>> +#include <sys/sleepqueue.h>
>> =A0#include <sys/syscallsubr.h>
>> =A0#include <sys/sysctl.h>
>> =A0#include <sys/sysent.h>
>> @@ -352,37 +353,40 @@ static int nanowait;
>> =A0int
>> =A0kern_nanosleep(struct thread *td, struct timespec *rqt, struct timesp=
ec *rmt)
>> =A0{
>> - =A0 =A0 =A0 struct timespec ts, ts2, ts3;
>> - =A0 =A0 =A0 struct timeval tv;
>> - =A0 =A0 =A0 int error;
>> + =A0 =A0 =A0 struct timespec ts;
>> + =A0 =A0 =A0 struct bintime bt, bt2, tmp;
>> + =A0 =A0 =A0 int catch =3D 0, error;
>>
>> =A0 =A0 =A0 =A0if (rqt->tv_nsec < 0 || rqt->tv_nsec >=3D 1000000000)
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return (EINVAL);
>> =A0 =A0 =A0 =A0if (rqt->tv_sec < 0 || (rqt->tv_sec =3D=3D 0 && rqt->tv_n=
sec =3D=3D 0))
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return (0);
>> - =A0 =A0 =A0 getnanouptime(&ts);
>> - =A0 =A0 =A0 timespecadd(&ts, rqt);
>> - =A0 =A0 =A0 TIMESPEC_TO_TIMEVAL(&tv, rqt);
>> + =A0 =A0 =A0 binuptime(&bt);
>> + =A0 =A0 =A0 timespec2bintime(rqt, &tmp);
>> + =A0 =A0 =A0 bintime_add(&bt,&tmp);
>> =A0 =A0 =A0 =A0for (;;) {
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 error =3D tsleep(&nanowait, PWAIT | PCATCH=
, "nanslp",
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 tvtohz(&tv));
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 getnanouptime(&ts2);
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (error !=3D EWOULDBLOCK) {
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (error =3D=3D ERESTART)
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 error =3D =
EINTR;
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (rmt !=3D NULL) {
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 timespecsu=
b(&ts, &ts2);
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (ts.tv_=
sec < 0)
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 timespecclear(&ts);
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 *rmt =3D t=
s;
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 sleepq_lock(&nanowait);
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 sleepq_add(&nanowait, NULL, "nanslp", PWAI=
T | PCATCH, 0);
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 sleepq_set_timeout_bt(&nanowait,bt);
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 error =3D sleepq_timedwait_sig(&nanowait,c=
atch);
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 binuptime(&bt2);
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (catch) {
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (error !=3D EWOULDBLOCK=
) {
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (error =
=3D=3D ERESTART)
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 error =3D EINTR;
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (rmt !=
=3D NULL) {
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 tmp =3D bt;
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 bintime_sub(&tmp, &bt2);
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 bintime2timespec(&tmp,&ts);
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 if (ts.tv_sec < 0)
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 =A0 =A0 timespecclear(&ts);
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 *rmt =3D ts;
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return (error);
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0}
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (timespeccmp(&ts2, &ts, >=3D))
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (bintime_cmp(&bt2, &bt, >=3D))
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return (0);
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 ts3 =3D ts;
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 timespecsub(&ts3, &ts2);
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 TIMESPEC_TO_TIMEVAL(&tv, &ts3);
>> =A0 =A0 =A0 =A0}
>> =A0}
>>
>>
>> Modified: projects/calloutng/sys/kern/subr_sleepqueue.c
>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D
>> --- projects/calloutng/sys/kern/subr_sleepqueue.c =A0 =A0 =A0 Fri Jun =
=A08 11:40:30 2012 =A0 =A0 =A0 =A0(r236743)
>> +++ projects/calloutng/sys/kern/subr_sleepqueue.c =A0 =A0 =A0 Fri Jun =
=A08 11:53:51 2012 =A0 =A0 =A0 =A0(r236744)
>> @@ -361,6 +361,22 @@ sleepq_add(void *wchan, struct lock_obje
>> =A0* Sets a timeout that will remove the current thread from the specifi=
ed
>> =A0* sleep queue after timo ticks if the thread has not already been awa=
kened.
>> =A0*/
>> +void
>> +sleepq_set_timeout_bt(void *wchan, struct bintime bt)
>> +{
>> +
>> + =A0 =A0 =A0 struct sleepqueue_chain *sc;
>> + =A0 =A0 =A0 struct thread *td;
>> +
>> + =A0 =A0 =A0 td =3D curthread;
>> + =A0 =A0 =A0 sc =3D SC_LOOKUP(wchan);
>> + =A0 =A0 =A0 mtx_assert(&sc->sc_lock, MA_OWNED);
>> + =A0 =A0 =A0 MPASS(TD_ON_SLEEPQ(td));
>> + =A0 =A0 =A0 MPASS(td->td_sleepqueue =3D=3D NULL);
>> + =A0 =A0 =A0 MPASS(wchan !=3D NULL);
>> + =A0 =A0 =A0 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.

Davide

Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CACYV=-FD6qNAK_y532y3jQQhCh7Ct7venkEsxYmFBEiqZMPnyw>