Re: [PATCH v1] [semaphore] Removed redundant code from semaphore's down family of function

[Satendra Singh Thakur]

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) <peterz@xxxxxxxxxxxxx>
> ---
> 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