On Mon, 26 Aug 2019 16:25:57 +0200, Peter Zijlstra wrote:
>On Mon, Aug 26, 2019 at 04:14:36PM +0200, Peter Zijlstra wrote:
> > (XXX, we should probably move the schedule_timeout() thing into its own
> > patch)
>
> A better version here...
>
> ---
> Subject: sched,time: Allow better constprop/DCE for schedule_timeout()
>
> If timeout is constant and MAX_SCHEDULE_TIMEOUT, it would be nice to
> allow to optimize away everything timeout.
>
> Signed-off-by: Peter Zijlstra (Intel) <pet...@infradead.org>
> ---
> include/linux/sched.h | 13 ++++++++++++-
> kernel/time/timer.c | 52 ++++++++++++++++++++++++---------------------------
> 2 files changed, 36 insertions(+), 29 deletions(-)
>
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index f0edee94834a..6003e96bce52 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -214,7 +214,18 @@ extern void scheduler_tick(void);
>
> #define MAX_SCHEDULE_TIMEOUT LONG_MAX
>
> -extern long schedule_timeout(long timeout);
> +extern long __schedule_timeout(long timeout);
> +
> +static inline long schedule_timeout(long timeout)
> +{
> + if (__builtin_constant_p(timeout) && timeout == MAX_SCHEDULE_TIMEOUT) {
> + schedule();
> + return timeout;
> + }
> +
> + return __schedule_timeout(timeout);
> +}
> +
> extern long schedule_timeout_interruptible(long timeout);
> extern long schedule_timeout_killable(long timeout);
> extern long schedule_timeout_uninterruptible(long timeout);
> diff --git a/kernel/time/timer.c b/kernel/time/timer.c
> index 0e315a2e77ae..912ae56b96b8 100644
> --- a/kernel/time/timer.c
> +++ b/kernel/time/timer.c
> @@ -1851,38 +1851,34 @@ static void process_timeout(struct timer_list *t)
> * jiffies will be returned. In all cases the return value is guaranteed
> * to be non-negative.
> */
> -signed long __sched schedule_timeout(signed long timeout)
> +signed long __sched __schedule_timeout(signed long timeout)
> {
> struct process_timer timer;
> unsigned long expire;
>
> - switch (timeout)
> - {
> - case MAX_SCHEDULE_TIMEOUT:
> - /*
> - * These two special cases are useful to be comfortable
> - * in the caller. Nothing more. We could take
> - * MAX_SCHEDULE_TIMEOUT from one of the negative value
> - * but I' d like to return a valid offset (>=0) to allow
> - * the caller to do everything it want with the retval.
> - */
> + /*
> + * We could take MAX_SCHEDULE_TIMEOUT from one of the negative values,
> + * but I'd like to return a valid offset (>= 0) to allow the caller to
> + * do everything it wants with the retval.
> + */
> + if (timeout == MAX_SCHEDULE_TIMEOUT) {
> schedule();
> - goto out;
Hi Mr Peter,
I have a suggestion here:
The condition timeout == MAX_SCHEDULE_TIMEOUT is already handled in
schedule_timeout function and same conditon is handled again in
__schedule_timeout.
Currently, no other function calls __schedule_timeout except schedule_timeout.
Therefore, it seems this condition will never become true.
This conditon will only be true when __schedule_timeout is called directly from
kernel
with timeout = MAX_SCHEDULE_TIMEOUT. This case may be rare. Therefore, we can
modify it as
if (unlikely(timeout == MAX_SCHEDULE_TIMEOUT))
Please let me know your comments.
Thanks
Satendra
> - default:
> - /*
> - * Another bit of PARANOID. Note that the retval will be
> - * 0 since no piece of kernel is supposed to do a check
> - * for a negative retval of schedule_timeout() (since it
> - * should never happens anyway). You just have the printk()
> - * that will tell you if something is gone wrong and where.
> - */
> - if (timeout < 0) {
> - printk(KERN_ERR "schedule_timeout: wrong timeout "
> + return timeout;
> + }
> +
> + /*
> + * Another bit of PARANOID. Note that the retval will be 0 since no
> + * piece of kernel is supposed to do a check for a negative retval of
> + * schedule_timeout() (since it should never happens anyway). You just
> + * have the printk() that will tell you if something is gone wrong and
> + * where.
> + */
> + if (timeout < 0) {
> + printk(KERN_ERR "schedule_timeout: wrong timeout "
> "value %lx\n", timeout);
> - dump_stack();
> - current->state = TASK_RUNNING;
> - goto out;
> - }
> + dump_stack();
> + current->state = TASK_RUNNING;
> + goto out;
> }
>
> expire = timeout + jiffies;
> @@ -1898,10 +1894,10 @@ signed long __sched schedule_timeout(signed long
> timeout)
>
> timeout = expire - jiffies;
>
> - out:
> +out:
> return timeout < 0 ? 0 : timeout;
> }
> -EXPORT_SYMBOL(schedule_timeout);
> +EXPORT_SYMBOL(__schedule_timeout);
>
> /*
> * We can use __set_current_state() here because schedule_timeout() calls
