Re: [PATCH] mm, slab: reschedule cache_reap() on the same CPU

[Vlastimil Babka]

On 04/12/2018 02:47 AM, Minchan Kim wrote:
> On Wed, Apr 11, 2018 at 09:00:07AM +0200, Vlastimil Babka wrote:
>> cache_reap() is initially scheduled in start_cpu_timer() via
>> schedule_delayed_work_on(). But then the next iterations are scheduled via
>> schedule_delayed_work(), i.e. using WORK_CPU_UNBOUND.
>>
>> Thus since commit ef557180447f ("workqueue: schedule WORK_CPU_UNBOUND work on
>> wq_unbound_cpumask CPUs") there is no guarantee the future iterations will 
>> run
>> on the originally intended cpu, although it's still preferred. I was able to
>> demonstrate this with /sys/module/workqueue/parameters/debug_force_rr_cpu.
>> IIUC, it may also happen due to migrating timers in nohz context. As a 
>> result,
>> some cpu's would be calling cache_reap() more frequently and others never.
>>
>> This patch uses schedule_delayed_work_on() with the current cpu when 
>> scheduling
>> the next iteration.
> 
> Could you write down part about "so what's the user effect on some 
> condition?".
> It would really help to pick up the patch.

Ugh, so let's continue the last paragraph with something like:

After this patch, cache_reap() executions will always remain on the
expected cpu. This can save some memory that could otherwise remain
indefinitely in array caches of some cpus or nodes, and prevent waste of
cpu cycles by executing cache_reap() too often on other cpus.

Re: [PATCH] mm, slab: reschedule cache_reap() on the same CPU

[Vlastimil Babka]

On 04/12/2018 02:47 AM, Minchan Kim wrote:
> On Wed, Apr 11, 2018 at 09:00:07AM +0200, Vlastimil Babka wrote:
>> cache_reap() is initially scheduled in start_cpu_timer() via
>> schedule_delayed_work_on(). But then the next iterations are scheduled via
>> schedule_delayed_work(), i.e. using WORK_CPU_UNBOUND.
>>
>> Thus since commit ef557180447f ("workqueue: schedule WORK_CPU_UNBOUND work on
>> wq_unbound_cpumask CPUs") there is no guarantee the future iterations will 
>> run
>> on the originally intended cpu, although it's still preferred. I was able to
>> demonstrate this with /sys/module/workqueue/parameters/debug_force_rr_cpu.
>> IIUC, it may also happen due to migrating timers in nohz context. As a 
>> result,
>> some cpu's would be calling cache_reap() more frequently and others never.
>>
>> This patch uses schedule_delayed_work_on() with the current cpu when 
>> scheduling
>> the next iteration.
> 
> Could you write down part about "so what's the user effect on some 
> condition?".
> It would really help to pick up the patch.

Ugh, so let's continue the last paragraph with something like:

After this patch, cache_reap() executions will always remain on the
expected cpu. This can save some memory that could otherwise remain
indefinitely in array caches of some cpus or nodes, and prevent waste of
cpu cycles by executing cache_reap() too often on other cpus.

Re: [PATCH] mm, slab: reschedule cache_reap() on the same CPU

[Minchan Kim]

On Wed, Apr 11, 2018 at 09:00:07AM +0200, Vlastimil Babka wrote:
> cache_reap() is initially scheduled in start_cpu_timer() via
> schedule_delayed_work_on(). But then the next iterations are scheduled via
> schedule_delayed_work(), i.e. using WORK_CPU_UNBOUND.
> 
> Thus since commit ef557180447f ("workqueue: schedule WORK_CPU_UNBOUND work on
> wq_unbound_cpumask CPUs") there is no guarantee the future iterations will run
> on the originally intended cpu, although it's still preferred. I was able to
> demonstrate this with /sys/module/workqueue/parameters/debug_force_rr_cpu.
> IIUC, it may also happen due to migrating timers in nohz context. As a result,
> some cpu's would be calling cache_reap() more frequently and others never.
> 
> This patch uses schedule_delayed_work_on() with the current cpu when 
> scheduling
> the next iteration.

Could you write down part about "so what's the user effect on some condition?".
It would really help to pick up the patch.

> 
> Signed-off-by: Vlastimil Babka 
> Fixes: ef557180447f ("workqueue: schedule WORK_CPU_UNBOUND work on 
> wq_unbound_cpumask CPUs")
> CC: 
> Cc: Joonsoo Kim 
> Cc: David Rientjes 
> Cc: Pekka Enberg 
> Cc: Christoph Lameter 
> Cc: Tejun Heo 
> Cc: Lai Jiangshan 
> Cc: John Stultz 
> Cc: Thomas Gleixner 
> Cc: Stephen Boyd 
> ---
>  mm/slab.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/slab.c b/mm/slab.c
> index 9095c3945425..a76006aae857 100644
> --- a/mm/slab.c
> +++ b/mm/slab.c
> @@ -4074,7 +4074,8 @@ static void cache_reap(struct work_struct *w)
>   next_reap_node();
>  out:
>   /* Set up the next iteration */
> - schedule_delayed_work(work, round_jiffies_relative(REAPTIMEOUT_AC));
> + schedule_delayed_work_on(smp_processor_id(), work,
> + round_jiffies_relative(REAPTIMEOUT_AC));
>  }
>  
>  void get_slabinfo(struct kmem_cache *cachep, struct slabinfo *sinfo)
> -- 
> 2.16.3
> 

Re: [PATCH] mm, slab: reschedule cache_reap() on the same CPU

[Minchan Kim]

On Wed, Apr 11, 2018 at 09:00:07AM +0200, Vlastimil Babka wrote:
> cache_reap() is initially scheduled in start_cpu_timer() via
> schedule_delayed_work_on(). But then the next iterations are scheduled via
> schedule_delayed_work(), i.e. using WORK_CPU_UNBOUND.
> 
> Thus since commit ef557180447f ("workqueue: schedule WORK_CPU_UNBOUND work on
> wq_unbound_cpumask CPUs") there is no guarantee the future iterations will run
> on the originally intended cpu, although it's still preferred. I was able to
> demonstrate this with /sys/module/workqueue/parameters/debug_force_rr_cpu.
> IIUC, it may also happen due to migrating timers in nohz context. As a result,
> some cpu's would be calling cache_reap() more frequently and others never.
> 
> This patch uses schedule_delayed_work_on() with the current cpu when 
> scheduling
> the next iteration.

Could you write down part about "so what's the user effect on some condition?".
It would really help to pick up the patch.

> 
> Signed-off-by: Vlastimil Babka 
> Fixes: ef557180447f ("workqueue: schedule WORK_CPU_UNBOUND work on 
> wq_unbound_cpumask CPUs")
> CC: 
> Cc: Joonsoo Kim 
> Cc: David Rientjes 
> Cc: Pekka Enberg 
> Cc: Christoph Lameter 
> Cc: Tejun Heo 
> Cc: Lai Jiangshan 
> Cc: John Stultz 
> Cc: Thomas Gleixner 
> Cc: Stephen Boyd 
> ---
>  mm/slab.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/slab.c b/mm/slab.c
> index 9095c3945425..a76006aae857 100644
> --- a/mm/slab.c
> +++ b/mm/slab.c
> @@ -4074,7 +4074,8 @@ static void cache_reap(struct work_struct *w)
>   next_reap_node();
>  out:
>   /* Set up the next iteration */
> - schedule_delayed_work(work, round_jiffies_relative(REAPTIMEOUT_AC));
> + schedule_delayed_work_on(smp_processor_id(), work,
> + round_jiffies_relative(REAPTIMEOUT_AC));
>  }
>  
>  void get_slabinfo(struct kmem_cache *cachep, struct slabinfo *sinfo)
> -- 
> 2.16.3
> 

Re: [PATCH] mm, slab: reschedule cache_reap() on the same CPU

[Christopher Lameter]

On Wed, 11 Apr 2018, Pekka Enberg wrote:

> Acked-by: Pekka Enberg 

Good to hear from you again.

Acked-by: Christoph Lameter 

Re: [PATCH] mm, slab: reschedule cache_reap() on the same CPU

[Christopher Lameter]

On Wed, 11 Apr 2018, Pekka Enberg wrote:

> Acked-by: Pekka Enberg 

Good to hear from you again.

Acked-by: Christoph Lameter 

Re: [PATCH] mm, slab: reschedule cache_reap() on the same CPU

[Pekka Enberg]

On 11/04/2018 10.00, Vlastimil Babka wrote:

cache_reap() is initially scheduled in start_cpu_timer() via
schedule_delayed_work_on(). But then the next iterations are scheduled via
schedule_delayed_work(), i.e. using WORK_CPU_UNBOUND.

Thus since commit ef557180447f ("workqueue: schedule WORK_CPU_UNBOUND work on
wq_unbound_cpumask CPUs") there is no guarantee the future iterations will run
on the originally intended cpu, although it's still preferred. I was able to
demonstrate this with /sys/module/workqueue/parameters/debug_force_rr_cpu.
IIUC, it may also happen due to migrating timers in nohz context. As a result,
some cpu's would be calling cache_reap() more frequently and others never.

This patch uses schedule_delayed_work_on() with the current cpu when scheduling
the next iteration.

Signed-off-by: Vlastimil Babka 
Fixes: ef557180447f ("workqueue: schedule WORK_CPU_UNBOUND work on 
wq_unbound_cpumask CPUs")
CC: 
Cc: Joonsoo Kim 
Cc: David Rientjes 
Cc: Pekka Enberg 
Cc: Christoph Lameter 
Cc: Tejun Heo 
Cc: Lai Jiangshan 
Cc: John Stultz 
Cc: Thomas Gleixner 
Cc: Stephen Boyd 


Acked-by: Pekka Enberg 


---
  mm/slab.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/mm/slab.c b/mm/slab.c
index 9095c3945425..a76006aae857 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -4074,7 +4074,8 @@ static void cache_reap(struct work_struct *w)
next_reap_node();
  out:
/* Set up the next iteration */
-   schedule_delayed_work(work, round_jiffies_relative(REAPTIMEOUT_AC));
+   schedule_delayed_work_on(smp_processor_id(), work,
+   round_jiffies_relative(REAPTIMEOUT_AC));
  }
  
  void get_slabinfo(struct kmem_cache *cachep, struct slabinfo *sinfo)

Re: [PATCH] mm, slab: reschedule cache_reap() on the same CPU

[Pekka Enberg]

On 11/04/2018 10.00, Vlastimil Babka wrote:

cache_reap() is initially scheduled in start_cpu_timer() via
schedule_delayed_work_on(). But then the next iterations are scheduled via
schedule_delayed_work(), i.e. using WORK_CPU_UNBOUND.

Thus since commit ef557180447f ("workqueue: schedule WORK_CPU_UNBOUND work on
wq_unbound_cpumask CPUs") there is no guarantee the future iterations will run
on the originally intended cpu, although it's still preferred. I was able to
demonstrate this with /sys/module/workqueue/parameters/debug_force_rr_cpu.
IIUC, it may also happen due to migrating timers in nohz context. As a result,
some cpu's would be calling cache_reap() more frequently and others never.

This patch uses schedule_delayed_work_on() with the current cpu when scheduling
the next iteration.

Signed-off-by: Vlastimil Babka 
Fixes: ef557180447f ("workqueue: schedule WORK_CPU_UNBOUND work on 
wq_unbound_cpumask CPUs")
CC: 
Cc: Joonsoo Kim 
Cc: David Rientjes 
Cc: Pekka Enberg 
Cc: Christoph Lameter 
Cc: Tejun Heo 
Cc: Lai Jiangshan 
Cc: John Stultz 
Cc: Thomas Gleixner 
Cc: Stephen Boyd 


Acked-by: Pekka Enberg 


---
  mm/slab.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/mm/slab.c b/mm/slab.c
index 9095c3945425..a76006aae857 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -4074,7 +4074,8 @@ static void cache_reap(struct work_struct *w)
next_reap_node();
  out:
/* Set up the next iteration */
-   schedule_delayed_work(work, round_jiffies_relative(REAPTIMEOUT_AC));
+   schedule_delayed_work_on(smp_processor_id(), work,
+   round_jiffies_relative(REAPTIMEOUT_AC));
  }
  
  void get_slabinfo(struct kmem_cache *cachep, struct slabinfo *sinfo)

[PATCH] mm, slab: reschedule cache_reap() on the same CPU

[Vlastimil Babka]

cache_reap() is initially scheduled in start_cpu_timer() via
schedule_delayed_work_on(). But then the next iterations are scheduled via
schedule_delayed_work(), i.e. using WORK_CPU_UNBOUND.

Thus since commit ef557180447f ("workqueue: schedule WORK_CPU_UNBOUND work on
wq_unbound_cpumask CPUs") there is no guarantee the future iterations will run
on the originally intended cpu, although it's still preferred. I was able to
demonstrate this with /sys/module/workqueue/parameters/debug_force_rr_cpu.
IIUC, it may also happen due to migrating timers in nohz context. As a result,
some cpu's would be calling cache_reap() more frequently and others never.

This patch uses schedule_delayed_work_on() with the current cpu when scheduling
the next iteration.

Signed-off-by: Vlastimil Babka 
Fixes: ef557180447f ("workqueue: schedule WORK_CPU_UNBOUND work on 
wq_unbound_cpumask CPUs")
CC: 
Cc: Joonsoo Kim 
Cc: David Rientjes 
Cc: Pekka Enberg 
Cc: Christoph Lameter 
Cc: Tejun Heo 
Cc: Lai Jiangshan 
Cc: John Stultz 
Cc: Thomas Gleixner 
Cc: Stephen Boyd 
---
 mm/slab.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/mm/slab.c b/mm/slab.c
index 9095c3945425..a76006aae857 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -4074,7 +4074,8 @@ static void cache_reap(struct work_struct *w)
next_reap_node();
 out:
/* Set up the next iteration */
-   schedule_delayed_work(work, round_jiffies_relative(REAPTIMEOUT_AC));
+   schedule_delayed_work_on(smp_processor_id(), work,
+   round_jiffies_relative(REAPTIMEOUT_AC));
 }
 
 void get_slabinfo(struct kmem_cache *cachep, struct slabinfo *sinfo)
-- 
2.16.3

[PATCH] mm, slab: reschedule cache_reap() on the same CPU

[Vlastimil Babka]

cache_reap() is initially scheduled in start_cpu_timer() via
schedule_delayed_work_on(). But then the next iterations are scheduled via
schedule_delayed_work(), i.e. using WORK_CPU_UNBOUND.

Thus since commit ef557180447f ("workqueue: schedule WORK_CPU_UNBOUND work on
wq_unbound_cpumask CPUs") there is no guarantee the future iterations will run
on the originally intended cpu, although it's still preferred. I was able to
demonstrate this with /sys/module/workqueue/parameters/debug_force_rr_cpu.
IIUC, it may also happen due to migrating timers in nohz context. As a result,
some cpu's would be calling cache_reap() more frequently and others never.

This patch uses schedule_delayed_work_on() with the current cpu when scheduling
the next iteration.

Signed-off-by: Vlastimil Babka 
Fixes: ef557180447f ("workqueue: schedule WORK_CPU_UNBOUND work on 
wq_unbound_cpumask CPUs")
CC: 
Cc: Joonsoo Kim 
Cc: David Rientjes 
Cc: Pekka Enberg 
Cc: Christoph Lameter 
Cc: Tejun Heo 
Cc: Lai Jiangshan 
Cc: John Stultz 
Cc: Thomas Gleixner 
Cc: Stephen Boyd 
---
 mm/slab.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/mm/slab.c b/mm/slab.c
index 9095c3945425..a76006aae857 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -4074,7 +4074,8 @@ static void cache_reap(struct work_struct *w)
next_reap_node();
 out:
/* Set up the next iteration */
-   schedule_delayed_work(work, round_jiffies_relative(REAPTIMEOUT_AC));
+   schedule_delayed_work_on(smp_processor_id(), work,
+   round_jiffies_relative(REAPTIMEOUT_AC));
 }
 
 void get_slabinfo(struct kmem_cache *cachep, struct slabinfo *sinfo)
-- 
2.16.3