Date: Sat, 9 Jun 2012 17:38:06 +0100 From: Attilio Rao <attilio@freebsd.org> To: Alexander Motin <mav@freebsd.org> Cc: Davide Italiano <davide@freebsd.org>, svn-src-projects@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r236744 - in projects/calloutng/sys: kern sys Message-ID: <CAJ-FndD2=kpTmdOZX4HAJgyoUpxrbe5wdr_oBm0CB8%2BYyM_ykQ@mail.gmail.com> In-Reply-To: <4FD3758E.4030702@FreeBSD.org> References: <201206081153.q58BrqG2056771@svn.freebsd.org> <CAJ-FndB7QPRnmqrwyR9ixF9Jom8n7P1OMs=rczOKhmRxt08m3g@mail.gmail.com> <4FD3707B.1050407@FreeBSD.org> <CAJ-FndD708o6ZX8tSZ-J-_LGEJ=5xOS0we9gV8n1T_S6=_ha6g@mail.gmail.com> <4FD3758E.4030702@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
2012/6/9 Alexander Motin <mav@freebsd.org>: > On 06/09/12 18:54, Attilio Rao wrote: >> >> 2012/6/9 Alexander Motin<mav@freebsd.org>: >>> >>> On 06/08/12 15:52, Attilio Rao wrote: >>>> >>>> >>>> 2012/6/8 Davide Italiano<davide@freebsd.org>: >>>>> >>>>> >>>>> Author: davide >>>>> Date: Fri Jun =C2=A08 11:53:51 2012 >>>>> New Revision: 236744 >>>>> URL: http://svn.freebsd.org/changeset/base/236744 >>>>> >>>>> Log: >>>>> =C2=A0Add (experimentally) a function to the sleepqueue(9) KPI >>>>> =C2=A0sleepq_set_timeout_bt() in which the timeout may be specified i= n terms >>>>> =C2=A0of bintime rather than ticks, and which takes advantage of the = new >>>>> =C2=A0precision capabilities of the callout subsystem. >>>>> >>>>> =C2=A0Modify the kern_nanosleep() function so that it may rely on >>>>> =C2=A0sleepq_set_timeout_bt() rather than tsleep(). >>>>> >>>>> Modified: >>>>> =C2=A0projects/calloutng/sys/kern/kern_time.c >>>>> =C2=A0projects/calloutng/sys/kern/subr_sleepqueue.c >>>>> =C2=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 =C2=A0 =C2=A0 Fri Jun =C2= =A08 11:40:30 >>>>> 2012 >>>>> =C2=A0 =C2=A0 =C2=A0 =C2=A0(r236743) >>>>> +++ projects/calloutng/sys/kern/kern_time.c =C2=A0 =C2=A0 Fri Jun =C2= =A08 11:53:51 >>>>> 2012 >>>>> =C2=A0 =C2=A0 =C2=A0 =C2=A0(r236744) >>>>> @@ -43,6 +43,7 @@ __FBSDID("$FreeBSD$"); >>>>> =C2=A0#include<sys/resourcevar.h> >>>>> =C2=A0#include<sys/signalvar.h> >>>>> =C2=A0#include<sys/kernel.h> >>>>> +#include<sys/sleepqueue.h> >>>>> =C2=A0#include<sys/syscallsubr.h> >>>>> =C2=A0#include<sys/sysctl.h> >>>>> =C2=A0#include<sys/sysent.h> >>>>> @@ -352,37 +353,40 @@ static int nanowait; >>>>> =C2=A0int >>>>> =C2=A0kern_nanosleep(struct thread *td, struct timespec *rqt, struct >>>>> timespec >>>>> *rmt) >>>>> =C2=A0{ >>>>> - =C2=A0 =C2=A0 =C2=A0 struct timespec ts, ts2, ts3; >>>>> - =C2=A0 =C2=A0 =C2=A0 struct timeval tv; >>>>> - =C2=A0 =C2=A0 =C2=A0 int error; >>>>> + =C2=A0 =C2=A0 =C2=A0 struct timespec ts; >>>>> + =C2=A0 =C2=A0 =C2=A0 struct bintime bt, bt2, tmp; >>>>> + =C2=A0 =C2=A0 =C2=A0 int catch =3D 0, error; >>>>> >>>>> =C2=A0 =C2=A0 =C2=A0 =C2=A0if (rqt->tv_nsec< =C2=A0 =C2=A00 || rqt->t= v_nsec>=3D 1000000000) >>>>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return (EINVAL= ); >>>>> =C2=A0 =C2=A0 =C2=A0 =C2=A0if (rqt->tv_sec< =C2=A0 =C2=A00 || (rqt->t= v_sec =3D=3D 0&& =C2=A0 =C2=A0rqt->tv_nsec =3D=3D >>>>> 0)) >>>>> >>>>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return (0); >>>>> - =C2=A0 =C2=A0 =C2=A0 getnanouptime(&ts); >>>>> - =C2=A0 =C2=A0 =C2=A0 timespecadd(&ts, rqt); >>>>> - =C2=A0 =C2=A0 =C2=A0 TIMESPEC_TO_TIMEVAL(&tv, rqt); >>>>> + =C2=A0 =C2=A0 =C2=A0 binuptime(&bt); >>>>> + =C2=A0 =C2=A0 =C2=A0 timespec2bintime(rqt,&tmp); >>>>> + =C2=A0 =C2=A0 =C2=A0 bintime_add(&bt,&tmp); >>>>> =C2=A0 =C2=A0 =C2=A0 =C2=A0for (;;) { >>>>> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 error =3D tsleep(&= nanowait, PWAIT | PCATCH, "nanslp", >>>>> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 tvto= hz(&tv)); >>>>> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 getnanouptime(&ts2= ); >>>>> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (error !=3D EWO= ULDBLOCK) { >>>>> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 if (error =3D=3D ERESTART) >>>>> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 error =3D EINTR; >>>>> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 if (rmt !=3D NULL) { >>>>> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 timespecsub(&ts,&ts2); >>>>> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (ts.tv_sec< =C2=A0 =C2=A00) >>>>> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 timespec= clear(&ts); >>>>> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 *rmt =3D ts; >>>>> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 } >>>>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 sleepq_lock(&nanow= ait); >>>>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 sleepq_add(&nanowa= it, NULL, "nanslp", PWAIT | PCATCH, >>>>> 0); >>>>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 sleepq_set_timeout= _bt(&nanowait,bt); >>>>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 error =3D sleepq_t= imedwait_sig(&nanowait,catch); >>>>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 binuptime(&bt2); >>>>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (catch) { >>>>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 if (error !=3D EWOULDBLOCK) { >>>>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (error =3D=3D ERESTART) >>>>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 error = =3D EINTR; >>>>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (rmt !=3D NULL) { >>>>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 tmp =3D = bt; >>>>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 bintime_= sub(&tmp,&bt2); >>>>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 bintime2= timespec(&tmp,&ts); >>>>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (ts.t= v_sec< =C2=A0 =C2=A00) >>>>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 timespecclear(&ts); >>>>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 *rmt =3D= ts; >>>>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 } >>>>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0return (error); >>>>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 } >>>>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0} >>>>> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (timespeccmp(&t= s2,&ts,>=3D)) >>>>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (bintime_cmp(&b= t2,&bt,>=3D)) >>>>> >>>>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0return (0); >>>>> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ts3 =3D ts; >>>>> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 timespecsub(&ts3,&= ts2); >>>>> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 TIMESPEC_TO_TIMEVA= L(&tv,&ts3); >>>>> =C2=A0 =C2=A0 =C2=A0 =C2=A0} >>>>> =C2=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 =C2=A0 =C2=A0 =C2= =A0 Fri Jun =C2=A08 >>>>> 11:40:30 2012 =C2=A0 =C2=A0 =C2=A0 =C2=A0(r236743) >>>>> +++ projects/calloutng/sys/kern/subr_sleepqueue.c =C2=A0 =C2=A0 =C2= =A0 Fri Jun =C2=A08 >>>>> 11:53:51 2012 =C2=A0 =C2=A0 =C2=A0 =C2=A0(r236744) >>>>> @@ -361,6 +361,22 @@ sleepq_add(void *wchan, struct lock_obje >>>>> =C2=A0* Sets a timeout that will remove the current thread from the >>>>> specified >>>>> =C2=A0* sleep queue after timo ticks if the thread has not already be= en >>>>> awakened. >>>>> =C2=A0*/ >>>>> +void >>>>> +sleepq_set_timeout_bt(void *wchan, struct bintime bt) >>>>> +{ >>>>> + >>>>> + =C2=A0 =C2=A0 =C2=A0 struct sleepqueue_chain *sc; >>>>> + =C2=A0 =C2=A0 =C2=A0 struct thread *td; >>>>> + >>>>> + =C2=A0 =C2=A0 =C2=A0 td =3D curthread; >>>>> + =C2=A0 =C2=A0 =C2=A0 sc =3D SC_LOOKUP(wchan); >>>>> + =C2=A0 =C2=A0 =C2=A0 mtx_assert(&sc->sc_lock, MA_OWNED); >>>>> + =C2=A0 =C2=A0 =C2=A0 MPASS(TD_ON_SLEEPQ(td)); >>>>> + =C2=A0 =C2=A0 =C2=A0 MPASS(td->td_sleepqueue =3D=3D NULL); >>>>> + =C2=A0 =C2=A0 =C2=A0 MPASS(wchan !=3D NULL); >>>>> + =C2=A0 =C2=A0 =C2=A0 callout_reset_bt_on(&td->td_slpcallout, bt, sl= eepq_timeout, td, >>>>> PCPU_GET(cpuid)); >>>>> +} >>>>> + >>>> >>>> >>>> >>>> For this, I'd rather prefer that you patch sleepq_set_timeout() direct= ly >>>> 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. > > > That is a question of comparing prices. In my opinion, the fact that we c= an > change KPI between major releases does not mean that we should do it with= out > really strong reason. IMO open source world too often tends to change the > rules and APIs. That may work for in-tree code, when person changing API > will catch up all consequences. But each change of that kind can make sad > some vendor you never heard about, when his code will no longer compile a= nd > he will have to either add some more #ifdef's or drop FreeBSD support. If > keeping compatibility would cost us some performance or hundreds lines of > extra code, I would agree, but IMHO this case with few lines of code is > different. Well, I spoke privately about that with Davide too and already spent a lot of words for this too, which is an issue which just doesn't deserve that. I just wanted to show you that it is true that we already have code doing that (ifilter), that it doesn't violate any rule and that it keeps the KPI smaller and avoids code duplication. You are free to ignore my opinion, this is your project, but for an example of the "unmaintenability" please look at the newer Davide's code about adding direct dispatching of callouts on ISR directly. It patches correctly the _bt() variant of callout but completely leaves out the timo variant. While this is not a problem because of code committed to a project branch I think this perfectly explains what I mean. Please feel free to ignore the whole thread at you convenience, though. Attilio --=20 Peace can only be achieved by understanding - A. Einstein
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAJ-FndD2=kpTmdOZX4HAJgyoUpxrbe5wdr_oBm0CB8%2BYyM_ykQ>