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

On 18.12.2012 20:03, Alexander Motin wrote:
> 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.

What will you say about this patch:
http://people.freebsd.org/~mav/calloutng_api2.patch
?

It is the second way of above. Somewhat less functions, one extra 
argument, no branching.

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