netdev - Re: [PATCH 1/2] timer: introduce upper bound timers

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 for Android: free password hash cracker in your pocket

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

Date:   Thu, 24 Mar 2022 13:28:43 +0100
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Artem Savkov <asavkov@...hat.com>, jpoimboe@...hat.com,
        netdev@...r.kernel.org
Cc:     davem@...emloft.net, yoshfuji@...ux-ipv6.org, dsahern@...nel.org,
        linux-kernel@...r.kernel.org, Artem Savkov <asavkov@...hat.com>,
        Anna-Maria Gleixner <anna-maria@...utronix.de>
Subject: Re: [PATCH 1/2] timer: introduce upper bound timers

Artem,

On Wed, Mar 23 2022 at 12:16, Artem Savkov wrote:
> Add TIMER_UPPER_BOUND flag which allows creation of timers that would
> expire at most at specified time or earlier.
>
> This was previously discussed here:
> https://lore.kernel.org/all/20210302001054.4qgrvnkltvkgikzr@treble/T/#u

please add the context to the changelog. A link is only supplemental
information and does not replace content.

>  static inline unsigned calc_index(unsigned long expires, unsigned lvl,
> -				  unsigned long *bucket_expiry)
> +				  unsigned long *bucket_expiry, bool upper_bound)
>  {
>  
>  	/*
> @@ -501,34 +501,39 @@ static inline unsigned calc_index(unsigned long expires, unsigned lvl,
>  	 * - Truncation of the expiry time in the outer wheel levels
>  	 *
>  	 * Round up with level granularity to prevent this.
> +	 * Do not perform round up in case of upper bound timer.
>  	 */
> -	expires = (expires + LVL_GRAN(lvl)) >> LVL_SHIFT(lvl);
> +	if (upper_bound)
> +		expires = expires >> LVL_SHIFT(lvl);
> +	else
> +		expires = (expires + LVL_GRAN(lvl)) >> LVL_SHIFT(lvl);

While this "works", I fundamentally hate this because it adds an extra
conditional into the common case. That affects every user of the timer
wheel. We went great length to optimize that code and I'm not really enthused
to sacrifice that just because of _one_ use case.

The resulting text bloat is +25% on x86/64 and a quick test case shows
that this is well measurable overhead. The first ones who will complain
about that are going to be - drumroll - the network people.

There must be smarter ways to solve that. Let me think about it.

Thanks,

        tglx