All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/1] timer: Improve the comment describing schedule_timeout()
@ 2020-01-17 22:59 Alexander Popov
  2020-02-07 21:44 ` Alexander Popov
                   ` (2 more replies)
  0 siblings, 3 replies; 4+ messages in thread
From: Alexander Popov @ 2020-01-17 22:59 UTC (permalink / raw)
  To: Linus Torvalds, John Stultz, Thomas Gleixner, Stephen Boyd,
	Ingo Molnar, Peter Zijlstra, Juri Lelli, Vincent Guittot,
	Dietmar Eggemann, Steven Rostedt, Ben Segall, Mel Gorman,
	Alexander Popov, linux-kernel
  Cc: notify

When we were preparing the patch 6dcd5d7a7a29c1e, we made a mistake noticed
by Linus: schedule_timeout() was called without setting the task state to
anything particular. It calls the scheduler, but doesn't delay anything,
because the task stays runnable. That happens because sched_submit_work()
does nothing for tasks in TASK_RUNNING state.

That turned out to be the intended behavior. Adding a WARN() is not useful.
Let's improve the comment about schedule_timeout() and describe that
more explicitly.

Signed-off-by: Alexander Popov <alex.popov@linux.com>
---
 kernel/time/timer.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/kernel/time/timer.c b/kernel/time/timer.c
index 4820823515e9..cb34fac9d9f7 100644
--- a/kernel/time/timer.c
+++ b/kernel/time/timer.c
@@ -1828,21 +1828,23 @@ static void process_timeout(struct timer_list *t)
  * schedule_timeout - sleep until timeout
  * @timeout: timeout value in jiffies
  *
- * Make the current task sleep until @timeout jiffies have
- * elapsed. The routine will return immediately unless
- * the current task state has been set (see set_current_state()).
+ * Make the current task sleep until @timeout jiffies have elapsed.
+ * The function behavior depends on the current task state
+ * (see also set_current_state() description):
  *
- * You can set the task state as follows -
+ * %TASK_RUNNING - the scheduler is called, but the task does not sleep
+ * at all. That happens because sched_submit_work() does nothing for
+ * tasks in %TASK_RUNNING state.
  *
  * %TASK_UNINTERRUPTIBLE - at least @timeout jiffies are guaranteed to
  * pass before the routine returns unless the current task is explicitly
- * woken up, (e.g. by wake_up_process())".
+ * woken up, (e.g. by wake_up_process()).
  *
  * %TASK_INTERRUPTIBLE - the routine may return early if a signal is
  * delivered to the current task or the current task is explicitly woken
  * up.
  *
- * The current task state is guaranteed to be TASK_RUNNING when this
+ * The current task state is guaranteed to be %TASK_RUNNING when this
  * routine returns.
  *
  * Specifying a @timeout value of %MAX_SCHEDULE_TIMEOUT will schedule
@@ -1850,7 +1852,7 @@ static void process_timeout(struct timer_list *t)
  * value will be %MAX_SCHEDULE_TIMEOUT.
  *
  * Returns 0 when the timer has expired otherwise the remaining time in
- * jiffies will be returned.  In all cases the return value is guaranteed
+ * jiffies will be returned. In all cases the return value is guaranteed
  * to be non-negative.
  */
 signed long __sched schedule_timeout(signed long timeout)
-- 
2.24.1


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH 1/1] timer: Improve the comment describing schedule_timeout()
  2020-01-17 22:59 [PATCH 1/1] timer: Improve the comment describing schedule_timeout() Alexander Popov
