Messages in this thread Patch in this message | | | From | Satendra Singh Thakur <> | Subject | Re: [PATCH v1] [semaphore] Removed redundant code from semaphore's down family of function | Date | Fri, 30 Aug 2019 16:10:10 +0530 |
| |
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@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
| |