From owner-freebsd-arch@FreeBSD.ORG Mon Dec 17 20:02:23 2012 Return-Path: Delivered-To: freebsd-arch@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 8C9A3F3; Mon, 17 Dec 2012 20:02:23 +0000 (UTC) (envelope-from luigi@onelab2.iet.unipi.it) Received: from onelab2.iet.unipi.it (onelab2.iet.unipi.it [131.114.59.238]) by mx1.freebsd.org (Postfix) with ESMTP id D94858FC13; Mon, 17 Dec 2012 20:02:22 +0000 (UTC) Received: by onelab2.iet.unipi.it (Postfix, from userid 275) id 68BF97300A; Mon, 17 Dec 2012 21:01:02 +0100 (CET) Date: Mon, 17 Dec 2012 21:01:02 +0100 From: Luigi Rizzo To: Davide Italiano Subject: API explosion (Re: [RFC/RFT] calloutng) Message-ID: <20121217200102.GA83832@onelab2.iet.unipi.it> References: Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.4.2.3i Cc: freebsd-current , freebsd-arch@freebsd.org X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 17 Dec 2012 20:02:23 -0000 [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 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