@ 2020-02-07 21:44 ` Alexander Popov
  2020-02-17 15:40 ` [tip: timers/core] " tip-bot2 for Alexander Popov
  2020-02-17 19:18 ` tip-bot2 for Alexander Popov
  2 siblings, 0 replies; 4+ messages in thread
From: Alexander Popov @ 2020-02-07 21:44 UTC (permalink / raw)
  To: Linus Torvalds, John Stultz, Thomas Gleixner, Stephen Boyd,
	Ingo Molnar, Peter Zijlstra, Juri Lelli, Vincent Guittot,
	Dietmar Eggemann, Steven Rostedt, Ben Segall, Mel Gorman,
	linux-kernel
  Cc: notify

Hello everyone!
Could I have any feedback for this patch?
Thanks!

On 18.01.2020 01:59, Alexander Popov wrote:
> When we were preparing the patch 6dcd5d7a7a29c1e, we made a mistake noticed
> by Linus: schedule_timeout() was called without setting the task state to
> anything particular. It calls the scheduler, but doesn't delay anything,
> because the task stays runnable. That happens because sched_submit_work()
> does nothing for tasks in TASK_RUNNING state.
> 
> That turned out to be the intended behavior. Adding a WARN() is not useful.
> Let's improve the comment about schedule_timeout() and describe that
> more explicitly.
> 
> Signed-off-by: Alexander Popov <alex.popov@linux.com>
> ---
>  kernel/time/timer.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/kernel/time/timer.c b/kernel/time/timer.c
> index 4820823515e9..cb34fac9d9f7 100644
> --- a/kernel/time/timer.c
> +++ b/kernel/time/timer.c
> @@ -1828,21 +1828,23 @@ static void process_timeout(struct timer_list *t)
>   * schedule_timeout - sleep until timeout
>   * @timeout: timeout value in jiffies
>   *
> - * Make the current task sleep until @timeout jiffies have
> - * elapsed. The routine will return immediately unless
> - * the current task state has been set (see set_current_state()).
> + * Make the current task sleep until @timeout jiffies have elapsed.
> + * The function behavior depends on the current task state
> + * (see also set_current_state() description):
>   *
> - * You can set the task state as follows -
> + * %TASK_RUNNING - the scheduler is called, but the task does not sleep
> + * at all. That happens because sched_submit_work() does nothing for
> + * tasks in %TASK_RUNNING state.
>   *
>   * %TASK_UNINTERRUPTIBLE - at least @timeout jiffies are guaranteed to
>   * pass before the routine returns unless the current task is explicitly
> - * woken up, (e.g. by wake_up_process())".
> + * woken up, (e.g. by wake_up_process()).
>   *
>   * %TASK_INTERRUPTIBLE - the routine may return early if a signal is
>   * delivered to the current task or the current task is explicitly woken
>   * up.
>   *
> - * The current task state is guaranteed to be TASK_RUNNING when this
> + * The current task state is guaranteed to be %TASK_RUNNING when this
>   * routine returns.
>   *
>   * Specifying a @timeout value of %MAX_SCHEDULE_TIMEOUT will schedule
> @@ -1850,7 +1852,7 @@ static void process_timeout(struct timer_list *t)
>   * value will be %MAX_SCHEDULE_TIMEOUT.
>   *
>   * Returns 0 when the timer has expired otherwise the remaining time in
> - * jiffies will be returned.  In all cases the return value is guaranteed
> + * jiffies will be returned. In all cases the return value is guaranteed
>   * to be non-negative.
>   */
>  signed long __sched schedule_timeout(signed long timeout)
> 


^ permalink raw reply	[flat|nested] 4+ messages in thread

* [tip: timers/core] timer: Improve the comment describing schedule_timeout()
  2020-01-17 22:59 [PATCH 1/1] timer: Improve the comment describing schedule_timeout() Alexander Popov
  2020-02-07 21:44 ` Alexander Popov
@ 2020-02-17 15:40 ` tip-bot2 for Alexander Popov
  2020-02-17 19:18 ` tip-bot2 for Alexander Popov
  2 siblings, 0 replies; 4+ messages in thread
From: tip-bot2 for Alexander Popov @ 2020-02-17 15:40 UTC (permalink / raw)
  To: linux-tip-commits; +Cc: Alexander Popov, Thomas Gleixner, x86, LKML

