Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Date:   Wed, 23 Mar 2022 11:40:26 -0700
From:   Josh Poimboeuf <jpoimboe@redhat.com>
To:     Artem Savkov <asavkov@redhat.com>
Cc:     tglx@linutronix.de, netdev@vger.kernel.org, davem@davemloft.net,
        yoshfuji@linux-ipv6.org, dsahern@kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/2] timer: introduce upper bound timers
Message-ID: <20220323184026.wkj55y55jbeumngs@treble>
References: <20220323111642.2517885-1-asavkov@redhat.com>
 <20220323111642.2517885-2-asavkov@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20220323111642.2517885-2-asavkov@redhat.com>
Precedence: bulk

On Wed, Mar 23, 2022 at 12:16:41PM +0100, 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
> 
> Suggested-by: Josh Poimboeuf <jpoimboe@redhat.com>
> Signed-off-by: Artem Savkov <asavkov@redhat.com>

Thanks Artem, this approach looks good to me.  Some kernel style nits...

The commit log could use some more background and motivation for the
change:

a) describe the current timer bound functionality and why it works for
   most cases

b) describe the type of situation where it doesn't work and why

c) describe the solution

> ---
>  include/linux/timer.h |  3 ++-
>  kernel/time/timer.c   | 36 ++++++++++++++++++++++--------------
>  2 files changed, 24 insertions(+), 15 deletions(-)
> 
> diff --git a/include/linux/timer.h b/include/linux/timer.h
> index fda13c9d1256..9b0963f49528 100644
> --- a/include/linux/timer.h
> +++ b/include/linux/timer.h
> @@ -67,7 +67,8 @@ struct timer_list {
>  #define TIMER_DEFERRABLE	0x00080000
>  #define TIMER_PINNED		0x00100000
>  #define TIMER_IRQSAFE		0x00200000
> -#define TIMER_INIT_FLAGS	(TIMER_DEFERRABLE | TIMER_PINNED | TIMER_IRQSAFE)
> +#define TIMER_UPPER_BOUND	0x00400000
> +#define TIMER_INIT_FLAGS	(TIMER_DEFERRABLE | TIMER_PINNED | TIMER_IRQSAFE | TIMER_UPPER_BOUND)
>  #define TIMER_ARRAYSHIFT	22
>  #define TIMER_ARRAYMASK		0xFFC00000

This new flag needs a comment above, where the other user flags are also
commented.

>  	}
>  	return idx;
>  }
> @@ -607,7 +613,8 @@ static void internal_add_timer(struct timer_base *base, struct timer_list *timer
>  	unsigned long bucket_expiry;
>  	unsigned int idx;
>  
> -	idx = calc_wheel_index(timer->expires, base->clk, &bucket_expiry);
> +	idx = calc_wheel_index(timer->expires, base->clk, &bucket_expiry,
> +			timer->flags & TIMER_UPPER_BOUND);

Here and elsewhere, to follow kernel convention, the 2nd line needs
spaces after the tabs to align it with the previous line.  That makes it
more obvious that the 2nd line is just another function argument.  Like:

	idx = calc_wheel_index(timer->expires, base->clk, &bucket_expiry,
			       timer->flags & TIMER_UPPER_BOUND);

-- 
Josh