linux-kernel - Re: [RFC PATCH 04/11] Add a function to start/reduce a 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 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]

Date:   Fri, 20 Oct 2017 14:20:27 +0200 (CEST)
From:   Thomas Gleixner <tglx@...utronix.de>
To:     David Howells <dhowells@...hat.com>
cc:     linux-afs@...ts.infradead.org, linux-fsdevel@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH 04/11] Add a function to start/reduce a timer

On Fri, 1 Sep 2017, David Howells wrote:

> Add a function, similar to mod_timer(), that will start a timer it isn't

s/it /if it /

> running and will modify it if it is running and has an expiry time longer
> than the new time.  If the timer is running with an expiry time that's the
> same or sooner, no change is made.
>
> The function looks like:
>
>       int reduce_timer(struct timer_list *timer, unsigned long expires);

Well, yes. But what's the purpose of this function? You explain the what,
but not the why.

> +extern int reduce_timer(struct timer_list *timer, unsigned long expires);

For new timer functions we really should use the timer_xxxx()
convention. The historic naming convention is horrible.

Aside of that timer_reduce() is kinda ugly but I failed to come up with
something reasonable as well.

>  static inline int
> -__mod_timer(struct timer_list *timer, unsigned long expires, bool pending_only)
> +__mod_timer(struct timer_list *timer, unsigned long expires, unsigned int options)
>  {
>  	struct timer_base *base, *new_base;
>  	unsigned int idx = UINT_MAX;
> @@ -938,8 +941,13 @@ __mod_timer(struct timer_list *timer, unsigned long expires, bool pending_only)
>  	 * same array bucket then just return:
>  	 */
>  	if (timer_pending(timer)) {
> -		if (timer->expires == expires)
> -			return 1;
> +		if (options & MOD_TIMER_REDUCE) {
> +			if (time_before_eq(timer->expires, expires))
> +				return 1;
> +		} else {
> +			if (timer->expires == expires)
> +				return 1;
> +		}

This hurts the common networking optimzation case. Please keep that check
first:

		if (timer->expires == expires)
			return 1;

		if ((options & MOD_TIMER_REDUCE) &&
		    time_before(timer->expires, expires))
		    	return 1;

Also please check whether it's more efficient code wise to have that option
thing or if an additional 'bool reduce' argument cerates better code.

Thanks,

	tglx