Date: Wed, 20 Jun 2012 15:25:44 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Davide Italiano <davide.italiano@gmail.com> Cc: Attilio Rao <attilio@FreeBSD.org>, svn-src-projects@FreeBSD.org, Alexander Motin <mav@FreeBSD.org>, src-committers@FreeBSD.org Subject: Re: svn commit: r237202 - in projects/calloutng/sys: kern sys Message-ID: <20120620151115.A1111@besplex.bde.org> In-Reply-To: <CACYV=-E6zE8D_2X4VE-uGxuTBxJmouHmO1iSzA0EfGPi7WOmkg@mail.gmail.com> References: <201206172045.q5HKjj92031635@svn.freebsd.org> <CAJ-FndA%2BzK5seKuVhx%2BQsprK2NTP0yqrREbSG3Nk4Qa%2B%2Be9S3A@mail.gmail.com> <CACYV=-E6zE8D_2X4VE-uGxuTBxJmouHmO1iSzA0EfGPi7WOmkg@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
This message is in MIME format. The first part should be readable text, while the remaining parts are likely unreadable without MIME-aware tools. --0-670748366-1340169944=:1111 Content-Type: TEXT/PLAIN; charset=X-UNKNOWN; format=flowed Content-Transfer-Encoding: QUOTED-PRINTABLE On Tue, 19 Jun 2012, Davide Italiano wrote: > On Mon, Jun 18, 2012 at 3:40 PM, Attilio Rao <attilio@freebsd.org> wrote: >> 2012/6/17, Davide Italiano <davide@freebsd.org>: >>> Author: davide >>> Date: Sun Jun 17 20:45:45 2012 >>> New Revision: 237202 >>> URL: http://svn.freebsd.org/changeset/base/237202 >>> >>> Log: >>> =A0 - Extend the condvar(9) KPI introducing a new cv_timedwait_bt_sig() >>> =A0 function so that we can specify timeout precision in terms of struc= t >>> =A0 bintime. >>> >>> =A0 - Now seltdwait() takes three argument rather than two so that thei= r >>> =A0 consumers can specify if the timeout should be passed as ticks or >>> bintime. >>> >>> =A0 - Refactor the kern_select() and the sys_poll() code so that these = two >>> =A0 services may rely on cv_timedwait_bt_sig() rather than on the previ= ous >>> less >>> =A0 precise cv_timedwait_sig(). >>> >>> =A0 - Rethink the sleepqueue(9) KPI in order to make an attempt of avoi= ding >>> =A0 both code duplication and breakages. Your mileage WILL vary, feel f= ree to >>> =A0 comment. >> >> I would still prefer the unified KPI, but at least the committed code >> avoids code-duplication, I appreciate your effort here, thanks. >> >> Why you don't do the same thing for callout_ KPI? I prefer separate functions with no flags args to tell them who they are, or at least macros to hide the difference (like timeout() still existss, and tsleep() is still spelled tsleep() although it is really msleep() with a macro hiding this). > Attilio, > here you can find a patch: > http://people.freebsd.org/~davide/callout_refactorkpi.diff > I'm not confident enough at this point to break all the > callout_reset_on() stuffs, but at least I thought it's good to provide > a variant of it named callout_reset_direct_on() that takes an additive > parameter so that you can specifiy if you want to execute your callout > directly from hw interrupt context or in SWI thread. I also patched > the callout_reset_on() to always execute in SWI thread, as it has done > until now. Note that this solution is a good compromise among > breakages and code duplication avoidance, even though, as you pointed > out makes the KPI more verbose. > If you want to make a review, feel free. > I've some doubt on the style of the macros I've added, to I cc'ed > Bruce, I guess he's the best person to point me out what I'm doing > wrong. I see you committed it. I pointed out some problems not related to the APIs in private mail. The macros for hiding the details of the API changes seem reasonable. % Index: sys/sys/callout.h % =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 % --- sys/sys/callout.h=09(revision 237202) % +++ sys/sys/callout.h=09(working copy) % ... % @@ -69,9 +71,16 @@ % =09_callout_init_lock((c), ((rw) !=3D NULL) ? &(rw)->lock_object :=09\ % =09 NULL, (flags)) % #define=09callout_pending(c)=09((c)->c_flags & CALLOUT_PENDING) % -int=09callout_reset_bt_on(struct callout *, struct bintime, void(*)(void= *), % -=09 void *, int, int); % -int=09callout_reset_on(struct callout *, int, void (*)(void *), void *, = int); % +int=09_callout_reset_on(struct callout *, struct bintime *, int, % +=09 void (*)(void *), void *, int, int); % +#define=09callout_reset_on(c, to_ticks, fn, arg, cpu)=09=09=09\ % + _callout_reset_on((c), (NULL), (to_ticks), (fn), (arg), (cpu),=09\ % + (0)) % +#define callout_reset_flags_on(c, to_ticks, fn, arg, cpu, flags)=09\ % + _callout_reset_on((c), (NULL), (to_ticks), (fn), (arg), (cpu),=09\ % + (flags))=20 % +#define callout_reset_bt_on(c, bt, fn, arg, cpu, direct)=09=09\ % + _callout_reset_on((c), (bt), (0), (fn), (arg), (cpu), (direct))=20 % #define=09callout_reset(c, on_tick, fn, arg)=09=09=09=09\ % callout_reset_on((c), (on_tick), (fn), (arg), (c)->c_cpu) % #define=09callout_reset_curcpu(c, on_tick, fn, arg)=09=09=09\ I think I would sort the macros more. They don't have to be unsorted to place them after the functions that they invoke. Either put them all in a macro section separate from the prototypes, or sort them in the prototype= s. 2 out of 3 of the the new #defines have tab lossage, unlike all of the old #defines. The parentheses around (0) and (NULL)) are bogus. Especially the latter. Parentheses that are necessary are ugly enough, and using unnecessary ones makes it harder to see that all the necessary ones are there. After removing unnecessary parentheses, 2 lines need splitting even less than before. I didn't notice so many style bugs in the committed version. Bruce --0-670748366-1340169944=:1111--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20120620151115.A1111>