From owner-freebsd-current@FreeBSD.ORG Tue Dec 18 17:38:03 2012 Return-Path: Delivered-To: freebsd-current@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id DD567DB6; Tue, 18 Dec 2012 17:38:03 +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 6192E8FC13; Tue, 18 Dec 2012 17:38:03 +0000 (UTC) Received: by onelab2.iet.unipi.it (Postfix, from userid 275) id CA6527300A; Tue, 18 Dec 2012 18:36:43 +0100 (CET) Date: Tue, 18 Dec 2012 18:36:43 +0100 From: Luigi Rizzo To: Alexander Motin Subject: Re: API explosion (Re: [RFC/RFT] calloutng) Message-ID: <20121218173643.GA94266@onelab2.iet.unipi.it> References: <50CF88B9.6040004@FreeBSD.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <50CF88B9.6040004@FreeBSD.org> User-Agent: Mutt/1.4.2.3i Cc: Davide Italiano , freebsd-current , "freebsd-arch@freebsd.org" X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 18 Dec 2012 17:38:03 -0000 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