[PATCH REPOST v3 0/2] signal: Avoid preempt_disable() in ptrace_stop() on PREEMPT_RT

[PATCH REPOST v3 0/2] signal: Avoid preempt_disable() in ptrace_stop() on PREEMPT_RT

[Sebastian Andrzej Siewior]

this mini series updates the comment to properly explain why the
preempt-disable section has been added and then disables this on
PREEMPT_RT explaining why it can not be done.

v3…v1
  - Include the update comment of the second patch which was posted as
    v2 within the orignal thread.
  - Add Oleg's Acks.

This is the repost of
	https://lore.kernel.org/20230615105627.1311437-1-bigeasy@linutronix.de

Previous post:
	https://lore.kernel.org/20230606085524.2049961-1-bigeasy@linutronix.de

[PATCH 1/2] signal: Add proper comment about the preempt-disable in ptrace_stop().

[Sebastian Andrzej Siewior]

Commit 53da1d9456fe7 ("fix ptrace slowness") added a preempt-disable section
between read_unlock() and the following schedule() invocation without
explaining why it is needed.

Replace the comment with an explanation why this is needed. Clarify that
it is needed for correctness but for performance reasons.

Acked-by: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 kernel/signal.c |   17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -2318,10 +2318,21 @@ static int ptrace_stop(int exit_code, in
 		do_notify_parent_cldstop(current, false, why);
 
 	/*
-	 * Don't want to allow preemption here, because
-	 * sys_ptrace() needs this task to be inactive.
+	 * The previous do_notify_parent_cldstop() invocation woke ptracer.
+	 * One a PREEMPTION kernel this can result in preemption requirement
+	 * which will be fulfilled after read_unlock() and the ptracer will be
+	 * put on the CPU.
+	 * The ptracer is in wait_task_inactive(, __TASK_TRACED) waiting for
+	 * this task wait in schedule(). If this task gets preempted then it
+	 * remains enqueued on the runqueue. The ptracer will observe this and
+	 * then sleep for a delay of one HZ tick. In the meantime this task
+	 * gets scheduled, enters schedule() and will wait for the ptracer.
 	 *
-	 * XXX: implement read_unlock_no_resched().
+	 * This preemption point is not bad from correctness point of view but
+	 * extends the runtime by one HZ tick time due to the ptracer's sleep.
+	 * The preempt-disable section ensures that there will be no preemption
+	 * between unlock and schedule() and so improving the performance since
+	 * the ptracer has no reason to sleep.
 	 */
 	preempt_disable();
 	read_unlock(&tasklist_lock);

[tip: core/core] signal: Add a proper comment about preempt_disable() in ptrace_stop()

[tip-bot2 for Sebastian Andrzej Siewior]

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

Commit-ID:     a20d6f63dbfc176697886d7709312ad0a795648e
Gitweb:        https://git.kernel.org/tip/a20d6f63dbfc176697886d7709312ad0a795648e
Author:        Sebastian Andrzej Siewior <bigeasy@linutronix.de>
AuthorDate:    Thu, 03 Aug 2023 12:09:31 +02:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Tue, 19 Sep 2023 22:08:29 +02:00

signal: Add a proper comment about preempt_disable() in ptrace_stop()

Commit 53da1d9456fe7 ("fix ptrace slowness") added a preempt-disable section
between read_unlock() and the following schedule() invocation without
explaining why it is needed.

Replace the existing contentless comment with a proper explanation to
clarify that it is not needed for correctness but for performance reasons.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Oleg Nesterov <oleg@redhat.com>
Link: https://lore.kernel.org/r/20230803100932.325870-2-bigeasy@linutronix.de

---
 kernel/signal.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index 0901901..3035beb 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -2329,10 +2329,22 @@ static int ptrace_stop(int exit_code, int why, unsigned long message,
 		do_notify_parent_cldstop(current, false, why);
 
 	/*
-	 * Don't want to allow preemption here, because
-	 * sys_ptrace() needs this task to be inactive.
+	 * The previous do_notify_parent_cldstop() invocation woke ptracer.
+	 * One a PREEMPTION kernel this can result in preemption requirement
+	 * which will be fulfilled after read_unlock() and the ptracer will be
+	 * put on the CPU.
+	 * The ptracer is in wait_task_inactive(, __TASK_TRACED) waiting for
+	 * this task wait in schedule(). If this task gets preempted then it
+	 * remains enqueued on the runqueue. The ptracer will observe this and
+	 * then sleep for a delay of one HZ tick. In the meantime this task
+	 * gets scheduled, enters schedule() and will wait for the ptracer.
 	 *
-	 * XXX: implement read_unlock_no_resched().
+	 * This preemption point is not bad from a correctness point of
+	 * view but extends the runtime by one HZ tick time due to the
+	 * ptracer's sleep.  The preempt-disable section ensures that there
+	 * will be no preemption between unlock and schedule() and so
+	 * improving the performance since the ptracer will observe that
+	 * the tracee is scheduled out once it gets on the CPU.
 	 */
 	preempt_disable();
 	read_unlock(&tasklist_lock);

Re: [tip: core/core] signal: Add a proper comment about preempt_disable() in ptrace_stop()

[Ingo Molnar]

* tip-bot2 for Sebastian Andrzej Siewior <tip-bot2@linutronix.de> wrote:

> The following commit has been merged into the core/core branch of tip:
> 
> Commit-ID:     a20d6f63dbfc176697886d7709312ad0a795648e
> Gitweb:        https://git.kernel.org/tip/a20d6f63dbfc176697886d7709312ad0a795648e
> Author:        Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> AuthorDate:    Thu, 03 Aug 2023 12:09:31 +02:00
> Committer:     Thomas Gleixner <tglx@linutronix.de>
> CommitterDate: Tue, 19 Sep 2023 22:08:29 +02:00
> 
> signal: Add a proper comment about preempt_disable() in ptrace_stop()
> 
> Commit 53da1d9456fe7 ("fix ptrace slowness") added a preempt-disable section
> between read_unlock() and the following schedule() invocation without
> explaining why it is needed.
> 
> Replace the existing contentless comment with a proper explanation to
> clarify that it is not needed for correctness but for performance reasons.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Acked-by: Oleg Nesterov <oleg@redhat.com>
> Link: https://lore.kernel.org/r/20230803100932.325870-2-bigeasy@linutronix.de
> 
> ---
>  kernel/signal.c | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/signal.c b/kernel/signal.c
> index 0901901..3035beb 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -2329,10 +2329,22 @@ static int ptrace_stop(int exit_code, int why, unsigned long message,
>  		do_notify_parent_cldstop(current, false, why);
>  

Minor speling nits:

>  	/*
> -	 * Don't want to allow preemption here, because
> -	 * sys_ptrace() needs this task to be inactive.
> +	 * The previous do_notify_parent_cldstop() invocation woke ptracer.
> +	 * One a PREEMPTION kernel this can result in preemption requirement

s/One
 /On

> +	 * which will be fulfilled after read_unlock() and the ptracer will be
> +	 * put on the CPU.
> +	 * The ptracer is in wait_task_inactive(, __TASK_TRACED) waiting for
> +	 * this task wait in schedule(). If this task gets preempted then it
> +	 * remains enqueued on the runqueue. The ptracer will observe this and
> +	 * then sleep for a delay of one HZ tick. In the meantime this task
> +	 * gets scheduled, enters schedule() and will wait for the ptracer.
>  	 *
> -	 * XXX: implement read_unlock_no_resched().
> +	 * This preemption point is not bad from a correctness point of
> +	 * view but extends the runtime by one HZ tick time due to the
> +	 * ptracer's sleep.  The preempt-disable section ensures that there
> +	 * will be no preemption between unlock and schedule() and so
> +	 * improving the performance since the ptracer will observe that

s/improving the performance
 /improving performance

> +	 * the tracee is scheduled out once it gets on the CPU.
>  	 */
>  	preempt_disable();
>  	read_unlock(&tasklist_lock);

Thanks,

	Ingo

[PATCH] signal: Correct spelling typos in ptrace_stop().

[Sebastian Andrzej Siewior]

Ingo spotted typos in the recently added comment and suggested
corrections.

Use 'On' instead of 'One' and remove one superfluous 'the' in a
sentence.

Suggested-by: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 kernel/signal.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index f2a5578326ade..384b77ad1bc15 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -2330,7 +2330,7 @@ static int ptrace_stop(int exit_code, int why, unsigned long message,
 
 	/*
 	 * The previous do_notify_parent_cldstop() invocation woke ptracer.
-	 * One a PREEMPTION kernel this can result in preemption requirement
+	 * On a PREEMPTION kernel this can result in preemption requirement
 	 * which will be fulfilled after read_unlock() and the ptracer will be
 	 * put on the CPU.
 	 * The ptracer is in wait_task_inactive(, __TASK_TRACED) waiting for
@@ -2343,7 +2343,7 @@ static int ptrace_stop(int exit_code, int why, unsigned long message,
 	 * view but extends the runtime by one HZ tick time due to the
 	 * ptracer's sleep.  The preempt-disable section ensures that there
 	 * will be no preemption between unlock and schedule() and so
-	 * improving the performance since the ptracer will observe that
+	 * improving performance since the ptracer will observe that
 	 * the tracee is scheduled out once it gets on the CPU.
 	 *
 	 * On PREEMPT_RT locking tasklist_lock does not disable preemption.
-- 
2.40.1

[PATCH 2/2] signal: Don't disable preemption in ptrace_stop() on PREEMPT_RT.

[Sebastian Andrzej Siewior]

On PREEMPT_RT keeping preemption disabled during the invocation of
cgroup_enter_frozen() is a problem because the function acquires css_set_lock
which is a sleeping lock on PREEMPT_RT and must not be acquired with disabled
preemption.
The preempt-disabled section is only for performance optimisation
reasons and can be avoided.

Extend the comment and don't disable preemption before scheduling on
PREEMPT_RT.

Acked-by: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 kernel/signal.c |   13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -2333,11 +2333,20 @@ static int ptrace_stop(int exit_code, in
 	 * The preempt-disable section ensures that there will be no preemption
 	 * between unlock and schedule() and so improving the performance since
 	 * the ptracer has no reason to sleep.
+	 *
+	 * On PREEMPT_RT locking tasklist_lock does not disable preemption.
+	 * Therefore the task can be preempted (after
+	 * do_notify_parent_cldstop()) before unlocking tasklist_lock so there
+	 * is no benefit in doing this. The optimisation is harmful on
+	 * PEEMPT_RT because the spinlock_t (in cgroup_enter_frozen()) must not
+	 * be acquired with disabled preemption.
 	 */
-	preempt_disable();
+	if (!IS_ENABLED(CONFIG_PREEMPT_RT))
+		preempt_disable();
 	read_unlock(&tasklist_lock);
 	cgroup_enter_frozen();
-	preempt_enable_no_resched();
+	if (!IS_ENABLED(CONFIG_PREEMPT_RT))
+		preempt_enable_no_resched();
 	schedule();
 	cgroup_leave_frozen(true);
 

[tip: core/core] signal: Don't disable preemption in ptrace_stop() on PREEMPT_RT

[tip-bot2 for Sebastian Andrzej Siewior]

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

Commit-ID:     1aabbc532413ced293952f8e149ad0a607d6e470
Gitweb:        https://git.kernel.org/tip/1aabbc532413ced293952f8e149ad0a607d6e470
Author:        Sebastian Andrzej Siewior <bigeasy@linutronix.de>
AuthorDate:    Thu, 03 Aug 2023 12:09:32 +02:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Tue, 19 Sep 2023 22:08:29 +02:00

signal: Don't disable preemption in ptrace_stop() on PREEMPT_RT

On PREEMPT_RT keeping preemption disabled during the invocation of
cgroup_enter_frozen() is a problem because the function acquires
css_set_lock which is a sleeping lock on PREEMPT_RT and must not be
acquired with disabled preemption.

The preempt-disabled section is only for performance optimisation reasons
and can be avoided.

Extend the comment and don't disable preemption before scheduling on
PREEMPT_RT.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Oleg Nesterov <oleg@redhat.com>
Link: https://lore.kernel.org/r/20230803100932.325870-3-bigeasy@linutronix.de

---
 kernel/signal.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index 3035beb..f2a5578 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -2345,11 +2345,22 @@ static int ptrace_stop(int exit_code, int why, unsigned long message,
 	 * will be no preemption between unlock and schedule() and so
 	 * improving the performance since the ptracer will observe that
 	 * the tracee is scheduled out once it gets on the CPU.
+	 *
+	 * On PREEMPT_RT locking tasklist_lock does not disable preemption.
+	 * Therefore the task can be preempted after do_notify_parent_cldstop()
+	 * before unlocking tasklist_lock so there is no benefit in doing this.
+	 *
+	 * In fact disabling preemption is harmful on PREEMPT_RT because
+	 * the spinlock_t in cgroup_enter_frozen() must not be acquired
+	 * with preemption disabled due to the 'sleeping' spinlock
+	 * substitution of RT.
 	 */
-	preempt_disable();
+	if (!IS_ENABLED(CONFIG_PREEMPT_RT))
+		preempt_disable();
 	read_unlock(&tasklist_lock);
 	cgroup_enter_frozen();
-	preempt_enable_no_resched();
+	if (!IS_ENABLED(CONFIG_PREEMPT_RT))
+		preempt_enable_no_resched();
 	schedule();
 	cgroup_leave_frozen(true);
 

Re: [PATCH REPOST v3 0/2] signal: Avoid preempt_disable() in ptrace_stop() on PREEMPT_RT

[Sebastian Andrzej Siewior]

On 2023-08-03 12:09:30 [+0200], To linux-kernel@vger.kernel.org wrote:
> this mini series updates the comment to properly explain why the
> preempt-disable section has been added and then disables this on
> PREEMPT_RT explaining why it can not be done.

ping …

> v3…v1
>   - Include the update comment of the second patch which was posted as
>     v2 within the orignal thread.
>   - Add Oleg's Acks.
> 
> This is the repost of
> 	https://lore.kernel.org/20230615105627.1311437-1-bigeasy@linutronix.de
> 
> Previous post:
> 	https://lore.kernel.org/20230606085524.2049961-1-bigeasy@linutronix.de

Sebastian