[patch v2] mm: lru_cache_disable: replace work queue synchronization with synchronize_rcu

[patch v2] mm: lru_cache_disable: replace work queue synchronization with synchronize_rcu

[Marcelo Tosatti]

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:

    * synchronize_rcu() waits for preemption disabled
    * and RCU read side critical sections
    * For the users of lru_disable_count:
    *
    * preempt_disable, local_irq_disable() [bh_lru_lock()]
    * rcu_read_lock                        [lru_pvecs CONFIG_PREEMPT_RT]
    * preempt_disable                      [lru_pvecs !CONFIG_PREEMPT_RT]
    *
    *
    * so any calls of lru_cache_disabled wrapped by
    * local_lock+rcu_read_lock or preemption disabled would be
    * ordered by that. 

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 <mtosatti@redhat.com>

v2: rt_spin_lock calls rcu_read_lock, no need
to add it before local_lock on swap.c		(Nicolas Saenz Julienne) 

diff --git a/mm/swap.c b/mm/swap.c
index bcf3ac288b56..48299a125d68 100644
--- a/mm/swap.c
+++ b/mm/swap.c
@@ -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,14 +875,21 @@ 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:
+	 *
+	 * 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. The atomic
+	 * operation doesn't need to have stronger ordering requirements
+	 * because that is enforced by the scheduling guarantees.
 	 */
 	__lru_add_drain_all(true);
 #else

[patch v3] mm: lru_cache_disable: replace work queue synchronization with synchronize_rcu

[Marcelo Tosatti]

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 <mtosatti@redhat.com>

---

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) 

diff --git a/mm/swap.c b/mm/swap.c
index bcf3ac288b56..abb26293e7c1 100644
--- a/mm/swap.c
+++ b/mm/swap.c
@@ -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,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:
+	 *
+	 * 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

Re: [patch v3] mm: lru_cache_disable: replace work queue synchronization with synchronize_rcu

[Nicolas Saenz Julienne]

On Tue, 2022-02-22 at 13:07 -0300, Marcelo Tosatti 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
> 
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

Reviewed-by: Nicolas Saenz Julienne <nsaenzju@redhat.com>

Regards,

-- 
Nicolás Sáenz

Re: [patch v3] mm: lru_cache_disable: replace work queue synchronization with synchronize_rcu

[Andrew Morton]

(Question for paulmck below, please)

On Tue, 22 Feb 2022 13:07:35 -0300 Marcelo Tosatti <mtosatti@redhat.com> 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
> 
> ...
>
> --- a/mm/swap.c
> +++ b/mm/swap.c
> @@ -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?

> @@ -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:
> +	 *
> +	 * 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?

Re: [patch v3] mm: lru_cache_disable: replace work queue synchronization with synchronize_rcu

[Paul E. McKenney]

On Thu, Mar 03, 2022 at 05:03:23PM -0800, Andrew Morton wrote:
> (Question for paulmck below, please)
> 
> On Tue, 22 Feb 2022 13:07:35 -0300 Marcelo Tosatti <mtosatti@redhat.com> 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
> > 
> > ...
> >
> > --- a/mm/swap.c
> > +++ b/mm/swap.c
> > @@ -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?
> 
> > @@ -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:
> > +	 *
> > +	 * 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.

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...

> 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.

							Thanx, Paul

Re: [patch v3] mm: lru_cache_disable: replace work queue synchronization with synchronize_rcu

[Marcelo Tosatti]

On Thu, Mar 03, 2022 at 05:49:30PM -0800, Paul E. McKenney wrote:
> On Thu, Mar 03, 2022 at 05:03:23PM -0800, Andrew Morton wrote:
> > (Question for paulmck below, please)
> > 
> > On Tue, 22 Feb 2022 13:07:35 -0300 Marcelo Tosatti <mtosatti@redhat.com> 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
> > > 
> > > ...
> > >
> > > --- a/mm/swap.c
> > > +++ b/mm/swap.c
> > > @@ -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?

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.


> > > @@ -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:
> > > +	 *
> > > +	 * 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.
> 
> 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.

> Just be careful what you backport past v5.1...
> 
> > 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:

 */
+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.

Re: [patch v3] mm: lru_cache_disable: replace work queue synchronization with synchronize_rcu

[Marcelo Tosatti]

On Thu, Mar 03, 2022 at 05:03:23PM -0800, Andrew Morton wrote:
> (Question for paulmck below, please)
> 
> On Tue, 22 Feb 2022 13:07:35 -0300 Marcelo Tosatti <mtosatti@redhat.com> 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
> > 
> > ...
> >
> > --- a/mm/swap.c
> > +++ b/mm/swap.c
> > @@ -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?
> 
> > @@ -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:
> > +	 *
> > +	 * 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?

