From owner-freebsd-arch@FreeBSD.ORG Tue Dec 18 21:46:26 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 22788D0B; Tue, 18 Dec 2012 21:46:26 +0000 (UTC) (envelope-from mavbsd@gmail.com) Received: from mail-wi0-f177.google.com (mail-wi0-f177.google.com [209.85.212.177]) by mx1.freebsd.org (Postfix) with ESMTP id 4C96E8FC13; Tue, 18 Dec 2012 21:46:24 +0000 (UTC) Received: by mail-wi0-f177.google.com with SMTP id hm2so740685wib.4 for ; Tue, 18 Dec 2012 13:46:23 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=x-received:sender:message-id:date:from:user-agent:mime-version:to :cc:subject:references:in-reply-to:content-type :content-transfer-encoding; bh=A68M3grTghYB4r+16AGzieL1sMBAZo7c+1yR/mVHgog=; b=iaQtlPdUBNka2Ll0yOAcNFmT+A3Xdh8nkw7polEH7nqp6PlY4niYz3KUyanBWIKbfH /GZqHWm9exHES9bTyAnLN7k+37aenGG1GqixMmngrttu+y0Nok6Y3nIRjQ8zGN7BpWcw X8ZctZtsxOmOqBmv9W/lgtjXvODtdmWgLxTSjiRTwIAe+EWN50KNZmviV2+GHzeXk/IA K0A+5Aif/Y2jZ/Tp3DMr7zSDvyQWLhp3xAYUsDzbjSm+jOPTyAasWESa/ltJNIkTplZW I2AzXEe9Fe/Pru+/h6oRaKfF5qzYmz7AIA6L4o7I8Z+gsTdKB+XJk26dxsxFOCpe5CaQ VXUg== X-Received: by 10.194.118.229 with SMTP id kp5mr7520317wjb.2.1355867183807; Tue, 18 Dec 2012 13:46:23 -0800 (PST) Received: from mavbook.mavhome.dp.ua (mavhome.mavhome.dp.ua. [213.227.240.37]) by mx.google.com with ESMTPS id bd7sm4666335wib.8.2012.12.18.13.46.21 (version=TLSv1/SSLv3 cipher=OTHER); Tue, 18 Dec 2012 13:46:22 -0800 (PST) Sender: Alexander Motin Message-ID: <50D0E42B.6030605@FreeBSD.org> Date: Tue, 18 Dec 2012 23:46:19 +0200 From: Alexander Motin User-Agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:13.0) Gecko/20120628 Thunderbird/13.0.1 MIME-Version: 1.0 To: Luigi Rizzo Subject: Re: API explosion (Re: [RFC/RFT] calloutng) References: <50CF88B9.6040004@FreeBSD.org> <20121218173643.GA94266@onelab2.iet.unipi.it> <50D0B00D.8090002@FreeBSD.org> In-Reply-To: <50D0B00D.8090002@FreeBSD.org> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: Davide Italiano , 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: Tue, 18 Dec 2012 21:46:26 -0000 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