linux-kernel - Re: [PATCH 00/17] timers: Complete the timer

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87ikn6sibi.ffs@tglx>
Date: Mon, 14 Apr 2025 20:34:41 +0200
From: Thomas Gleixner <tglx@...utronix.de>
To: Ingo Molnar <mingo@...nel.org>, linux-kernel@...r.kernel.org
Cc: Frederic Weisbecker <frederic@...nel.org>, "H . Peter Anvin"
 <hpa@...or.com>, Linus Torvalds <torvalds@...ux-foundation.org>, Peter
 Zijlstra <peterz@...radead.org>, Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH 00/17] timers: Complete the timer_*() API renames

On Mon, Apr 14 2025 at 12:35, Ingo Molnar wrote:
> * Ingo Molnar <mingo@...nel.org> wrote:
>>        add_timer_global()                  =>          timer_add_global()
>>        add_timer_local()                   =>          timer_add_local()
>>        from_timer()                        =>          timer_container_of()
>>        mod_timer_pending()                 =>          timer_mod_pending()
>>        timer_delete()              ... [unchanged] ... timer_delete()
>>        timer_reduce()              ... [unchanged] ... timer_reduce()
>>        timer_shutdown()            ... [unchanged] ... timer_shutdown()
>>        timer_shutdown_sync()       ... [unchanged] ... timer_shutdown_sync()
>>        try_to_del_timer_sync()             =>          timer_delete_sync_try()
>>        add_timer()                         =>          timer_add()
>>        add_timer_on()                      =>          timer_add_on()
>>        mod_timer()                         =>          timer_mod()
>
> BTW., my suggestion would be to maybe change this to timer_modify(), 
> because timer_mod() reads a bit weirdly.

Can you make your mind up _before_ spamming people with half baked
changes done in a hurry? :)

While I appreciate proper namespace prefixes, this series is just a
mechanical conversion without any additional value. 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.

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.

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.

Thanks,

        tglx