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

On Mon, Dec 17, 2012 at 11:03:53PM +0200, Alexander Motin wrote:
> Hi.
> 
> > 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

cheers
luigi

> 
> 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.
> 
> PS: Please keep me in CC.
> 
> -- 
> Alexander Motin