mm: lru_cache_disable: replace work queue synchronization with synchronize_rcu

On systems that run FIFO:1 applications that busy loop on isolated CPUs, executing tasks on such CPUs under lower priority is undesired (since that will either hang the system, or cause longer interruption to the FIFO task due to execution of lower priority task with very small sched slices).

Commit d479960e44f27e0e52ba31b21740b703c538027c ("mm: disable LRU pagevec during the migration temporarily") relies on queueing work items on all online CPUs to ensure visibility of lru_disable_count.

However, its possible to use synchronize_rcu which will provide the same guarantees (see comment this patch modifies on lru_cache_disable).

Fixes:

[ 1873.243925] INFO: task kworker/u160:0:9 blocked for more than 622 seconds.
[ 1873.243927]       Tainted: G          I      --------- ---  5.14.0-31.rt21.31.el9.x86_64 #1
[ 1873.243929] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
[ 1873.243929] task:kworker/u160:0  state:D stack:    0 pid:    9 ppid:     2 flags:0x00004000
[ 1873.243932] Workqueue: cpuset_migrate_mm cpuset_migrate_mm_workfn
[ 1873.243936] Call Trace:
[ 1873.243938]  __schedule+0x21b/0x5b0
[ 1873.243941]  schedule+0x43/0xe0
[ 1873.243943]  schedule_timeout+0x14d/0x190
[ 1873.243946]  ? resched_curr+0x20/0xe0
[ 1873.243953]  ? __prepare_to_swait+0x4b/0x70
[ 1873.243958]  wait_for_completion+0x84/0xe0
[ 1873.243962]  __flush_work.isra.0+0x146/0x200
[ 1873.243966]  ? flush_workqueue_prep_pwqs+0x130/0x130
[ 1873.243971]  __lru_add_drain_all+0x158/0x1f0
[ 1873.243978]  do_migrate_pages+0x3d/0x2d0
[ 1873.243985]  ? pick_next_task_fair+0x39/0x3b0
[ 1873.243989]  ? put_prev_task_fair+0x1e/0x30
[ 1873.243992]  ? pick_next_task+0xb30/0xbd0
[ 1873.243995]  ? __tick_nohz_task_switch+0x1e/0x70
[ 1873.244000]  ? raw_spin_rq_unlock+0x18/0x60
[ 1873.244002]  ? finish_task_switch.isra.0+0xc1/0x2d0
[ 1873.244005]  ? __switch_to+0x12f/0x510
[ 1873.244013]  cpuset_migrate_mm_workfn+0x22/0x40
[ 1873.244016]  process_one_work+0x1e0/0x410
[ 1873.244019]  worker_thread+0x50/0x3b0
[ 1873.244022]  ? process_one_work+0x410/0x410
[ 1873.244024]  kthread+0x173/0x190
[ 1873.244027]  ? set_kthread_struct+0x40/0x40
[ 1873.244031]  ret_from_fork+0x1f/0x30

Signed-off-by Marcelo Tosatti <[email protected]>

On systems that run FIFO:1 applications that busy loop on isolated CPUs, executing tasks on such CPUs under lower priority is undesired (since that will either hang the system, or cause longer interruption to the FIFO task due to execution of lower priority task with very small sched slices).

Commit d479960e44f27e0e52ba31b21740b703c538027c ("mm: disable LRU pagevec during the migration temporarily") relies on queueing work items on all online CPUs to ensure visibility of lru_disable_count.

However, its possible to use synchronize_rcu which will provide the same guarantees (see comment this patch modifies on lru_cache_disable).

Fixes:

[ 1873.243925] INFO: task kworker/u160:0:9 blocked for more than 622 seconds.
[ 1873.243927]       Tainted: G          I      --------- ---  5.14.0-31.rt21.31.el9.x86_64 #1
[ 1873.243929] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
[ 1873.243929] task:kworker/u160:0  state:D stack:    0 pid:    9 ppid:     2 flags:0x00004000
[ 1873.243932] Workqueue: cpuset_migrate_mm cpuset_migrate_mm_workfn
[ 1873.243936] Call Trace:
[ 1873.243938]  __schedule+0x21b/0x5b0
[ 1873.243941]  schedule+0x43/0xe0
[ 1873.243943]  schedule_timeout+0x14d/0x190
[ 1873.243946]  ? resched_curr+0x20/0xe0
[ 1873.243953]  ? __prepare_to_swait+0x4b/0x70
[ 1873.243958]  wait_for_completion+0x84/0xe0
[ 1873.243962]  __flush_work.isra.0+0x146/0x200
[ 1873.243966]  ? flush_workqueue_prep_pwqs+0x130/0x130
[ 1873.243971]  __lru_add_drain_all+0x158/0x1f0
[ 1873.243978]  do_migrate_pages+0x3d/0x2d0
[ 1873.243985]  ? pick_next_task_fair+0x39/0x3b0
[ 1873.243989]  ? put_prev_task_fair+0x1e/0x30
[ 1873.243992]  ? pick_next_task+0xb30/0xbd0
[ 1873.243995]  ? __tick_nohz_task_switch+0x1e/0x70
[ 1873.244000]  ? raw_spin_rq_unlock+0x18/0x60
[ 1873.244002]  ? finish_task_switch.isra.0+0xc1/0x2d0
[ 1873.244005]  ? __switch_to+0x12f/0x510
[ 1873.244013]  cpuset_migrate_mm_workfn+0x22/0x40
[ 1873.244016]  process_one_work+0x1e0/0x410
[ 1873.244019]  worker_thread+0x50/0x3b0
[ 1873.244022]  ? process_one_work+0x410/0x410
[ 1873.244024]  kthread+0x173/0x190
[ 1873.244027]  ? set_kthread_struct+0x40/0x40
[ 1873.244031]  ret_from_fork+0x1f/0x30

...

mm/swap.c +4 -6

