svn commit: r237202 - in projects/calloutng/sys: kern sys

[Bruce Evans]

On Tue, 19 Jun 2012, Davide Italiano wrote:

> On Mon, Jun 18, 2012 at 3:40 PM, Attilio Rao <attilio at freebsd.org> wrote:
>> 2012/6/17, Davide Italiano <davide at freebsd.org>:
>>> Author: davide
>>> Date: Sun Jun 17 20:45:45 2012
>>> New Revision: 237202
>>> URL: http://svn.freebsd.org/changeset/base/237202
>>>
>>> Log:
>>>   - Extend the condvar(9) KPI introducing a new cv_timedwait_bt_sig()
>>>   function so that we can specify timeout precision in terms of struct
>>>   bintime.
>>>
>>>   - Now seltdwait() takes three argument rather than two so that their
>>>   consumers can specify if the timeout should be passed as ticks or
>>> bintime.
>>>
>>>   - Refactor the kern_select() and the sys_poll() code so that these two
>>>   services may rely on cv_timedwait_bt_sig() rather than on the previous
>>> less
>>>   precise cv_timedwait_sig().
>>>
>>>   - Rethink the sleepqueue(9) KPI in order to make an attempt of avoiding
>>>   both code duplication and breakages. Your mileage WILL vary, feel free to
>>>   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
% ===================================================================
% --- sys/sys/callout.h	(revision 237202)
% +++ sys/sys/callout.h	(working copy)
% ...
% @@ -69,9 +71,16 @@
%  	_callout_init_lock((c), ((rw) != NULL) ? &(rw)->lock_object :	\
%  	   NULL, (flags))
%  #define	callout_pending(c)	((c)->c_flags & CALLOUT_PENDING)
% -int	callout_reset_bt_on(struct callout *, struct bintime, void(*)(void *),
% -	    void *, int, int);
% -int	callout_reset_on(struct callout *, int, void (*)(void *), void *, int);
% +int	_callout_reset_on(struct callout *, struct bintime *, int,
% +	    void (*)(void *), void *, int, int);
% +#define	callout_reset_on(c, to_ticks, fn, arg, cpu)			\
% +    _callout_reset_on((c), (NULL), (to_ticks), (fn), (arg), (cpu),	\
% +        (0))
% +#define callout_reset_flags_on(c, to_ticks, fn, arg, cpu, flags)	\
% +    _callout_reset_on((c), (NULL), (to_ticks), (fn), (arg), (cpu),	\
% +        (flags)) 
% +#define callout_reset_bt_on(c, bt, fn, arg, cpu, direct)		\
% +    _callout_reset_on((c), (bt), (0), (fn), (arg), (cpu), (direct)) 
%  #define	callout_reset(c, on_tick, fn, arg)				\
%      callout_reset_on((c), (on_tick), (fn), (arg), (c)->c_cpu)
%  #define	callout_reset_curcpu(c, on_tick, fn, arg)			\

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 prototypes.

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