Re: API explosion (Re: [RFC/RFT] calloutng)

On 18.12.2012 19:36, Luigi Rizzo wrote:
> On Mon, Dec 17, 2012 at 11:03:53PM +0200, Alexander Motin wrote:
>>> I would instead do the following:
>>
>> I also don't very like the wide API and want to hear fresh ideas, but
>> approaches to time measurement there are too different to do what you
>> are proposing.  Main problem is that while ticks value is relative,
>> bintime is absolute. It is not easy to make conversion between them fast
>> and precise. I've managed to do it, but the only function that does it
>> now is _callout_reset_on(). All other functions are just passing values
>> down. I am not sure I want to duplicate that code in each place, though
>> doing it at least for for callout may be a good idea.
>
> I am afraid the above is not convincing.
>
> Most/all of the APIs i mentioned still have the conversion from
> ticks to bintime, and the code in your patch is just
> building multiple parallel paths (one for each of the
> three versions of the same function) to some final
> piece of code where the conversion takes place.
>
> The problem is that all of this goes through a set of obfuscating
> macros and the end result is horrible.
>
> To be clear, i believe the work you have been doing on cleaning up
> callout is great, i am just saying that this is the time to look
> at the code from a few steps away and clean up all those design
> decisions that perhaps were made in a haste to make things work.
>
> I will give you another example to show how convoluted
> is the code now:
>
> cv_timedwait() and cv_timedwait_sig() now have three
> versions each (plain, bt, flags).
>
> These six are remapped through macros to two functions, _cv_timedwait()
> and _cv_timedwait_sig(),  with a possible bug (cv_timedwait_bt()
> maps to _cv_timedwait_sig() )
>
> These two _cv_timedwait*() take both ticks and bintimes,
> and contain this sequence:
>
> +	if (bt == NULL)
> +		sleepq_set_timeout_flags(cvp, timo, flags);
> +	else
> +		sleepq_set_timeout_bt(cvp, bt, precision);
>
> Guess what, both sleepq_* are macros that remap to the same
> _sleepq_set_timeout(...) . So the above "if (bt == NULL)" is useless.
>
> But then if you dig into _sleepq_set_timeout() you'll see
>
> +	if (bt == NULL)
> +		callout_reset_flags_on(&td->td_slpcallout, timo,
> +		    sleepq_timeout, td, PCPU_GET(cpuid), flags | C_DIRECT_EXEC);
> +	else
> +		callout_reset_bt_on(&td->td_slpcallout, bt, precision,
> +		    sleepq_timeout, td, PCPU_GET(cpuid), flags | C_DIRECT_EXEC);
>
> and again both callout_reset*() are again remapped through
> macros to _callout_reset_on(), so another useless "if (bt == NULL)"
> And in the end you have the conversion from ticks to bintime.
>
> So basically the code path for cv_timedwait() has those
> two useless switches and one useless extra argument,
> and the conversion from ticks to bintime is down
> deep down in _callout_reset_on() where it can only
> be resolved at runtime, whereas by doing the conversion
> at the beginning the decision could have been made at compile
> time.
>
> So I believe my proposal would give large simplifications in
> the code and lead to a much cleaner implementation of what
> you have designed:
>
> 1. acknowledge the fact that the only representation of time
>     that callouts use internally is a bintime+precision, define one
>     single function (instead of two or three or six) that implements
>     the blessed API, and implement the others with macros or
>     inline functions doing the appropriate conversions;
>
> 2. specifically, the *_flags() variant has no reason to exist.
>     It can be implemented through the *_bt() variant, and
>     being a new function the only places where you introduce it
>     require manual modifications so you can directly invoke
>     the new function.
>
> Again, please take this as constructive criticism, as i really
> like the work you have been doing and appreciate the time and
> effort you are putting on it

Your words about useless cascaded ifs touched me. Actually I've looked 
on _callout_reset_bt_on() yesterday, thinking about moving tick to bt 
conversion to separate function or wrapper. The only thing we would save 
in such case is single integer argument (ticks), as all others (bt, 
prec, flags) are used in the new world order. From the other side, to 
make the conversion process really effective and correct, I've used 
quite specific way of obtaining time, that may change in the future. I 
would not really like it to be inlined in every consumer function and 
become an ABI. So I see two possible ways: make that conversion a 
separate non-inline function (that will require two temporary variables 
to return results and will consume some time on call/return), or make 
callout_reset_bt_on() to have extra ticks argument, allowing to use it 
in one or another way without external ifs and macros. In last case all 
_bt functions in other APIs will also obtain ticks, bt, pr and flags 
arguments. Actually flags there could be used to specify time scale 
(monotonic or wall) and time base (relative or absolute), if we decide 
to implement all of them at some point.

>> Creating sets of three functions I had three different goals:
>>   - callout_reset() -- it is legacy variant required to keep API
>> compatibility;
>>   - callout_reset_flags() -- it is for cases where custom precision
>> specification needs to be added to the existing code, or where direct
>> callout execution is needed. Conversion to bintime would additionally
>> complicate consumer code, that I would try to avoid.
>>   - callout_reset_bt() -- API for the new code, which needs high
>> precision and doesn't mind to operate bintime. Now there is only three
>> such places in kernel now, and I don't think there will be much more.
>>
>> Respectively, these three options are replicated to other APIs where
>> time intervals are used.

-- 
Alexander Motin