It does not. synchronize_rcu() replaces queueing the work functions,
to ensure visibility of lru_disable_count.

Re: [patch v3] mm: lru_cache_disable: replace work queue synchronization with synchronize_rcu

[Paul E. McKenney]

On Fri, Mar 04, 2022 at 12:08:46PM -0300, Marcelo Tosatti wrote:
> On Thu, Mar 03, 2022 at 05:49:30PM -0800, Paul E. McKenney wrote:
> > On Thu, Mar 03, 2022 at 05:03:23PM -0800, Andrew Morton wrote:
> > > (Question for paulmck below, please)
> > > 
> > > On Tue, 22 Feb 2022 13:07:35 -0300 Marcelo Tosatti <mtosatti@redhat.com> 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
> > > > 
> > > > ...
> > > >
> > > > --- a/mm/swap.c
> > > > +++ b/mm/swap.c
> > > > @@ -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?
> 
> 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.
> 
> 
> > > > @@ -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:
> > > > +	 *
> > > > +	 * 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.
> > 
> > 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.

And if someone does need to backport to a kernel version with old-style
limited synchronize_rcu() semantics, you can do this:

	synchronize_rcu_mult(call_rcu_mult, call_rcu_sched);

This will wait concurrently for an RCU and and RCU-sched grace period.

If you want the full-up semantics, you can do this to wait on all
three flavors, also including RCU-bh:

	synchronize_rcu_mult(call_rcu_mult, call_rcu_sched, call_rcu_bh);

> > Just be careful what you backport past v5.1...
> > 
> > > 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:
> 
>  */
> +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.

OK, that does sound plausible.  (But please note that I am not familiar
with that code.)

							Thanx, Paul

[patch v4] mm: lru_cache_disable: replace work queue synchronization with synchronize_rcu

[Marcelo Tosatti]

 
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 <mtosatti@redhat.com>
 
 ---
 
 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) 

diff --git a/mm/swap.c b/mm/swap.c
index bcf3ac288b56..b5ee163daa66 100644
--- a/mm/swap.c
+++ b/mm/swap.c
@@ -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();

Re: [patch v4] mm: lru_cache_disable: replace work queue synchronization with synchronize_rcu

[Andrew Morton]

On Fri, 4 Mar 2022 13:29:31 -0300 Marcelo Tosatti <mtosatti@redhat.com> 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:
> 
> ...
>
> --- a/mm/swap.c
> +++ b/mm/swap.c
> @@ -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)) ||

Please changelog this alteration?

>  		    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();

Re: [patch v4] mm: lru_cache_disable: replace work queue synchronization with synchronize_rcu

[Paul E. McKenney]

On Fri, Mar 04, 2022 at 01:29:31PM -0300, Marcelo Tosatti 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
> 
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

Given the explanation and the comments below, this does look plausible
to me.

							Thanx, Paul

>  ---
>  
>  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) 
> 
> diff --git a/mm/swap.c b/mm/swap.c
> index bcf3ac288b56..b5ee163daa66 100644
> --- a/mm/swap.c
> +++ b/mm/swap.c
> @@ -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();
> 

Re: [patch v4] mm: lru_cache_disable: replace work queue synchronization with synchronize_rcu

[Marcelo Tosatti]

On Fri, Mar 04, 2022 at 04:35:54PM -0800, Andrew Morton wrote:
> On Fri, 4 Mar 2022 13:29:31 -0300 Marcelo Tosatti <mtosatti@redhat.com> 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:
> > 
> > ...
> >
> > --- a/mm/swap.c
> > +++ b/mm/swap.c
> > @@ -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)) ||
> 
> Please changelog this alteration?

It should be now. Are you OK with this changelog ?
(if not, please let me know what should be improved).

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.

Fixes:
...

Thanks.

Re: [patch v4] mm: lru_cache_disable: replace work queue synchronization with synchronize_rcu

[Minchan Kim]

On Fri, Mar 04, 2022 at 01:29:31PM -0300, Marcelo Tosatti 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
> 
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

Looks like great to me. 

Acked-by: Minchan Kim <minchan@kernel.org>

While I reviewed it, it seems I found a bug that br_lru_install
needs to check lru_cache_disabled under bh_lru_lock. Let me
cook the patch.

Thanks!

[patch v5] mm: lru_cache_disable: replace work queue synchronization with synchronize_rcu

