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

[top posting for readability;
in summary we were discussing the new callout API trying to avoid
an explosion of methods and arguments while at the same time
supporting the old API and the new one]
(I am also Cc-ing phk as he might have better insight
on the topic).

I think the patch you propose is a step in the right direction,
but i still remain concerned by having to pass two bintimes
(by reference, but they should really go by value)
and one 'ticks' value to all these functions.

I am also dubious that we need a full 128 bits to specify
the 'precision': there would be absolutely no loss of functionality
if we decided to specify the precision in powers of 2, so a precision
'k' (signed) means 2^k seconds. This way 8 bits are enough to
represent any precision we want.

The key difference between 'ticks' and bintimes (and the main
difficulty in the conversion) is that ticks are relative and bintimes
are interpreted as absolute. This could be easily solved by using
a flag to specify if the 'bt' argument is absolute or relative, and
passing the argument by value.

So now the flags could contain C_DIRECT_EXEC, C_BT_IS_RELATIVE, the
precision, and another 64 or 128 bit field contains the bintime.

How does this look ?

cheers
luigi

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