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

[Luigi Rizzo]

[again, response to another issue i raised]

On Fri, Dec 14, 2012 at 01:57:36PM +0100, Davide Italiano wrote:
> On Fri, Dec 14, 2012 at 7:41 AM, Luigi Rizzo <rizzo at iet.unipi.it> wrote:
...
> > Finally, a more substantial comment:
> > - a lot of functions which formerly had only a "timo" argument
> >   now have "timo, bt, precision, flags". Take seltdwait() as an example.
> >
> 
> seltdwait() is not part of the public KPI. It has been modified to
> avoid code duplication. Having seltdwait() and seltdwait_bt(), i.e.
> two separate functions, even though we could share most of the code is
> not a clever approach, IMHO.
> As I told before, seltdwait() is not exposed so we can modify its
> argument without breaking anything.
> 
> >   It seems that you have been undecided between two approaches:
> >   for some of these functions you have preserved the original function
> >   that deals with ticks and introduced a new one that deals with the
> > bintime,
> >   whereas in other cases you have modified the original function to add
> >   "bt, precision, flags".
> >
> 
> I'm not. All the functions which are part of the public KPI (e.g.
> condvar(9), sleepq(9), *) are still available.  *_flags variants have
> been introduced so that consumers can take advantage of the new
> 'precision tolerance mechanism' implemented. Also, *_bt variants have
> been introduced. I don't see any "undecision" between the two
> approaches.
> Please note that now the callout backend deals with bintime, so every
> time callout_reset_on() is called, the 'tick' argument passed is
> silently converted to bintime.

I will try to give more specific example, using the latest patch
from mav

http://people.freebsd.org/~mav/calloutng_12_16.patch

In the manpage, for instance, the existing functions
now are extended with two more variants (sometimes;
msleep_spin() for instance is missing msleep_spin_bt()
but perhaps that is just an oversight).

         .Nm sleepq_set_timeout ,
        +.Nm sleepq_set_timeout_flags ,
        +.Nm sleepq_set_timeout_bt ,

         .Nm msleep ,
        +.Nm msleep_flags ,
        +.Nm msleep_bt ,
         .Nm msleep_spin ,
        +.Nm msleep_spin_flags ,
         .Nm pause ,
        +.Nm pause_flags ,
        +.Nm pause_bt ,
         .Nm tsleep ,
        +.Nm tsleep_flags ,
        +.Nm tsleep_bt ,

         .Nm cv_timedwait ,
        +.Nm cv_timedwait_bt ,
        +.Nm cv_timedwait_flags ,
         .Nm cv_timedwait_sig ,
        +.Nm cv_timedwait_sig_bt ,
        +.Nm cv_timedwait_sig_flags ,

         .Nm callout_reset ,
        +.Nm callout_reset_flags ,
         .Nm callout_reset_on ,
        +.Nm callout_reset_flags_on ,
        +.Nm callout_reset_bt_on ,

If you look at the backends, they take both a timo and a bintime.

        -int    _cv_timedwait(struct cv *cvp, struct lock_object *lock, int timo);
        -int    _cv_timedwait_sig(struct cv *cvp, struct lock_object *lock, int timo);
        +int    _cv_timedwait(struct cv *cvp, struct lock_object *lock,
        +           struct bintime *bt, struct bintime *precision, int timo,
        +           int flags);
        +int    _cv_timedwait_sig(struct cv *cvp, struct lock_object *lock,
        +           struct bintime *bt, struct bintime *precision, int timo,
        +           int flags);

and then internally they call the 'timo' or the 'bt' version
depending on the case

+       if (bt == NULL)
+               sleepq_set_timeout_flags(cvp, timo, flags);
+       else
+               sleepq_set_timeout_bt(cvp, bt, precision);

So basically you are doing the following:
   + create two new variant for each existing function

        foo(, ... timo, ... )                   old method
        foo_flags(, ... timo, ... )             new method
        foo_bt(... , bt, precision, ...)        new method

     (the 'API explosion' i am mentioning in the Subject)

   + the variants are mapped to the same internal function
        _foo_internal(..., timo, bt, precision, flags, ...)

   + and then the internal function has a (runtime) conditional

        if (bt == NULL) {
                // convert timo to bt
        } else {
                // have a bt + precision
        }
        ...

I would instead do the following:

+ create a new API function that takes only bintime+precision+flags, no ticks.
  I am not sure if there is even a need to have an internal name
        _cv_timedwait_sig( .... )
  or you can just have
        cv_timedwait_sig_bt(...)

+ use a macro or an inline function to remap the old API to
  the (single) new one, making the argument conversion immediately.
        #define cv_timedwait_sig(...)   cv_timedwait_sig_bt( ...)

  This has the advantage that conversions might be done at compile
  time perhaps with some advantage in terms of code and performance.

+ do not bother creating yet another cv_timedwait_sig_flags() function.
  Since it did not exist before, you have to do the conversion manually
  anyways, at which point you just change the argument to use bintime
  instead of ticks.

Note that what i am proposing is a simplification of your code
and should not require much extra effort.

cheers
luigi