[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]

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 <mtosatti@redhat.com>
Reviewed-by: Nicolas Saenz Julienne <nsaenzju@redhat.com>
Acked-by: Minchan Kim <minchan@kernel.org>

---
 
 v5: changelog improvements  		(Andrew Morton)
 v4: improve comment clarity, 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) 

diff --git a/mm/swap.c b/mm/swap.c
index bcf3ac288b56..b5ee163daa66 100644
--- a/mm/swap.c
+++ b/mm/swap.c
@@ -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();

Re: [patch v5] mm: lru_cache_disable: replace work queue synchronization with synchronize_rcu

[Andrew Morton]

On Thu, 10 Mar 2022 10:22:12 -0300 Marcelo Tosatti <mtosatti@redhat.com> 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.
> 
> 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.

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

I'm inclined to send this upstream for 5.18-rc1, with that -stable tag.

But if agreeable, how far can we backport this?  Paul, do we know which
kernel version(s) have the desired synchronize_rcu() behaviour?

Now, we don't want -stable people backporting this into kernels where
synchronize_rcu() doesn't do what we want it to do.  So a sneaky thing
we could do is to identify the change which added the desired
synchronize_rcu() behaviour and make this patch Fixes:thatpatch.  That
should prevent people from backporting it too far.

Re: [patch v5] mm: lru_cache_disable: replace work queue synchronization with synchronize_rcu

[Sebastian Andrzej Siewior]

+ sched division

On 2022-03-10 18:23:26 [-0800], Andrew Morton wrote:
> On Thu, 10 Mar 2022 10:22:12 -0300 Marcelo Tosatti <mtosatti@redhat.com> 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.

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.

Sebastian

Re: [patch v5] mm: lru_cache_disable: replace work queue synchronization with synchronize_rcu

[Andrew Morton]

On Fri, 11 Mar 2022 09:35:49 +0100 Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:

> + sched division
> 
> On 2022-03-10 18:23:26 [-0800], Andrew Morton wrote:
> > On Thu, 10 Mar 2022 10:22:12 -0300 Marcelo Tosatti <mtosatti@redhat.com> 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.
> 
> 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.

Does all this apply if the kernel is non-preemptible?

Marcelo, do you know how the offending system bypassed the failsafe?

Re: [patch v5] mm: lru_cache_disable: replace work queue synchronization with synchronize_rcu

[Marcelo Tosatti]

On Fri, Mar 11, 2022 at 09:35:49AM +0100, Sebastian Andrzej Siewior wrote:
> + sched division
> 
> On 2022-03-10 18:23:26 [-0800], Andrew Morton wrote:
> > On Thu, 10 Mar 2022 10:22:12 -0300 Marcelo Tosatti <mtosatti@redhat.com> 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.
> 
> 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.

> 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.

Re: [patch v5] mm: lru_cache_disable: replace work queue synchronization with synchronize_rcu

[Hillf Danton]

On Sat, 12 Mar 2022 17:39:40 -0300 Marcelo Tosatti wrote:
> On Fri, Mar 11, 2022 at 09:35:49AM +0100, Sebastian Andrzej Siewior wrote:
> > + sched division
> > 
> > On 2022-03-10 18:23:26 [-0800], Andrew Morton wrote:
> > > On Thu, 10 Mar 2022 10:22:12 -0300 Marcelo Tosatti <mtosatti@redhat.com> 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.
> > 
> > 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.

Wonder if it is not a part of isolation to drain the lru cache.

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

If workqueues are still required on the isolated CPUs then how to
walk around the system hang mentioned at the begining of your commit log?

Hillf
> 
> > 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.

Re: [patch v5] mm: lru_cache_disable: replace work queue synchronization with synchronize_rcu

[Borislav Petkov]

On Thu, Mar 10, 2022 at 10:22:12AM -0300, Marcelo Tosatti 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.
> 
> 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 <mtosatti@redhat.com>
> Reviewed-by: Nicolas Saenz Julienne <nsaenzju@redhat.com>
> Acked-by: Minchan Kim <minchan@kernel.org>

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...

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [patch v5] mm: lru_cache_disable: replace work queue synchronization with synchronize_rcu

[Marcelo Tosatti]

On Thu, Mar 31, 2022 at 03:52:45PM +0200, Borislav Petkov wrote:
> On Thu, Mar 10, 2022 at 10:22:12AM -0300, Marcelo Tosatti 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.
> > 
> > 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 <mtosatti@redhat.com>
> > Reviewed-by: Nicolas Saenz Julienne <nsaenzju@redhat.com>
> > Acked-by: Minchan Kim <minchan@kernel.org>
> 
> 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?

