Re: [PATCH 00/17] timers: Complete the timer_*() API renames

[Ingo Molnar]

* Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote:

> While I appreciate proper namespace prefixes, this series is just a
> mechanical conversion without any additional value.

Yes, intentionally so, as mentioned I didn't even rename any of the 
APIs beyond the trivial conversion, because I wanted to gather feedback 
first:

> > I didn't want to make bigger, discretionary changes in the first 
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > iteration, but I can easily propagate any such suggestions into 
    ^^^^^^^^^
> > future versions of this series.

    https://lore.kernel.org/all/Z_zk94RFo2bK85iJ@xxxxxxxxx/

Can you please acknowledge this clearly declared, intentionally limited 
scope of the -v1 series, instead of deriding it as 'mindless'? :-)

> [...] Some of the conversions like try_to_del_timer_sync() are 
> obviously fine and can't provide moar than a namespace consolidation.
> 
> But if you look at the actual functions and their usage all over the
> place then you can see that there is way more cleanup and consolidation
> potential especially for those functions which add or modify timers.
> 
> First of all the question is whether add() and mod() are really valuable
> distinctions. I'm not convinced at all. Back then, when we introduced
> hrtimers, we came to the conclusion that hrtimer_start() is sufficient.

I didn't want to mix namespace cleanups with functional cleanups, but 
sure, I'd be glad to do that too.

> But that aside there is a major cleanup potential for this stuff. The
> vast majority of add/mod_timer() sites uses:
> 
>     - Precomputed timeout values derived from a timeout provided in SE
>       units
> 
>     - Instant conversions of SE unit based timeouts to jiffies
> 
>           msec/usec/sec_to_jiffies()
> 
>     - All variants of HZ, HZ * N, HZ / N ....
> 
> This is lots of duplicated and copy and pasted code. So instead of
> blindly renaming things, we can be smarter and provide sensible
> functions:
> 
> mod_timer() takes an absolute expiry value, but most places use
> 
>     mod_timer(t, jiffies + $timeout);
> 
> So the obvious first step is to provide:
> 
>     timer_start_rel(t, $timeout);
> 
> which does the addition of jiffies under the hood.
> 
> And because $timeout is some of the above calculations, we can be
> smart and provide:
> 
>    timer_start_rel_secs(t, timeout_in_seconds);
>    timer_start_rel_msecs(t, timeout_in_milliseconds);
>    timer_start_rel_usecs(t, timeout_in_microseconds);
> 
> This all can be sensibly converted with coccinelle, which even can
> handle the cases where $timeout is calculated from HZ / N.
> 
> I have a pile of half finished coccinelle scripts somewhere, which do
> exactly such a conversion. I just ran out of time to play with that, as
> I ran into a few things which need more thoughts about proper
> interfaces. I'm happy to share them.

Please do!

See why I sent the simple approach first? :-)

> Converting the whole timer arming to use SE unit based timeouts makes 
> a lot of sense in general and also paves the way to boot-time 
> controlled HZ, which is something distros and others are asking for 
> since years (of course nobody wants to sit down and do the actual 
> work as usual...)
> 
> That said, I'm fine to convert the obvious things, like 
> try_timer_del*(), where there is no other consolidation value, but 
> for everything else we better sit down and think about proper 
> interfaces and large scale consolidation.

No arguments from me! :-)

Thanks,

	Ingo