The following commit has been merged into the timers/core branch of tip:

Commit-ID:     1b8020618b8922b9128b3015224221d0f869b471
Gitweb:        https://git.kernel.org/tip/1b8020618b8922b9128b3015224221d0f869b471
Author:        Alexander Popov <alex.popov@linux.com>
AuthorDate:    Sat, 18 Jan 2020 01:59:00 +03:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Mon, 17 Feb 2020 16:36:05 +01:00

timer: Improve the comment describing schedule_timeout()

When working commit 6dcd5d7a7a29c1e, a mistake was noticed by Linus:
schedule_timeout() was called without setting the task state to anything
particular.

It calls the scheduler, but doesn't delay anything, because the task stays
runnable. That happens because sched_submit_work() does nothing for tasks
in TASK_RUNNING state.

That turned out to be the intended behavior. Adding a WARN() is not useful
as the task could be woken up right after setting the state and before
reaching schedule_timeout().

Improve the comment about schedule_timeout() and describe that more
explicitly.

Signed-off-by: Alexander Popov <alex.popov@linux.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Link: https://lkml.kernel.org/r/20200117225900.16340-1-alex.popov@linux.com

---
 kernel/time/timer.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/kernel/time/timer.c b/kernel/time/timer.c
index 4820823..cb34fac 100644
--- a/kernel/time/timer.c
+++ b/kernel/time/timer.c
@@ -1828,21 +1828,23 @@ static void process_timeout(struct timer_list *t)
  * schedule_timeout - sleep until timeout
  * @timeout: timeout value in jiffies
  *
- * Make the current task sleep until @timeout jiffies have
- * elapsed. The routine will return immediately unless
- * the current task state has been set (see set_current_state()).
+ * Make the current task sleep until @timeout jiffies have elapsed.
+ * The function behavior depends on the current task state
+ * (see also set_current_state() description):
  *
- * You can set the task state as follows -
+ * %TASK_RUNNING - the scheduler is called, but the task does not sleep
+ * at all. That happens because sched_submit_work() does nothing for
+ * tasks in %TASK_RUNNING state.
  *
  * %TASK_UNINTERRUPTIBLE - at least @timeout jiffies are guaranteed to
  * pass before the routine returns unless the current task is explicitly
- * woken up, (e.g. by wake_up_process())".
+ * woken up, (e.g. by wake_up_process()).
  *
  * %TASK_INTERRUPTIBLE - the routine may return early if a signal is
  * delivered to the current task or the current task is explicitly woken
  * up.
  *
- * The current task state is guaranteed to be TASK_RUNNING when this
+ * The current task state is guaranteed to be %TASK_RUNNING when this
  * routine returns.
  *
  * Specifying a @timeout value of %MAX_SCHEDULE_TIMEOUT will schedule
@@ -1850,7 +1852,7 @@ static void process_timeout(struct timer_list *t)
  * value will be %MAX_SCHEDULE_TIMEOUT.
  *
  * Returns 0 when the timer has expired otherwise the remaining time in
- * jiffies will be returned.  In all cases the return value is guaranteed
+ * jiffies will be returned. In all cases the return value is guaranteed
  * to be non-negative.
  */
 signed long __sched schedule_timeout(signed long timeout)

^ permalink raw reply related	[flat|nested] 4+ messages in thread

* [tip: timers/core] timer: Improve the comment describing schedule_timeout()
  2020-01-17 22:59 [PATCH 1/1] timer: Improve the comment describing schedule_timeout() Alexander Popov
  2020-02-07 21:44 ` Alexander Popov
  2020-02-17 15:40 ` [tip: timers/core] " tip-bot2 for Alexander Popov