@@ -876,14 +875,19 @@ atomic_t lru_disable_count = ATOMIC_INIT(0);
 void lru_cache_disable(void)
 {
 	atomic_inc(&lru_disable_count);
+	synchronize_rcu();
 #ifdef CONFIG_SMP
 	/*
-	 * lru_add_drain_all in the force mode will schedule draining on
-	 * all online CPUs so any calls of lru_cache_disabled wrapped by
-	 * local_lock or preemption disabled would be ordered by that.
-	 * The atomic operation doesn't need to have stronger ordering
-	 * requirements because that is enforced by the scheduling
-	 * guarantees.
+	 * synchronize_rcu() waits for preemption disabled
+	 * and RCU read side critical sections.
+	 * For the users of lru_disable_count:

(Question for paulmck below, please)

Quoted 54 lines · On Tue, 22 Feb 2022 13:07:35 -0300 Marcelo Tosatti <[email protected]> wrote:

On systems that run FIFO:1 applications that busy loop on isolated CPUs, executing tasks on such CPUs under lower priority is undesired (since that will either hang the system, or cause longer interruption to the FIFO task due to execution of lower priority task with very small sched slices).

Commit d479960e44f27e0e52ba31b21740b703c538027c ("mm: disable LRU pagevec during the migration temporarily") relies on queueing work items on all online CPUs to ensure visibility of lru_disable_count.

However, its possible to use synchronize_rcu which will provide the same guarantees (see comment this patch modifies on lru_cache_disable).

Fixes:

[ 1873.243925] INFO: task kworker/u160:0:9 blocked for more than 622 seconds.
[ 1873.243927]       Tainted: G          I      --------- ---  5.14.0-31.rt21.31.el9.x86_64 #1
[ 1873.243929] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
[ 1873.243929] task:kworker/u160:0  state:D stack:    0 pid:    9 ppid:     2 flags:0x00004000
[ 1873.243932] Workqueue: cpuset_migrate_mm cpuset_migrate_mm_workfn
[ 1873.243936] Call Trace:
[ 1873.243938]  __schedule+0x21b/0x5b0
[ 1873.243941]  schedule+0x43/0xe0
[ 1873.243943]  schedule_timeout+0x14d/0x190
[ 1873.243946]  ? resched_curr+0x20/0xe0
[ 1873.243953]  ? __prepare_to_swait+0x4b/0x70
[ 1873.243958]  wait_for_completion+0x84/0xe0
[ 1873.243962]  __flush_work.isra.0+0x146/0x200
[ 1873.243966]  ? flush_workqueue_prep_pwqs+0x130/0x130
[ 1873.243971]  __lru_add_drain_all+0x158/0x1f0
[ 1873.243978]  do_migrate_pages+0x3d/0x2d0
[ 1873.243985]  ? pick_next_task_fair+0x39/0x3b0
[ 1873.243989]  ? put_prev_task_fair+0x1e/0x30
[ 1873.243992]  ? pick_next_task+0xb30/0xbd0
[ 1873.243995]  ? __tick_nohz_task_switch+0x1e/0x70
[ 1873.244000]  ? raw_spin_rq_unlock+0x18/0x60
[ 1873.244002]  ? finish_task_switch.isra.0+0xc1/0x2d0
[ 1873.244005]  ? __switch_to+0x12f/0x510
[ 1873.244013]  cpuset_migrate_mm_workfn+0x22/0x40
[ 1873.244016]  process_one_work+0x1e0/0x410
[ 1873.244019]  worker_thread+0x50/0x3b0
[ 1873.244022]  ? process_one_work+0x410/0x410
[ 1873.244024]  kthread+0x173/0x190
[ 1873.244027]  ? set_kthread_struct+0x40/0x40
[ 1873.244031]  ret_from_fork+0x1f/0x30

...

mm/swap.c inline void __lru_add_drain_all(bool force_all_cpus) · line 837

Quoted 9 lines

mm/swap.c +1 -2

@@ -831,8 +831,7 @@ inline void __lru_add_drain_all(bool force_all_cpus)
 	for_each_online_cpu(cpu) {
 		struct work_struct *work = &per_cpu(lru_add_drain_work, cpu);
 
-		if (force_all_cpus ||
-		    pagevec_count(&per_cpu(lru_pvecs.lru_add, cpu)) ||
+		if (pagevec_count(&per_cpu(lru_pvecs.lru_add, cpu)) ||
 		    data_race(pagevec_count(&per_cpu(lru_rotate.pvec, cpu))) ||
 		    pagevec_count(&per_cpu(lru_pvecs.lru_deactivate_file, cpu)) ||
 		    pagevec_count(&per_cpu(lru_pvecs.lru_deactivate, cpu)) ||

This change appears to be "don't queue work on CPUs which don't have any work to do". Correct? This isn't changelogged?

Quoted 16 lines

mm/swap.c +4 -6

@@ -876,14 +875,19 @@ atomic_t lru_disable_count = ATOMIC_INIT(0);
 void lru_cache_disable(void)
 {
 	atomic_inc(&lru_disable_count);
+	synchronize_rcu();
 #ifdef CONFIG_SMP
 	/*
-	 * lru_add_drain_all in the force mode will schedule draining on
-	 * all online CPUs so any calls of lru_cache_disabled wrapped by
-	 * local_lock or preemption disabled would be ordered by that.
-	 * The atomic operation doesn't need to have stronger ordering
-	 * requirements because that is enforced by the scheduling
-	 * guarantees.
+	 * synchronize_rcu() waits for preemption disabled
+	 * and RCU read side critical sections.
+	 * For the users of lru_disable_count:

mm/swap.c atomic_t lru_disable_count = ATOMIC_INIT(0); · line 893

Quoted 10 lines

mm/swap.c +7 -0

+	 *
+	 * preempt_disable, local_irq_disable  [bh_lru_lock()]
+	 * rcu_read_lock		       [rt_spin_lock CONFIG_PREEMPT_RT]
+	 * preempt_disable		       [local_lock !CONFIG_PREEMPT_RT]
+	 *
+	 * so any calls of lru_cache_disabled wrapped by local_lock or
+	 * preemption disabled would be ordered by that.
 	 */
 	__lru_add_drain_all(true);
 #else

Does this also work with CONFIG_TINY_RCU?

This seems abusive of synchronize_rcu(). None of this code uses RCU, but it so happens that synchronize_rcu() happily provides the desired effects. Changes in RCU's happy side-effects might break this. Perhaps a formal API function which does whatever-you-want-it-to-do would be better.

And... I really don't understand the fix. What is it about synchronize_rcu() which guarantees that a work function which is queued on CPU N will now get executed even if CPU N is spinning in SCHED_FIFO userspace?

(Question for paulmck below, please)

Quoted 54 lines · On Tue, 22 Feb 2022 13:07:35 -0300 Marcelo Tosatti <[email protected]> wrote:

On systems that run FIFO:1 applications that busy loop on isolated CPUs, executing tasks on such CPUs under lower priority is undesired (since that will either hang the system, or cause longer interruption to the FIFO task due to execution of lower priority task with very small sched slices).

Commit d479960e44f27e0e52ba31b21740b703c538027c ("mm: disable LRU pagevec during the migration temporarily") relies on queueing work items on all online CPUs to ensure visibility of lru_disable_count.

However, its possible to use synchronize_rcu which will provide the same guarantees (see comment this patch modifies on lru_cache_disable).

Fixes:

[ 1873.243925] INFO: task kworker/u160:0:9 blocked for more than 622 seconds.
[ 1873.243927]       Tainted: G          I      --------- ---  5.14.0-31.rt21.31.el9.x86_64 #1
[ 1873.243929] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
[ 1873.243929] task:kworker/u160:0  state:D stack:    0 pid:    9 ppid:     2 flags:0x00004000
[ 1873.243932] Workqueue: cpuset_migrate_mm cpuset_migrate_mm_workfn
[ 1873.243936] Call Trace:
[ 1873.243938]  __schedule+0x21b/0x5b0
[ 1873.243941]  schedule+0x43/0xe0
[ 1873.243943]  schedule_timeout+0x14d/0x190
[ 1873.243946]  ? resched_curr+0x20/0xe0
[ 1873.243953]  ? __prepare_to_swait+0x4b/0x70
[ 1873.243958]  wait_for_completion+0x84/0xe0
[ 1873.243962]  __flush_work.isra.0+0x146/0x200
[ 1873.243966]  ? flush_workqueue_prep_pwqs+0x130/0x130
[ 1873.243971]  __lru_add_drain_all+0x158/0x1f0
[ 1873.243978]  do_migrate_pages+0x3d/0x2d0
[ 1873.243985]  ? pick_next_task_fair+0x39/0x3b0
[ 1873.243989]  ? put_prev_task_fair+0x1e/0x30
[ 1873.243992]  ? pick_next_task+0xb30/0xbd0
[ 1873.243995]  ? __tick_nohz_task_switch+0x1e/0x70
[ 1873.244000]  ? raw_spin_rq_unlock+0x18/0x60
[ 1873.244002]  ? finish_task_switch.isra.0+0xc1/0x2d0
[ 1873.244005]  ? __switch_to+0x12f/0x510
[ 1873.244013]  cpuset_migrate_mm_workfn+0x22/0x40
[ 1873.244016]  process_one_work+0x1e0/0x410
[ 1873.244019]  worker_thread+0x50/0x3b0
[ 1873.244022]  ? process_one_work+0x410/0x410
[ 1873.244024]  kthread+0x173/0x190
[ 1873.244027]  ? set_kthread_struct+0x40/0x40
[ 1873.244031]  ret_from_fork+0x1f/0x30

...

mm/swap.c inline void __lru_add_drain_all(bool force_all_cpus) · line 837

Quoted 9 lines

mm/swap.c +1 -2

@@ -831,8 +831,7 @@ inline void __lru_add_drain_all(bool force_all_cpus)
 	for_each_online_cpu(cpu) {
 		struct work_struct *work = &per_cpu(lru_add_drain_work, cpu);
 
-		if (force_all_cpus ||
-		    pagevec_count(&per_cpu(lru_pvecs.lru_add, cpu)) ||
+		if (pagevec_count(&per_cpu(lru_pvecs.lru_add, cpu)) ||
 		    data_race(pagevec_count(&per_cpu(lru_rotate.pvec, cpu))) ||
 		    pagevec_count(&per_cpu(lru_pvecs.lru_deactivate_file, cpu)) ||
 		    pagevec_count(&per_cpu(lru_pvecs.lru_deactivate, cpu)) ||

This change appears to be "don't queue work on CPUs which don't have any work to do". Correct? This isn't changelogged?

Quoted 16 lines

mm/swap.c +4 -6

@@ -876,14 +875,19 @@ atomic_t lru_disable_count = ATOMIC_INIT(0);
 void lru_cache_disable(void)
 {
 	atomic_inc(&lru_disable_count);
+	synchronize_rcu();
 #ifdef CONFIG_SMP
 	/*
-	 * lru_add_drain_all in the force mode will schedule draining on
-	 * all online CPUs so any calls of lru_cache_disabled wrapped by
-	 * local_lock or preemption disabled would be ordered by that.
-	 * The atomic operation doesn't need to have stronger ordering
-	 * requirements because that is enforced by the scheduling
-	 * guarantees.
+	 * synchronize_rcu() waits for preemption disabled
+	 * and RCU read side critical sections.
+	 * For the users of lru_disable_count:

mm/swap.c atomic_t lru_disable_count = ATOMIC_INIT(0); · line 893

Quoted 10 lines

mm/swap.c +7 -0

+	 *
+	 * preempt_disable, local_irq_disable  [bh_lru_lock()]
+	 * rcu_read_lock		       [rt_spin_lock CONFIG_PREEMPT_RT]
+	 * preempt_disable		       [local_lock !CONFIG_PREEMPT_RT]
+	 *
+	 * so any calls of lru_cache_disabled wrapped by local_lock or
+	 * preemption disabled would be ordered by that.
 	 */
 	__lru_add_drain_all(true);
 #else

Does this also work with CONFIG_TINY_RCU?

This seems abusive of synchronize_rcu(). None of this code uses RCU, but it so happens that synchronize_rcu() happily provides the desired effects. Changes in RCU's happy side-effects might break this. Perhaps a formal API function which does whatever-you-want-it-to-do would be better.

And... I really don't understand the fix. What is it about synchronize_rcu() which guarantees that a work function which is queued on CPU N will now get executed even if CPU N is spinning in SCHED_FIFO userspace?

Quoted 61 lines · Andrew Morton

(Question for paulmck below, please)

Quoted 57 lines · On Tue, 22 Feb 2022 13:07:35 -0300 Marcelo Tosatti <[email protected]> wrote:

On systems that run FIFO:1 applications that busy loop on isolated CPUs, executing tasks on such CPUs under lower priority is undesired (since that will either hang the system, or cause longer interruption to the FIFO task due to execution of lower priority task with very small sched slices).

Commit d479960e44f27e0e52ba31b21740b703c538027c ("mm: disable LRU pagevec during the migration temporarily") relies on queueing work items on all online CPUs to ensure visibility of lru_disable_count.

However, its possible to use synchronize_rcu which will provide the same guarantees (see comment this patch modifies on lru_cache_disable).

Fixes:

[ 1873.243925] INFO: task kworker/u160:0:9 blocked for more than 622 seconds.
[ 1873.243927]       Tainted: G          I      --------- ---  5.14.0-31.rt21.31.el9.x86_64 #1
[ 1873.243929] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
[ 1873.243929] task:kworker/u160:0  state:D stack:    0 pid:    9 ppid:     2 flags:0x00004000
[ 1873.243932] Workqueue: cpuset_migrate_mm cpuset_migrate_mm_workfn
[ 1873.243936] Call Trace:
[ 1873.243938]  __schedule+0x21b/0x5b0
[ 1873.243941]  schedule+0x43/0xe0
[ 1873.243943]  schedule_timeout+0x14d/0x190
[ 1873.243946]  ? resched_curr+0x20/0xe0
[ 1873.243953]  ? __prepare_to_swait+0x4b/0x70
[ 1873.243958]  wait_for_completion+0x84/0xe0
[ 1873.243962]  __flush_work.isra.0+0x146/0x200
[ 1873.243966]  ? flush_workqueue_prep_pwqs+0x130/0x130
[ 1873.243971]  __lru_add_drain_all+0x158/0x1f0
[ 1873.243978]  do_migrate_pages+0x3d/0x2d0
[ 1873.243985]  ? pick_next_task_fair+0x39/0x3b0
[ 1873.243989]  ? put_prev_task_fair+0x1e/0x30
[ 1873.243992]  ? pick_next_task+0xb30/0xbd0
[ 1873.243995]  ? __tick_nohz_task_switch+0x1e/0x70
[ 1873.244000]  ? raw_spin_rq_unlock+0x18/0x60
[ 1873.244002]  ? finish_task_switch.isra.0+0xc1/0x2d0
[ 1873.244005]  ? __switch_to+0x12f/0x510
[ 1873.244013]  cpuset_migrate_mm_workfn+0x22/0x40
[ 1873.244016]  process_one_work+0x1e0/0x410
[ 1873.244019]  worker_thread+0x50/0x3b0
[ 1873.244022]  ? process_one_work+0x410/0x410
[ 1873.244024]  kthread+0x173/0x190
[ 1873.244027]  ? set_kthread_struct+0x40/0x40
[ 1873.244031]  ret_from_fork+0x1f/0x30

...

mm/swap.c +0 -0

@@ -831,8 +831,7 @@ inline void __lru_add_drain_all(bool force_all_cpus)
 	for_each_online_cpu(cpu) {
 		struct work_struct *work = &per_cpu(lru_add_drain_work, cpu);
 

Quoted 9 lines

mm/swap.c inline void __lru_add_drain_all(bool force_all_cpus) · line 837

Quoted 6 lines

mm/swap.c +1 -2

-		if (force_all_cpus ||
-		    pagevec_count(&per_cpu(lru_pvecs.lru_add, cpu)) ||
+		if (pagevec_count(&per_cpu(lru_pvecs.lru_add, cpu)) ||
 		    data_race(pagevec_count(&per_cpu(lru_rotate.pvec, cpu))) ||
 		    pagevec_count(&per_cpu(lru_pvecs.lru_deactivate_file, cpu)) ||
 		    pagevec_count(&per_cpu(lru_pvecs.lru_deactivate, cpu)) ||

This change appears to be "don't queue work on CPUs which don't have any work to do". Correct? This isn't changelogged?

Quoted 29 lines

Quoted 16 lines

mm/swap.c +4 -6

@@ -876,14 +875,19 @@ atomic_t lru_disable_count = ATOMIC_INIT(0);
 void lru_cache_disable(void)
 {
 	atomic_inc(&lru_disable_count);
+	synchronize_rcu();
 #ifdef CONFIG_SMP
 	/*
-	 * lru_add_drain_all in the force mode will schedule draining on
-	 * all online CPUs so any calls of lru_cache_disabled wrapped by
-	 * local_lock or preemption disabled would be ordered by that.
-	 * The atomic operation doesn't need to have stronger ordering
-	 * requirements because that is enforced by the scheduling
-	 * guarantees.
+	 * synchronize_rcu() waits for preemption disabled
+	 * and RCU read side critical sections.
+	 * For the users of lru_disable_count:

mm/swap.c atomic_t lru_disable_count = ATOMIC_INIT(0); · line 893

Quoted 10 lines

mm/swap.c +7 -0

+	 *
+	 * preempt_disable, local_irq_disable  [bh_lru_lock()]
+	 * rcu_read_lock		       [rt_spin_lock CONFIG_PREEMPT_RT]
+	 * preempt_disable		       [local_lock !CONFIG_PREEMPT_RT]
+	 *
+	 * so any calls of lru_cache_disabled wrapped by local_lock or
+	 * preemption disabled would be ordered by that.
 	 */
 	__lru_add_drain_all(true);
 #else

Does this also work with CONFIG_TINY_RCU?

Quoted 5 lines

This seems abusive of synchronize_rcu(). None of this code uses RCU, but it so happens that synchronize_rcu() happily provides the desired effects. Changes in RCU's happy side-effects might break this. Perhaps a formal API function which does whatever-you-want-it-to-do would be better.

I don't claim to understand the full lru_cache_disable() use case, but since v5.1 synchronize_rcu() is guaranteed to wait on preempt_disable() regions of code. In contrast, back in the old days, you had to use synchronize_sched() to wait on preempt_disable() regions, even in CONFIG_PREEMPT=y kernels. So if the comment is accurate, it is OK.

Just be careful what you backport past v5.1...

Quoted 4 lines

And... I really don't understand the fix. What is it about synchronize_rcu() which guarantees that a work function which is queued on CPU N will now get executed even if CPU N is spinning in SCHED_FIFO userspace?

I don't understand this part, either.

Quoted 62 lines · Paul E. McKenney

Quoted 61 lines · Andrew Morton

(Question for paulmck below, please)

Quoted 57 lines · On Tue, 22 Feb 2022 13:07:35 -0300 Marcelo Tosatti <[email protected]> wrote:

On systems that run FIFO:1 applications that busy loop on isolated CPUs, executing tasks on such CPUs under lower priority is undesired (since that will either hang the system, or cause longer interruption to the FIFO task due to execution of lower priority task with very small sched slices).

Commit d479960e44f27e0e52ba31b21740b703c538027c ("mm: disable LRU pagevec during the migration temporarily") relies on queueing work items on all online CPUs to ensure visibility of lru_disable_count.

However, its possible to use synchronize_rcu which will provide the same guarantees (see comment this patch modifies on lru_cache_disable).

Fixes:

[ 1873.243925] INFO: task kworker/u160:0:9 blocked for more than 622 seconds.
[ 1873.243927]       Tainted: G          I      --------- ---  5.14.0-31.rt21.31.el9.x86_64 #1
[ 1873.243929] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
[ 1873.243929] task:kworker/u160:0  state:D stack:    0 pid:    9 ppid:     2 flags:0x00004000
[ 1873.243932] Workqueue: cpuset_migrate_mm cpuset_migrate_mm_workfn
[ 1873.243936] Call Trace:
[ 1873.243938]  __schedule+0x21b/0x5b0
[ 1873.243941]  schedule+0x43/0xe0
[ 1873.243943]  schedule_timeout+0x14d/0x190
[ 1873.243946]  ? resched_curr+0x20/0xe0
[ 1873.243953]  ? __prepare_to_swait+0x4b/0x70
[ 1873.243958]  wait_for_completion+0x84/0xe0
[ 1873.243962]  __flush_work.isra.0+0x146/0x200
[ 1873.243966]  ? flush_workqueue_prep_pwqs+0x130/0x130
[ 1873.243971]  __lru_add_drain_all+0x158/0x1f0
[ 1873.243978]  do_migrate_pages+0x3d/0x2d0
[ 1873.243985]  ? pick_next_task_fair+0x39/0x3b0
[ 1873.243989]  ? put_prev_task_fair+0x1e/0x30
[ 1873.243992]  ? pick_next_task+0xb30/0xbd0
[ 1873.243995]  ? __tick_nohz_task_switch+0x1e/0x70
[ 1873.244000]  ? raw_spin_rq_unlock+0x18/0x60
[ 1873.244002]  ? finish_task_switch.isra.0+0xc1/0x2d0
[ 1873.244005]  ? __switch_to+0x12f/0x510
[ 1873.244013]  cpuset_migrate_mm_workfn+0x22/0x40
[ 1873.244016]  process_one_work+0x1e0/0x410
[ 1873.244019]  worker_thread+0x50/0x3b0
[ 1873.244022]  ? process_one_work+0x410/0x410
[ 1873.244024]  kthread+0x173/0x190
[ 1873.244027]  ? set_kthread_struct+0x40/0x40
[ 1873.244031]  ret_from_fork+0x1f/0x30

...

mm/swap.c +0 -0

@@ -831,8 +831,7 @@ inline void __lru_add_drain_all(bool force_all_cpus)
 	for_each_online_cpu(cpu) {
 		struct work_struct *work = &per_cpu(lru_add_drain_work, cpu);
 

Quoted 9 lines

Quoted 9 lines

mm/swap.c inline void __lru_add_drain_all(bool force_all_cpus) · line 837

Quoted 6 lines

mm/swap.c +1 -2

-		if (force_all_cpus ||
-		    pagevec_count(&per_cpu(lru_pvecs.lru_add, cpu)) ||
+		if (pagevec_count(&per_cpu(lru_pvecs.lru_add, cpu)) ||
 		    data_race(pagevec_count(&per_cpu(lru_rotate.pvec, cpu))) ||
 		    pagevec_count(&per_cpu(lru_pvecs.lru_deactivate_file, cpu)) ||
 		    pagevec_count(&per_cpu(lru_pvecs.lru_deactivate, cpu)) ||

This change appears to be "don't queue work on CPUs which don't have any work to do". Correct? This isn't changelogged?

Its replaced by synchronize_rcu, and its mentioned in the changelog:

"However, its possible to use synchronize_rcu which will provide the same
guarantees (see comment this patch modifies on lru_cache_disable)."

Will resend -v4 with a more verbose changelog.

Quoted 35 lines

Quoted 34 lines

Quoted 16 lines

mm/swap.c +4 -6

@@ -876,14 +875,19 @@ atomic_t lru_disable_count = ATOMIC_INIT(0);
 void lru_cache_disable(void)
 {
 	atomic_inc(&lru_disable_count);
+	synchronize_rcu();
 #ifdef CONFIG_SMP
 	/*
-	 * lru_add_drain_all in the force mode will schedule draining on
-	 * all online CPUs so any calls of lru_cache_disabled wrapped by
-	 * local_lock or preemption disabled would be ordered by that.
-	 * The atomic operation doesn't need to have stronger ordering
-	 * requirements because that is enforced by the scheduling
-	 * guarantees.
+	 * synchronize_rcu() waits for preemption disabled
+	 * and RCU read side critical sections.
+	 * For the users of lru_disable_count:

mm/swap.c atomic_t lru_disable_count = ATOMIC_INIT(0); · line 893

Quoted 10 lines

mm/swap.c +7 -0

+	 *
+	 * preempt_disable, local_irq_disable  [bh_lru_lock()]
+	 * rcu_read_lock		       [rt_spin_lock CONFIG_PREEMPT_RT]
+	 * preempt_disable		       [local_lock !CONFIG_PREEMPT_RT]
+	 *
+	 * so any calls of lru_cache_disabled wrapped by local_lock or
+	 * preemption disabled would be ordered by that.
 	 */
 	__lru_add_drain_all(true);
 #else

Does this also work with CONFIG_TINY_RCU?

This seems abusive of synchronize_rcu(). None of this code uses RCU, but it so happens that synchronize_rcu() happily provides the desired effects. Changes in RCU's happy side-effects might break this. Perhaps a formal API function which does whatever-you-want-it-to-do would be better.

Quoted 5 lines

I don't claim to understand the full lru_cache_disable() use case, but since v5.1 synchronize_rcu() is guaranteed to wait on preempt_disable() regions of code. In contrast, back in the old days, you had to use synchronize_sched() to wait on preempt_disable() regions, even in CONFIG_PREEMPT=y kernels. So if the comment is accurate, it is OK.

OK, will add an additional comment regarding v5.1.

Quoted 8 lines

Just be careful what you backport past v5.1...

Quoted 4 lines

And... I really don't understand the fix. What is it about synchronize_rcu() which guarantees that a work function which is queued on CPU N will now get executed even if CPU N is spinning in SCHED_FIFO userspace?

I don't understand this part, either.

All CPUs should see lru_disable_count (and therefore not add pages to per-CPU LRU pvecs, otherwise the page migration bug fixed by d479960e44f27e0e52ba31b21740b703c538027c can occur.

To do this, the commit above ("mm: disable LRU pagevec during the migration temporarily") relies on queueing work items on all online CPUs to ensure visibility of lru_disable_count:

*/

(quoted diff) +14 -0

+void lru_cache_disable(void)
+{
+       atomic_inc(&lru_disable_count);
+#ifdef CONFIG_SMP
+       /*
+        * lru_add_drain_all in the force mode will schedule draining on
+        * all online CPUs so any calls of lru_cache_disabled wrapped by
+        * local_lock or preemption disabled would be ordered by that.
+        * The atomic operation doesn't need to have stronger ordering
+        * requirements because that is enforeced by the scheduling
+        * guarantees.
+        */
+       __lru_add_drain_all(true);
+#else

CPU-0 CPU-1

					local_lock(&lru_pvecs.lock);
                			pvec = this_cpu_ptr(&lru_pvecs.lru_deactivate_file);
					add page to per-CPU LRU pvec
					if atomic_read(lru_disable_count) != 0
						flush per-CPU LRU pvec

atomic_inc(lru_disable_count)

local_unlock(&lru_pvec.lock)
lru_add_drain_all(force_all_cpus=true)

However queueing the work items disturbs isolated CPUs. To avoid it, its possible to use synchronize_rcu instead:

CPU-0 CPU-1

                                        local_lock(&lru_pvecs.lock);
                                        pvec = this_cpu_ptr(&lru_pvecs.lru_deactivate_file);
                                        add page to per-CPU LRU pvec
                                        if atomic_read(lru_disable_count) != 0
                                                flush per-CPU LRU pvec

atomic_inc(lru_disable_count)

local_unlock(&lru_pvec.lock)
synchronize_rcu()

Which will wait for all preemption (or IRQ disabled) sections to complete, therefore ensuring visibilily of lru_disable_count.

On systems that run FIFO:1 applications that busy loop on isolated CPUs, executing tasks on such CPUs under lower priority is undesired (since that will either hang the system, or cause longer interruption to the FIFO task due to execution of lower priority task with very small sched slices).

Commit d479960e44f27e0e52ba31b21740b703c538027c ("mm: disable LRU pagevec during the migration temporarily") relies on queueing work items on all online CPUs to ensure visibility of lru_disable_count.

However, its possible to use synchronize_rcu which will provide the same guarantees (see comment this patch modifies on lru_cache_disable).

Fixes:

...

mm/swap.c +13 -7

 		    data_race(pagevec_count(&per_cpu(lru_rotate.pvec, cpu))) ||
 		    pagevec_count(&per_cpu(lru_pvecs.lru_deactivate_file, cpu)) ||
 		    pagevec_count(&per_cpu(lru_pvecs.lru_deactivate, cpu)) ||

@@ -876,15 +875,21 @@ atomic_t lru_disable_count = ATOMIC_INIT(0);
 void lru_cache_disable(void)
 {
 	atomic_inc(&lru_disable_count);
-#ifdef CONFIG_SMP
 	/*
-	 * lru_add_drain_all in the force mode will schedule draining on
-	 * all online CPUs so any calls of lru_cache_disabled wrapped by
-	 * local_lock or preemption disabled would be ordered by that.
-	 * The atomic operation doesn't need to have stronger ordering
-	 * requirements because that is enforced by the scheduling
-	 * guarantees.
+	 * Readers of lru_disable_count are protected by either disabling
+	 * preemption or rcu_read_lock:
+	 *
+	 * preempt_disable, local_irq_disable  [bh_lru_lock()]
+	 * rcu_read_lock		       [rt_spin_lock CONFIG_PREEMPT_RT]
+	 * preempt_disable		       [local_lock !CONFIG_PREEMPT_RT]
+	 *
+	 * Since v5.1 kernel, synchronize_rcu() is guaranteed to wait on
+	 * preempt_disable() regions of code. So any CPU which sees
+	 * lru_disable_count = 0 will have exited the critical
+	 * section when synchronize_rcu() returns.
 	 */
+	synchronize_rcu();
+#ifdef CONFIG_SMP
 	__lru_add_drain_all(true);
 #else
 	lru_add_and_bh_lrus_drain();

On systems that run FIFO:1 applications that busy loop on isolated CPUs, executing tasks on such CPUs under lower priority is undesired (since that will either hang the system, or cause longer interruption to the FIFO task due to execution of lower priority task with very small sched slices).

Commit d479960e44f27e0e52ba31b21740b703c538027c ("mm: disable LRU pagevec during the migration temporarily") relies on queueing work items on all online CPUs to ensure visibility of lru_disable_count.

However, its possible to use synchronize_rcu which will provide the same guarantees (see comment this patch modifies on lru_cache_disable).

Fixes:

[ 1873.243925] INFO: task kworker/u160:0:9 blocked for more than 622 seconds.
[ 1873.243927]       Tainted: G          I      --------- ---  5.14.0-31.rt21.31.el9.x86_64 #1
[ 1873.243929] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
[ 1873.243929] task:kworker/u160:0  state:D stack:    0 pid:    9 ppid:     2 flags:0x00004000
[ 1873.243932] Workqueue: cpuset_migrate_mm cpuset_migrate_mm_workfn
[ 1873.243936] Call Trace:
[ 1873.243938]  __schedule+0x21b/0x5b0
[ 1873.243941]  schedule+0x43/0xe0
[ 1873.243943]  schedule_timeout+0x14d/0x190
[ 1873.243946]  ? resched_curr+0x20/0xe0
[ 1873.243953]  ? __prepare_to_swait+0x4b/0x70
[ 1873.243958]  wait_for_completion+0x84/0xe0
[ 1873.243962]  __flush_work.isra.0+0x146/0x200
[ 1873.243966]  ? flush_workqueue_prep_pwqs+0x130/0x130
[ 1873.243971]  __lru_add_drain_all+0x158/0x1f0
[ 1873.243978]  do_migrate_pages+0x3d/0x2d0
[ 1873.243985]  ? pick_next_task_fair+0x39/0x3b0
[ 1873.243989]  ? put_prev_task_fair+0x1e/0x30
[ 1873.243992]  ? pick_next_task+0xb30/0xbd0
[ 1873.243995]  ? __tick_nohz_task_switch+0x1e/0x70
[ 1873.244000]  ? raw_spin_rq_unlock+0x18/0x60
[ 1873.244002]  ? finish_task_switch.isra.0+0xc1/0x2d0
[ 1873.244005]  ? __switch_to+0x12f/0x510
[ 1873.244013]  cpuset_migrate_mm_workfn+0x22/0x40
[ 1873.244016]  process_one_work+0x1e0/0x410
[ 1873.244019]  worker_thread+0x50/0x3b0
[ 1873.244022]  ? process_one_work+0x410/0x410
[ 1873.244024]  kthread+0x173/0x190
[ 1873.244027]  ? set_kthread_struct+0x40/0x40
[ 1873.244031]  ret_from_fork+0x1f/0x30

Signed-off-by Marcelo Tosatti <[email protected]>

---

v4: improve comment clarify, mention synchronize_rcu guarantees
on v5.1 (Andrew Morton /
Paul E. McKenney)
v3: update stale comment (Nicolas Saenz Julienne)
v2: rt_spin_lock calls rcu_read_lock, no need
to add it before local_lock on swap.c (Nicolas Saenz Julienne)

mm/swap.c +14 -9

index bcf3ac288b56..b5ee163daa66 100644
@@ -831,8 +831,7 @@ inline void __lru_add_drain_all(bool force_all_cpus)
 	for_each_online_cpu(cpu) {
 		struct work_struct *work = &per_cpu(lru_add_drain_work, cpu);
 
-		if (force_all_cpus ||
-		    pagevec_count(&per_cpu(lru_pvecs.lru_add, cpu)) ||
+		if (pagevec_count(&per_cpu(lru_pvecs.lru_add, cpu)) ||
 		    data_race(pagevec_count(&per_cpu(lru_rotate.pvec, cpu))) ||
 		    pagevec_count(&per_cpu(lru_pvecs.lru_deactivate_file, cpu)) ||
 		    pagevec_count(&per_cpu(lru_pvecs.lru_deactivate, cpu)) ||

@@ -876,15 +875,21 @@ atomic_t lru_disable_count = ATOMIC_INIT(0);
 void lru_cache_disable(void)
 {
 	atomic_inc(&lru_disable_count);
-#ifdef CONFIG_SMP
 	/*
-	 * lru_add_drain_all in the force mode will schedule draining on
-	 * all online CPUs so any calls of lru_cache_disabled wrapped by
-	 * local_lock or preemption disabled would be ordered by that.
-	 * The atomic operation doesn't need to have stronger ordering
-	 * requirements because that is enforced by the scheduling
-	 * guarantees.
+	 * Readers of lru_disable_count are protected by either disabling
+	 * preemption or rcu_read_lock:
+	 *
+	 * preempt_disable, local_irq_disable  [bh_lru_lock()]
+	 * rcu_read_lock		       [rt_spin_lock CONFIG_PREEMPT_RT]
+	 * preempt_disable		       [local_lock !CONFIG_PREEMPT_RT]
+	 *
+	 * Since v5.1 kernel, synchronize_rcu() is guaranteed to wait on
+	 * preempt_disable() regions of code. So any CPU which sees
+	 * lru_disable_count = 0 will have exited the critical
+	 * section when synchronize_rcu() returns.
 	 */
+	synchronize_rcu();
+#ifdef CONFIG_SMP
 	__lru_add_drain_all(true);
 #else
 	lru_add_and_bh_lrus_drain();

Quoted 26 lines · On Fri, 4 Mar 2022 13:29:31 -0300 Marcelo Tosatti <[email protected]> wrote:

On systems that run FIFO:1 applications that busy loop on isolated CPUs, executing tasks on such CPUs under lower priority is undesired (since that will either hang the system, or cause longer interruption to the FIFO task due to execution of lower priority task with very small sched slices).

Commit d479960e44f27e0e52ba31b21740b703c538027c ("mm: disable LRU pagevec during the migration temporarily") relies on queueing work items on all online CPUs to ensure visibility of lru_disable_count.

However, its possible to use synchronize_rcu which will provide the same guarantees (see comment this patch modifies on lru_cache_disable).

Fixes:

...

mm/swap.c +0 -0

@@ -831,8 +831,7 @@ inline void __lru_add_drain_all(bool force_all_cpus)
 	for_each_online_cpu(cpu) {
 		struct work_struct *work = &per_cpu(lru_add_drain_work, cpu);
 

mm/swap.c inline void __lru_add_drain_all(bool force_all_cpus) · line 834

Quoted 3 lines

mm/swap.c +1 -2

-		if (force_all_cpus ||
-		    pagevec_count(&per_cpu(lru_pvecs.lru_add, cpu)) ||
+		if (pagevec_count(&per_cpu(lru_pvecs.lru_add, cpu)) ||

Please changelog this alteration?

On systems that run FIFO:1 applications that busy loop on isolated CPUs, executing tasks on such CPUs under lower priority is undesired (since that will either hang the system, or cause longer interruption to the FIFO task due to execution of lower priority task with very small sched slices).

Commit d479960e44f27e0e52ba31b21740b703c538027c ("mm: disable LRU pagevec during the migration temporarily") relies on queueing work items on all online CPUs to ensure visibility of lru_disable_count.

However, its possible to use synchronize_rcu which will provide the same guarantees (see comment this patch modifies on lru_cache_disable).

Fixes:

[ 1873.243925] INFO: task kworker/u160:0:9 blocked for more than 622 seconds.
[ 1873.243927]       Tainted: G          I      --------- ---  5.14.0-31.rt21.31.el9.x86_64 #1
[ 1873.243929] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
[ 1873.243929] task:kworker/u160:0  state:D stack:    0 pid:    9 ppid:     2 flags:0x00004000
[ 1873.243932] Workqueue: cpuset_migrate_mm cpuset_migrate_mm_workfn
[ 1873.243936] Call Trace:
[ 1873.243938]  __schedule+0x21b/0x5b0
[ 1873.243941]  schedule+0x43/0xe0
[ 1873.243943]  schedule_timeout+0x14d/0x190
[ 1873.243946]  ? resched_curr+0x20/0xe0
[ 1873.243953]  ? __prepare_to_swait+0x4b/0x70
[ 1873.243958]  wait_for_completion+0x84/0xe0
[ 1873.243962]  __flush_work.isra.0+0x146/0x200
[ 1873.243966]  ? flush_workqueue_prep_pwqs+0x130/0x130
[ 1873.243971]  __lru_add_drain_all+0x158/0x1f0
[ 1873.243978]  do_migrate_pages+0x3d/0x2d0
[ 1873.243985]  ? pick_next_task_fair+0x39/0x3b0
[ 1873.243989]  ? put_prev_task_fair+0x1e/0x30
[ 1873.243992]  ? pick_next_task+0xb30/0xbd0
[ 1873.243995]  ? __tick_nohz_task_switch+0x1e/0x70
[ 1873.244000]  ? raw_spin_rq_unlock+0x18/0x60
[ 1873.244002]  ? finish_task_switch.isra.0+0xc1/0x2d0
[ 1873.244005]  ? __switch_to+0x12f/0x510
[ 1873.244013]  cpuset_migrate_mm_workfn+0x22/0x40
[ 1873.244016]  process_one_work+0x1e0/0x410
[ 1873.244019]  worker_thread+0x50/0x3b0
[ 1873.244022]  ? process_one_work+0x410/0x410
[ 1873.244024]  kthread+0x173/0x190
[ 1873.244027]  ? set_kthread_struct+0x40/0x40
[ 1873.244031]  ret_from_fork+0x1f/0x30

Signed-off-by Marcelo Tosatti <[email protected]>

On systems that run FIFO:1 applications that busy loop, any SCHED_OTHER task that attempts to execute on such a CPU (such as work threads) will not be scheduled, which leads to system hangs.

Commit d479960e44f27e0e52ba31b21740b703c538027c ("mm: disable LRU pagevec during the migration temporarily") relies on queueing work items on all online CPUs to ensure visibility of lru_disable_count.

To fix this, replace the usage of work items with synchronize_rcu, which provides the same guarantees.

Readers of lru_disable_count are protected by either disabling preemption or rcu_read_lock:

preempt_disable, local_irq_disable  [bh_lru_lock()]
rcu_read_lock                       [rt_spin_lock CONFIG_PREEMPT_RT]
preempt_disable                     [local_lock !CONFIG_PREEMPT_RT]

Since v5.1 kernel, synchronize_rcu() is guaranteed to wait on preempt_disable() regions of code. So any CPU which sees lru_disable_count = 0 will have exited the critical section when synchronize_rcu() returns.

Quoted 4 lines · On Thu, 10 Mar 2022 10:22:12 -0300 Marcelo Tosatti <[email protected]> wrote:

On systems that run FIFO:1 applications that busy loop, any SCHED_OTHER task that attempts to execute on such a CPU (such as work threads) will not be scheduled, which leads to system hangs.

Permitting a realtime thread to hang the entire system warrants a -stable backport, I think. That's just rude.

+ sched division

Quoted 5 lines · Andrew Morton

Quoted 4 lines · On Thu, 10 Mar 2022 10:22:12 -0300 Marcelo Tosatti <[email protected]> wrote:

On systems that run FIFO:1 applications that busy loop, any SCHED_OTHER task that attempts to execute on such a CPU (such as work threads) will not be scheduled, which leads to system hangs.

…

Quoted 3 lines

Permitting a realtime thread to hang the entire system warrants a -stable backport, I think. That's just rude.

I'm not sure if someone is not willingly breaking the system. Based on my experience, a thread with an elevated priority (that FIFO, RR or DL) should not hog the CPU. A normal user (!root && !CAP_SYS_NICE) can't increase the priority of the task.
To avoid a system hangup there is sched_rt_runtime_us which ensures that all RT threads are throttled once the RT class exceed a certain amount of runtime. This has been relaxed a little on systems with more CPUs so that the RT runtime can be shared but this sharing (RT_RUNTIME_SHARE) has been disabled by default a while ago. That safe switch (sched_rt_runtime_us) can be disabled and is usually disabled on RT system since the RT tasks usually run longer especially in corner cases.

+ sched division

Quoted 5 lines · Andrew Morton

Quoted 4 lines · On Thu, 10 Mar 2022 10:22:12 -0300 Marcelo Tosatti <[email protected]> wrote:

On systems that run FIFO:1 applications that busy loop, any SCHED_OTHER task that attempts to execute on such a CPU (such as work threads) will not be scheduled, which leads to system hangs.

…

Quoted 3 lines

Permitting a realtime thread to hang the entire system warrants a -stable backport, I think. That's just rude.

I'm not sure if someone is not willingly breaking the system. Based on my experience, a thread with an elevated priority (that FIFO, RR or DL) should not hog the CPU. A normal user (!root && !CAP_SYS_NICE) can't increase the priority of the task.
To avoid a system hangup there is sched_rt_runtime_us which ensures that all RT threads are throttled once the RT class exceed a certain amount of runtime. This has been relaxed a little on systems with more CPUs so that the RT runtime can be shared but this sharing (RT_RUNTIME_SHARE) has been disabled by default a while ago. That safe switch (sched_rt_runtime_us) can be disabled and is usually disabled on RT system since the RT tasks usually run longer especially in corner cases.

People often isolate CPUs and have busy-loop tasks running with SCHED_OTHER given that there is nothing else going on there will be no preemption. In this case, the worker would preempt the task. In this scenario I _can_ understand that one wants to avoid that preemption and try things differently like this patch suggests. We can even offload RCU thread from isolated CPUs.
But I wouldn't say this requires a backport because there is way for a RT thread, that claims 100% of the CPU, to break the system.

Quoted 13 lines · Sebastian Andrzej Siewior

+ sched division

Quoted 5 lines · Andrew Morton

Quoted 4 lines · On Thu, 10 Mar 2022 10:22:12 -0300 Marcelo Tosatti <[email protected]> wrote:

On systems that run FIFO:1 applications that busy loop, any SCHED_OTHER task that attempts to execute on such a CPU (such as work threads) will not be scheduled, which leads to system hangs.

…

Quoted 3 lines

Permitting a realtime thread to hang the entire system warrants a -stable backport, I think. That's just rude.

Quoted 11 lines

I'm not sure if someone is not willingly breaking the system. Based on my experience, a thread with an elevated priority (that FIFO, RR or DL) should not hog the CPU. A normal user (!root && !CAP_SYS_NICE) can't increase the priority of the task.
To avoid a system hangup there is sched_rt_runtime_us which ensures that all RT threads are throttled once the RT class exceed a certain amount of runtime. This has been relaxed a little on systems with more CPUs so that the RT runtime can be shared but this sharing (RT_RUNTIME_SHARE) has been disabled by default a while ago. That safe switch (sched_rt_runtime_us) can be disabled and is usually disabled on RT system since the RT tasks usually run longer especially in corner cases.

Sebastian,

Certain classes of applications appear to benefit from very low latency (or rather very low, to zero, task interruption length). For example, 5G RAN processing might require maximum interruption of < 10us.

IIRC the resolution of sched_rt_runtime_us being too high led to the creation of stalld:

https://github.com/bristot/stalld

The stalld program (which stands for 'stall daemon') is a mechanism to prevent the starvation of operating system threads in a Linux system. The premise is to start up on a housekeeping cpu (one that is not used for real-application purposes) and to periodically monitor the state of each thread in the system, looking for a thread that has been on a run queue (i.e. ready to run) for a specifed length of time without being run. This condition is usually hit when the thread is on the same cpu as a high-priority cpu-intensive task and therefore is being given no opportunity to run.

When a thread is judged to be starving, stalld changes that thread to use the SCHED_DEADLINE policy and gives the thread a small slice of time for that cpu (specified on the command line). The thread then runs and when that timeslice is used, the thread is then returned to its original scheduling policy and stalld then continues to monitor thread states.

---

Configuring 10us per second for time slice of lower priority tasks that stalld schedules, you'd still get a higher interruption than 10us to the "RT" high priority application.

So our impression seems to be that for such low latency requirements its better to avoid any lower priority work at all on the isolated CPUs.

Yes, there are still cases where work queues are required to do certain things... we have been trying to reduce such cases.

Quoted 9 lines

People often isolate CPUs and have busy-loop tasks running with SCHED_OTHER given that there is nothing else going on there will be no preemption. In this case, the worker would preempt the task. In this scenario I _can_ understand that one wants to avoid that preemption and try things differently like this patch suggests. We can even offload RCU thread from isolated CPUs.
But I wouldn't say this requires a backport because there is way for a RT thread, that claims 100% of the CPU, to break the system.

On RHEL-8 we had patches (from -RT tree) to avoid workqueues for flushing memory. So for RHEL-8 -> RHEL-9 the lack of the patch above is a regression (and the usage of workqueues and not performing the flush remotely).

But one might still see the pagevecs not being empty at __lru_add_drain_all:

                if (pagevec_count(&per_cpu(lru_pvecs.lru_add, cpu)) ||
                    data_race(pagevec_count(&per_cpu(lru_rotate.pvec, cpu))) ||
                    pagevec_count(&per_cpu(lru_pvecs.lru_deactivate_file, cpu)) ||
                    pagevec_count(&per_cpu(lru_pvecs.lru_deactivate, cpu)) ||
                    pagevec_count(&per_cpu(lru_pvecs.lru_lazyfree, cpu)) ||
                    need_activate_page_drain(cpu) ||
                    has_bh_in_lru(cpu, NULL)) {
                        INIT_WORK(work, lru_add_drain_per_cpu);
                        queue_work_on(cpu, mm_percpu_wq, work);
                        __cpumask_set_cpu(cpu, &has_work);
                }

Which needs fixing for a complete solution.

On systems that run FIFO:1 applications that busy loop, any SCHED_OTHER task that attempts to execute on such a CPU (such as work threads) will not be scheduled, which leads to system hangs.

Commit d479960e44f27e0e52ba31b21740b703c538027c ("mm: disable LRU pagevec during the migration temporarily") relies on queueing work items on all online CPUs to ensure visibility of lru_disable_count.

To fix this, replace the usage of work items with synchronize_rcu, which provides the same guarantees.

Readers of lru_disable_count are protected by either disabling preemption or rcu_read_lock:

preempt_disable, local_irq_disable  [bh_lru_lock()]
rcu_read_lock                       [rt_spin_lock CONFIG_PREEMPT_RT]
preempt_disable                     [local_lock !CONFIG_PREEMPT_RT]

Since v5.1 kernel, synchronize_rcu() is guaranteed to wait on preempt_disable() regions of code. So any CPU which sees lru_disable_count = 0 will have exited the critical section when synchronize_rcu() returns.

Signed-off-by Marcelo Tosatti <[email protected]>

Reviewed-by Nicolas Saenz Julienne <[email protected]>

Acked-by Minchan Kim <[email protected]>

Quoted 21 lines · Marcelo Tosatti

On systems that run FIFO:1 applications that busy loop, any SCHED_OTHER task that attempts to execute on such a CPU (such as work threads) will not be scheduled, which leads to system hangs.

Commit d479960e44f27e0e52ba31b21740b703c538027c ("mm: disable LRU pagevec during the migration temporarily") relies on queueing work items on all online CPUs to ensure visibility of lru_disable_count.

To fix this, replace the usage of work items with synchronize_rcu, which provides the same guarantees.

Readers of lru_disable_count are protected by either disabling preemption or rcu_read_lock:

preempt_disable, local_irq_disable  [bh_lru_lock()]
rcu_read_lock                       [rt_spin_lock CONFIG_PREEMPT_RT]
preempt_disable                     [local_lock !CONFIG_PREEMPT_RT]

Quoted 8 lines

Since v5.1 kernel, synchronize_rcu() is guaranteed to wait on preempt_disable() regions of code. So any CPU which sees lru_disable_count = 0 will have exited the critical section when synchronize_rcu() returns.

Signed-off-by Marcelo Tosatti <[email protected]>

Reviewed-by Nicolas Saenz Julienne <[email protected]>

Acked-by Minchan Kim <[email protected]>

Someone pointed me at this:

https://www.phoronix.com/scan.php?page=news_item&px=Linux-518-Stress-NUMA-Goes-Boom

which says this one causes a performance regression with stress-ng's NUMA test...

Quoted 2 lines · Borislav Petkov

Quoted 1 line · Marcelo Tosatti

...

Quoted 7 lines

Someone pointed me at this:

https://www.phoronix.com/scan.php?page=news_item&px=Linux-518-Stress-NUMA-Goes-Boom

which says this one causes a performance regression with stress-ng's NUMA test...

Michael,

This is probably do_migrate_pages that is taking too long due to synchronize_rcu().

Switching to synchronize_rcu_expedited() should probably fix it... Can you give it a try, please?

mm/swap.c +1 -1

index bceff0cb559c..04a8bbf9817a 100644
@@ -879,7 +879,7 @@ void lru_cache_disable(void)
 	 * lru_disable_count = 0 will have exited the critical
 	 * section when synchronize_rcu() returns.
 	 */
-	synchronize_rcu();
+	synchronize_rcu_expedited();
 #ifdef CONFIG_SMP
 	__lru_add_drain_all(true);
 #else

Quoted 15 lines · On Thu, 28 Apr 2022 15:00:11 -0300 Marcelo Tosatti <[email protected]> wrote:

Quoted 1 line · Borislav Petkov

On Thu, Mar 10, 2022 at 10:22:12AM -0300, Marcelo Tosatti wrote:

...

Quoted 6 lines

Someone pointed me at this:

https://www.phoronix.com/scan.php?page=news_item&px=Linux-518-Stress-NUMA-Goes-Boom

which says this one causes a performance regression with stress-ng's NUMA test...

Michael,

This is probably do_migrate_pages that is taking too long due to synchronize_rcu().

Quoted 2 lines

Switching to synchronize_rcu_expedited() should probably fix it... Can you give it a try, please?

I guess not.

Is anyone else able to demonstrate a stress-ng performance regression due to ff042f4a9b0508? And if so, are they able to try Marcelo's one-liner?

Quoted 16 lines

mm/swap.c +1 -1

index bceff0cb559c..04a8bbf9817a 100644
@@ -879,7 +879,7 @@ void lru_cache_disable(void)
  	 * lru_disable_count = 0 will have exited the critical
  	 * section when synchronize_rcu() returns.
  	 */
-	synchronize_rcu();
+	synchronize_rcu_expedited();
  #ifdef CONFIG_SMP
  	__lru_add_drain_all(true);
  #else

Quoted 18 lines · Andrew Morton

Quoted 16 lines · On Thu, 28 Apr 2022 15:00:11 -0300 Marcelo Tosatti <[email protected]> wrote:

Quoted 1 line · Borislav Petkov

On Thu, Mar 10, 2022 at 10:22:12AM -0300, Marcelo Tosatti wrote:

...

Quoted 7 lines

Someone pointed me at this:

https://www.phoronix.com/scan.php?page=news_item&px=Linux-518-Stress-NUMA-Goes-Boom

which says this one causes a performance regression with stress-ng's NUMA test...

Michael,

This is probably do_migrate_pages that is taking too long due to synchronize_rcu().

Quoted 7 lines

Quoted 2 lines

Switching to synchronize_rcu_expedited() should probably fix it... Can you give it a try, please?

I guess not.

Is anyone else able to demonstrate a stress-ng performance regression due to ff042f4a9b0508?  And if so, are they able to try Marcelo's one-liner?

Apologies I don't believe I got the email previously (or if it ended up in spam or otherwise overlooked) so just noticed this thread now...

I have the system around and will work on verifying it can reproduce still and can then test the patch, should be able to get it tomorrow.

Thanks and sorry about the delay.

Michael

Quoted 17 lines

Quoted 17 lines

mm/swap.c +1 -1

index bceff0cb559c..04a8bbf9817a 100644
@@ -879,7 +879,7 @@ void lru_cache_disable(void)
        * lru_disable_count = 0 will have exited the critical
        * section when synchronize_rcu() returns.
        */
-    synchronize_rcu();
+    synchronize_rcu_expedited();
   #ifdef CONFIG_SMP
       __lru_add_drain_all(true);
   #else

Quoted 22 lines · Michael Larabel

Quoted 13 lines · Andrew Morton

Quoted 11 lines · On Thu, 28 Apr 2022 15:00:11 -0300 Marcelo Tosatti <[email protected]> wrote:

Quoted 6 lines · Borislav Petkov

On Thu, Mar 10, 2022 at 10:22:12AM -0300, Marcelo Tosatti wrote:
Someone pointed me at this:
https://www.phoronix.com/scan.php?page=news_item&px=Linux-518-Stress-NUMA-Goes-Boom

which says this one causes a performance regression with stress-ng's NUMA test...

This is probably do_migrate_pages that is taking too long due to synchronize_rcu().

Quoted 7 lines

Quoted 2 lines

Switching to synchronize_rcu_expedited() should probably fix it... Can you give it a try, please?

I guess not.

Is anyone else able to demonstrate a stress-ng performance regression due to ff042f4a9b0508?  And if so, are they able to try Marcelo's one-liner?

Quoted 7 lines

Apologies I don't believe I got the email previously (or if it ended up in spam or otherwise overlooked) so just noticed this thread now...

I have the system around and will work on verifying it can reproduce still and can then test the patch, should be able to get it tomorrow.

Thanks and sorry about the delay.

Had a chance to look at it today still. I was able to reproduce the regression still on that 5950X system going from v5.17 to v5.18 (using newer stress-ng benchmark and other system changes since the prior tests). Confirmed it also still showed slower as of today's Git.

I can confirm with Marcelo's patch below that the stress-ng NUMA performance is back to the v5.17 level of performance (actually, faster) and certainly not like what I was seeing on v5.18 or Git to this point.

So all seems to be good with that one-liner for the stress-ng NUMA test case. All the system details and results for those interested is documented @ https://openbenchmarking.org/result/2205284-PTS-NUMAREGR17
but basically amounts to:

    Stress-NG 0.14
    Test: NUMA
    Bogo Ops/s > Higher Is Better
    v5.17: 412.88
    v5.18: 49.33
    20220528 Git: 49.66
    20220528 Git + sched-rcu-exped patch: 468.81

Apologies again about the delay / not seeing the email thread earlier.
lru_cache_disable: replace work queue synchronization with synchronize_rcu Thanks,

Michael

Tested-by Michael Larabel <[email protected]>

Quoted 15 lines

Quoted 15 lines

Quoted 15 lines

mm/swap.c +1 -1

index bceff0cb559c..04a8bbf9817a 100644
@@ -879,7 +879,7 @@ void lru_cache_disable(void)
        * lru_disable_count = 0 will have exited the critical
        * section when synchronize_rcu() returns.
        */
-    synchronize_rcu();
+    synchronize_rcu_expedited();
   #ifdef CONFIG_SMP
       __lru_add_drain_all(true);
   #else

Andrew, is there a reason why this patch afaics isn't mainlined yet and lingering in linux-next for so long?