diff --git a/mm/swap.c b/mm/swap.c
index bceff0cb559c..04a8bbf9817a 100644
--- a/mm/swap.c
+++ b/mm/swap.c
@@ -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

Re: [patch v5] mm: lru_cache_disable: replace work queue synchronization with synchronize_rcu

[Andrew Morton]

On Thu, 28 Apr 2022 15:00:11 -0300 Marcelo Tosatti <mtosatti@redhat.com> wrote:

> On Thu, Mar 31, 2022 at 03:52:45PM +0200, Borislav Petkov wrote:
> > 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...
> 
> 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?

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?

> diff --git a/mm/swap.c b/mm/swap.c
> index bceff0cb559c..04a8bbf9817a 100644
> --- a/mm/swap.c
> +++ b/mm/swap.c
> @@ -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
> 
> 

Re: [patch v5] mm: lru_cache_disable: replace work queue synchronization with synchronize_rcu

[Michael Larabel]

On 5/28/22 16:18, Andrew Morton wrote:
> On Thu, 28 Apr 2022 15:00:11 -0300 Marcelo Tosatti <mtosatti@redhat.com> wrote:
>
>> On Thu, Mar 31, 2022 at 03:52:45PM +0200, Borislav Petkov wrote:
>>> 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...
>> 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?
> 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



>
>> diff --git a/mm/swap.c b/mm/swap.c
>> index bceff0cb559c..04a8bbf9817a 100644
>> --- a/mm/swap.c
>> +++ b/mm/swap.c
>> @@ -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
>>
>>

Re: [patch v5] mm: lru_cache_disable: replace work queue synchronization with synchronize_rcu

[Michael Larabel]

On 5/28/22 17:54, Michael Larabel wrote:
> On 5/28/22 16:18, Andrew Morton wrote:
>> On Thu, 28 Apr 2022 15:00:11 -0300 Marcelo Tosatti 
>> <mtosatti@redhat.com> wrote:
>>
>>> On Thu, Mar 31, 2022 at 03:52:45PM +0200, Borislav Petkov wrote:
>>>> 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...
>>> 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?
>> 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
>
>

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.

Thanks,

Michael


Tested-by: Michael Larabel <Michael@MichaelLarabel.com>



>
>>
>>> diff --git a/mm/swap.c b/mm/swap.c
>>> index bceff0cb559c..04a8bbf9817a 100644
>>> --- a/mm/swap.c
>>> +++ b/mm/swap.c
>>> @@ -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
>>>
>>>

Re: [patch v5] mm: lru_cache_disable: replace work queue synchronization with synchronize_rcu

[Thorsten Leemhuis]

Hi, this is your Linux kernel regression tracker.

On 29.05.22 02:48, Michael Larabel wrote:
> On 5/28/22 17:54, Michael Larabel wrote:
>> On 5/28/22 16:18, Andrew Morton wrote:
>>> On Thu, 28 Apr 2022 15:00:11 -0300 Marcelo Tosatti
>>> <mtosatti@redhat.com> wrote:
>>>> On Thu, Mar 31, 2022 at 03:52:45PM +0200, Borislav Petkov wrote:
>>>>> 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().
>>>>
>>>> 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.
> 
> 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 <Michael@MichaelLarabel.com>

Andrew, is there a reason why this patch afaics isn't mainlined yet and
lingering in linux-next for so long? Michael confirmed that this patch
fixes a regression three weeks ago and a few days later Stefan confirmed
that his problem was solved as well:
https://lore.kernel.org/regressions/79bb603e-37cb-d1dd-1e12-7ce28d7cfdae@i2se.com/

Reminder: unless there are good reasons it shouldn't take this long to
for reason explained in
https://docs.kernel.org/process/handling-regressions.html

Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)

P.S.: As the Linux kernel's regression tracker I deal with a lot of
reports and sometimes miss something important when writing mails like
this. If that's the case here, don't hesitate to tell me in a public
reply, it's in everyone's interest to set the public record straight.

>>>> diff --git a/mm/swap.c b/mm/swap.c
>>>> index bceff0cb559c..04a8bbf9817a 100644
>>>> --- a/mm/swap.c
>>>> +++ b/mm/swap.c
>>>> @@ -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
>>>>
>>>>

Re: [patch v5] mm: lru_cache_disable: replace work queue synchronization with synchronize_rcu

[Andrew Morton]

On Sun, 19 Jun 2022 14:14:03 +0200 Thorsten Leemhuis <regressions@leemhuis.info> wrote:

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

I didn't bother doing a hotfixes merge last week because there wasn't anything
very urgent-looking in there.  I'll be putting together a pull request later this week.