@ 2020-02-17 19:18 ` tip-bot2 for Alexander Popov
  2 siblings, 0 replies; 4+ messages in thread
From: tip-bot2 for Alexander Popov @ 2020-02-17 19:18 UTC (permalink / raw)
  To: linux-tip-commits; +Cc: Alexander Popov, Thomas Gleixner, x86, LKML

The following commit has been merged into the timers/core branch of tip:

Commit-ID:     6e317c32fd39a13e4854a27958d5e35d15d196be
Gitweb:        https://git.kernel.org/tip/6e317c32fd39a13e4854a27958d5e35d15d196be
Author:        Alexander Popov <alex.popov@linux.com>
AuthorDate:    Sat, 18 Jan 2020 01:59:00 +03:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Mon, 17 Feb 2020 20:12:19 +01:00

timer: Improve the comment describing schedule_timeout()

When working commit 6dcd5d7a7a29c1e, a mistake was noticed by Linus:
schedule_timeout() was called without setting the task state to anything
particular.

It calls the scheduler, but doesn't delay anything, because the task stays
runnable. That happens because sched_submit_work() does nothing for tasks
in TASK_RUNNING state.

That turned out to be the intended behavior. Adding a WARN() is not useful
as the task could be woken up right after setting the state and before
reaching schedule_timeout().

Improve the comment about schedule_timeout() and describe that more
explicitly.

Signed-off-by: Alexander Popov <alex.popov@linux.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Link: https://lkml.kernel.org/r/20200117225900.16340-1-alex.popov@linux.com


---
 kernel/time/timer.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/kernel/time/timer.c b/kernel/time/timer.c
index 4820823..cb34fac 100644
--- a/kernel/time/timer.c
+++ b/kernel/time/timer.c
@@ -1828,21 +1828,23 @@ static void process_timeout(struct timer_list *t)
  * schedule_timeout - sleep until timeout
  * @timeout: timeout value in jiffies
  *
- * Make the current task sleep until @timeout jiffies have
- * elapsed. The routine will return immediately unless
- * the current task state has been set (see set_current_state()).
+ * Make the current task sleep until @timeout jiffies have elapsed.
+ * The function behavior depends on the current task state
+ * (see also set_current_state() description):
  *
- * You can set the task state as follows -
+ * %TASK_RUNNING - the scheduler is called, but the task does not sleep
+ * at all. That happens because sched_submit_work() does nothing for
+ * tasks in %TASK_RUNNING state.
  *
  * %TASK_UNINTERRUPTIBLE - at least @timeout jiffies are guaranteed to
  * pass before the routine returns unless the current task is explicitly
- * woken up, (e.g. by wake_up_process())".
+ * woken up, (e.g. by wake_up_process()).
  *
  * %TASK_INTERRUPTIBLE - the routine may return early if a signal is
  * delivered to the current task or the current task is explicitly woken
  * up.
  *
- * The current task state is guaranteed to be TASK_RUNNING when this
+ * The current task state is guaranteed to be %TASK_RUNNING when this
  * routine returns.
  *
  * Specifying a @timeout value of %MAX_SCHEDULE_TIMEOUT will schedule
@@ -1850,7 +1852,7 @@ static void process_timeout(struct timer_list *t)
  * value will be %MAX_SCHEDULE_TIMEOUT.
  *
  * Returns 0 when the timer has expired otherwise the remaining time in
- * jiffies will be returned.  In all cases the return value is guaranteed
+ * jiffies will be returned. In all cases the return value is guaranteed
  * to be non-negative.
  */
 signed long __sched schedule_timeout(signed long timeout)

^ permalink raw reply related	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2020-02-17 19:19 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-17 22:59 [PATCH 1/1] timer: Improve the comment describing schedule_timeout() Alexander Popov
2020-02-07 21:44 ` Alexander Popov
2020-02-17 15:40 ` [tip: timers/core] " tip-bot2 for Alexander Popov
2020-02-17 19:18 ` tip-bot2 for Alexander Popov

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.