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
