[RFC PATCH] mm: move pcp and lru-pcp drainging into vmstat_wq

[RFC PATCH] mm: move pcp and lru-pcp drainging into vmstat_wq

[Michal Hocko]

From: Michal Hocko <mhocko@suse.com>

We currently have 2 specific WQ_RECLAIM workqueues. One for updating
pcp stats vmstat_wq and one dedicated to drain per cpu lru caches. This
seems more than necessary because both can run on a single WQ. Both
do not block on locks requiring a memory allocation nor perform any
allocations themselves. We will save one rescuer thread this way.

On the other hand drain_all_pages queues work on the system wq which
doesn't have rescuer and so this depend on memory allocation (when all
workers are stuck allocating and new ones cannot be created). This is
not critical as there should be somebody invoking the OOM killer (e.g.
the forking worker) and get the situation unstuck and eventually
performs the draining. Quite annoying though. This worker should be
using WQ_RECLAIM as well. We can reuse the same one as for lru draining
and vmstat.

Suggested-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Signed-off-by: Michal Hocko <mhocko@suse.com>
---

Hi,
Tetsuo has noted that drain_all_pages doesn't use WQ_RECLAIM [1]
and asked whether we can move the worker to the vmstat_wq which is
WQ_RECLAIM. I think the deadlock he has described shouldn't happen but
it would be really better to have the rescuer. I also think that we do
not really need 2 or more workqueues and also pull lru draining in.

What do you think? Please note I haven't tested it yet.

[1] http://lkml.kernel.org/r/201702031957.AGH86961.MLtOQVFOSHJFFO@I-love.SAKURA.ne.jp

 mm/internal.h   |  6 ++++++
 mm/page_alloc.c |  2 +-
 mm/swap.c       | 20 +-------------------
 mm/vmstat.c     | 11 ++++++-----
 4 files changed, 14 insertions(+), 25 deletions(-)

diff --git a/mm/internal.h b/mm/internal.h
index ccfc2a2969f4..9ecafefe33ba 100644
--- a/mm/internal.h
+++ b/mm/internal.h
@@ -498,4 +498,10 @@ extern const struct trace_print_flags pageflag_names[];
 extern const struct trace_print_flags vmaflag_names[];
 extern const struct trace_print_flags gfpflag_names[];
 
+/*
+ * only for MM internal work items which do not depend on
+ * any allocations or locks which might depend on allocations
+ */
+extern struct workqueue_struct *vmstat_wq;
+
 #endif	/* __MM_INTERNAL_H */
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 6c48053bcd81..0c0a7c38cd91 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -2419,7 +2419,7 @@ void drain_all_pages(struct zone *zone)
 	for_each_cpu(cpu, &cpus_with_pcps) {
 		struct work_struct *work = per_cpu_ptr(&pcpu_drain, cpu);
 		INIT_WORK(work, drain_local_pages_wq);
-		schedule_work_on(cpu, work);
+		queue_work_on(cpu, vmstat_wq, work);
 	}
 	for_each_cpu(cpu, &cpus_with_pcps)
 		flush_work(per_cpu_ptr(&pcpu_drain, cpu));
diff --git a/mm/swap.c b/mm/swap.c
index c4910f14f957..23f09d6dd212 100644
--- a/mm/swap.c
+++ b/mm/swap.c
@@ -670,24 +670,6 @@ static void lru_add_drain_per_cpu(struct work_struct *dummy)
 
 static DEFINE_PER_CPU(struct work_struct, lru_add_drain_work);
 
-/*
- * lru_add_drain_wq is used to do lru_add_drain_all() from a WQ_MEM_RECLAIM
- * workqueue, aiding in getting memory freed.
- */
-static struct workqueue_struct *lru_add_drain_wq;
-
-static int __init lru_init(void)
-{
-	lru_add_drain_wq = alloc_workqueue("lru-add-drain", WQ_MEM_RECLAIM, 0);
-
-	if (WARN(!lru_add_drain_wq,
-		"Failed to create workqueue lru_add_drain_wq"))
-		return -ENOMEM;
-
-	return 0;
-}
-early_initcall(lru_init);
-
 void lru_add_drain_all(void)
 {
 	static DEFINE_MUTEX(lock);
@@ -707,7 +689,7 @@ void lru_add_drain_all(void)
 		    pagevec_count(&per_cpu(lru_deactivate_pvecs, cpu)) ||
 		    need_activate_page_drain(cpu)) {
 			INIT_WORK(work, lru_add_drain_per_cpu);
-			queue_work_on(cpu, lru_add_drain_wq, work);
+			queue_work_on(cpu, vmstat_wq, work);
 			cpumask_set_cpu(cpu, &has_work);
 		}
 	}
diff --git a/mm/vmstat.c b/mm/vmstat.c
index 69f9aff39a2e..fc9c2d9f014b 100644
--- a/mm/vmstat.c
+++ b/mm/vmstat.c
@@ -1548,8 +1548,8 @@ static const struct file_operations proc_vmstat_file_operations = {
 };
 #endif /* CONFIG_PROC_FS */
 
+struct workqueue_struct *vmstat_wq;
 #ifdef CONFIG_SMP
-static struct workqueue_struct *vmstat_wq;
 static DEFINE_PER_CPU(struct delayed_work, vmstat_work);
 int sysctl_stat_interval __read_mostly = HZ;
 
@@ -1715,7 +1715,6 @@ static void __init start_shepherd_timer(void)
 		INIT_DEFERRABLE_WORK(per_cpu_ptr(&vmstat_work, cpu),
 			vmstat_update);
 
-	vmstat_wq = alloc_workqueue("vmstat", WQ_FREEZABLE|WQ_MEM_RECLAIM, 0);
 	schedule_delayed_work(&shepherd,
 		round_jiffies_relative(sysctl_stat_interval));
 }
@@ -1763,9 +1762,11 @@ static int vmstat_cpu_dead(unsigned int cpu)
 
 static int __init setup_vmstat(void)
 {
-#ifdef CONFIG_SMP
-	int ret;
+	int ret = 0;
+
+	vmstat_wq = alloc_workqueue("vmstat", WQ_FREEZABLE|WQ_MEM_RECLAIM, 0);
 
+#ifdef CONFIG_SMP
 	ret = cpuhp_setup_state_nocalls(CPUHP_MM_VMSTAT_DEAD, "mm/vmstat:dead",
 					NULL, vmstat_cpu_dead);
 	if (ret < 0)
@@ -1789,7 +1790,7 @@ static int __init setup_vmstat(void)
 	proc_create("vmstat", S_IRUGO, NULL, &proc_vmstat_file_operations);
 	proc_create("zoneinfo", S_IRUGO, NULL, &proc_zoneinfo_file_operations);
 #endif
-	return 0;
+	return ret;
 }
 module_init(setup_vmstat)
 
-- 
2.11.0

[RFC PATCH] mm: move pcp and lru-pcp drainging into vmstat_wq

[Michal Hocko]

From: Michal Hocko <mhocko@suse.com>

We currently have 2 specific WQ_RECLAIM workqueues. One for updating
pcp stats vmstat_wq and one dedicated to drain per cpu lru caches. This
seems more than necessary because both can run on a single WQ. Both
do not block on locks requiring a memory allocation nor perform any
allocations themselves. We will save one rescuer thread this way.

On the other hand drain_all_pages queues work on the system wq which
doesn't have rescuer and so this depend on memory allocation (when all
workers are stuck allocating and new ones cannot be created). This is
not critical as there should be somebody invoking the OOM killer (e.g.
the forking worker) and get the situation unstuck and eventually
performs the draining. Quite annoying though. This worker should be
using WQ_RECLAIM as well. We can reuse the same one as for lru draining
and vmstat.

Suggested-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Signed-off-by: Michal Hocko <mhocko@suse.com>
---

Hi,
Tetsuo has noted that drain_all_pages doesn't use WQ_RECLAIM [1]
and asked whether we can move the worker to the vmstat_wq which is
WQ_RECLAIM. I think the deadlock he has described shouldn't happen but
it would be really better to have the rescuer. I also think that we do
not really need 2 or more workqueues and also pull lru draining in.

What do you think? Please note I haven't tested it yet.

[1] http://lkml.kernel.org/r/201702031957.AGH86961.MLtOQVFOSHJFFO@I-love.SAKURA.ne.jp

 mm/internal.h   |  6 ++++++
 mm/page_alloc.c |  2 +-
 mm/swap.c       | 20 +-------------------
 mm/vmstat.c     | 11 ++++++-----
 4 files changed, 14 insertions(+), 25 deletions(-)

diff --git a/mm/internal.h b/mm/internal.h
index ccfc2a2969f4..9ecafefe33ba 100644
--- a/mm/internal.h
+++ b/mm/internal.h
@@ -498,4 +498,10 @@ extern const struct trace_print_flags pageflag_names[];
 extern const struct trace_print_flags vmaflag_names[];
 extern const struct trace_print_flags gfpflag_names[];
 
+/*
+ * only for MM internal work items which do not depend on
+ * any allocations or locks which might depend on allocations
+ */
+extern struct workqueue_struct *vmstat_wq;
+
 #endif	/* __MM_INTERNAL_H */
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 6c48053bcd81..0c0a7c38cd91 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -2419,7 +2419,7 @@ void drain_all_pages(struct zone *zone)
 	for_each_cpu(cpu, &cpus_with_pcps) {
 		struct work_struct *work = per_cpu_ptr(&pcpu_drain, cpu);
 		INIT_WORK(work, drain_local_pages_wq);
-		schedule_work_on(cpu, work);
+		queue_work_on(cpu, vmstat_wq, work);
 	}
 	for_each_cpu(cpu, &cpus_with_pcps)
 		flush_work(per_cpu_ptr(&pcpu_drain, cpu));
diff --git a/mm/swap.c b/mm/swap.c
index c4910f14f957..23f09d6dd212 100644
--- a/mm/swap.c
+++ b/mm/swap.c
@@ -670,24 +670,6 @@ static void lru_add_drain_per_cpu(struct work_struct *dummy)
 
 static DEFINE_PER_CPU(struct work_struct, lru_add_drain_work);
 
-/*
- * lru_add_drain_wq is used to do lru_add_drain_all() from a WQ_MEM_RECLAIM
- * workqueue, aiding in getting memory freed.
- */
-static struct workqueue_struct *lru_add_drain_wq;
-
-static int __init lru_init(void)
-{
-	lru_add_drain_wq = alloc_workqueue("lru-add-drain", WQ_MEM_RECLAIM, 0);
-
-	if (WARN(!lru_add_drain_wq,
-		"Failed to create workqueue lru_add_drain_wq"))
-		return -ENOMEM;
-
-	return 0;
-}
-early_initcall(lru_init);
-
 void lru_add_drain_all(void)
 {
 	static DEFINE_MUTEX(lock);
@@ -707,7 +689,7 @@ void lru_add_drain_all(void)
 		    pagevec_count(&per_cpu(lru_deactivate_pvecs, cpu)) ||
 		    need_activate_page_drain(cpu)) {
 			INIT_WORK(work, lru_add_drain_per_cpu);
-			queue_work_on(cpu, lru_add_drain_wq, work);
+			queue_work_on(cpu, vmstat_wq, work);
 			cpumask_set_cpu(cpu, &has_work);
 		}
 	}
diff --git a/mm/vmstat.c b/mm/vmstat.c
index 69f9aff39a2e..fc9c2d9f014b 100644
--- a/mm/vmstat.c
+++ b/mm/vmstat.c
@@ -1548,8 +1548,8 @@ static const struct file_operations proc_vmstat_file_operations = {
 };
 #endif /* CONFIG_PROC_FS */
 
+struct workqueue_struct *vmstat_wq;
 #ifdef CONFIG_SMP
-static struct workqueue_struct *vmstat_wq;
 static DEFINE_PER_CPU(struct delayed_work, vmstat_work);
 int sysctl_stat_interval __read_mostly = HZ;
 
@@ -1715,7 +1715,6 @@ static void __init start_shepherd_timer(void)
 		INIT_DEFERRABLE_WORK(per_cpu_ptr(&vmstat_work, cpu),
 			vmstat_update);
 
-	vmstat_wq = alloc_workqueue("vmstat", WQ_FREEZABLE|WQ_MEM_RECLAIM, 0);
 	schedule_delayed_work(&shepherd,
 		round_jiffies_relative(sysctl_stat_interval));
 }
@@ -1763,9 +1762,11 @@ static int vmstat_cpu_dead(unsigned int cpu)
 
 static int __init setup_vmstat(void)
 {
-#ifdef CONFIG_SMP
-	int ret;
+	int ret = 0;
+
+	vmstat_wq = alloc_workqueue("vmstat", WQ_FREEZABLE|WQ_MEM_RECLAIM, 0);
 
+#ifdef CONFIG_SMP
 	ret = cpuhp_setup_state_nocalls(CPUHP_MM_VMSTAT_DEAD, "mm/vmstat:dead",
 					NULL, vmstat_cpu_dead);
 	if (ret < 0)
@@ -1789,7 +1790,7 @@ static int __init setup_vmstat(void)
 	proc_create("vmstat", S_IRUGO, NULL, &proc_vmstat_file_operations);
 	proc_create("zoneinfo", S_IRUGO, NULL, &proc_zoneinfo_file_operations);
 #endif
-	return 0;
+	return ret;
 }
 module_init(setup_vmstat)
 
-- 
2.11.0

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC PATCH] mm: move pcp and lru-pcp drainging into vmstat_wq

[Vlastimil Babka]

On 02/07/2017 10:09 PM, Michal Hocko wrote:
> From: Michal Hocko <mhocko@suse.com>
>
> We currently have 2 specific WQ_RECLAIM workqueues. One for updating
> pcp stats vmstat_wq and one dedicated to drain per cpu lru caches. This
> seems more than necessary because both can run on a single WQ. Both
> do not block on locks requiring a memory allocation nor perform any
> allocations themselves. We will save one rescuer thread this way.
>
> On the other hand drain_all_pages queues work on the system wq which
> doesn't have rescuer and so this depend on memory allocation (when all
> workers are stuck allocating and new ones cannot be created). This is
> not critical as there should be somebody invoking the OOM killer (e.g.
> the forking worker) and get the situation unstuck and eventually
> performs the draining. Quite annoying though. This worker should be
> using WQ_RECLAIM as well. We can reuse the same one as for lru draining
> and vmstat.
>
> Suggested-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Signed-off-by: Michal Hocko <mhocko@suse.com>
> ---
>
> Hi,
> Tetsuo has noted that drain_all_pages doesn't use WQ_RECLAIM [1]
> and asked whether we can move the worker to the vmstat_wq which is
> WQ_RECLAIM. I think the deadlock he has described shouldn't happen but
> it would be really better to have the rescuer. I also think that we do
> not really need 2 or more workqueues and also pull lru draining in.
>
> What do you think? Please note I haven't tested it yet.

Why not, I guess, of course I may be overlooking some subtlety. You could have 
CC'd Tejun and Christoph.
Watch out for the init order though, maybe? Is there no caller of the lru/pcp 
drain before module_init(setup_vmstat) happens?

Also one nit below.


> [1] http://lkml.kernel.org/r/201702031957.AGH86961.MLtOQVFOSHJFFO@I-love.SAKURA.ne.jp
>
>  mm/internal.h   |  6 ++++++
>  mm/page_alloc.c |  2 +-
>  mm/swap.c       | 20 +-------------------
>  mm/vmstat.c     | 11 ++++++-----
>  4 files changed, 14 insertions(+), 25 deletions(-)
>
> diff --git a/mm/internal.h b/mm/internal.h
> index ccfc2a2969f4..9ecafefe33ba 100644
> --- a/mm/internal.h
> +++ b/mm/internal.h
> @@ -498,4 +498,10 @@ extern const struct trace_print_flags pageflag_names[];
>  extern const struct trace_print_flags vmaflag_names[];
>  extern const struct trace_print_flags gfpflag_names[];
>
> +/*
> + * only for MM internal work items which do not depend on
> + * any allocations or locks which might depend on allocations
> + */
> +extern struct workqueue_struct *vmstat_wq;
> +
>  #endif	/* __MM_INTERNAL_H */
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 6c48053bcd81..0c0a7c38cd91 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -2419,7 +2419,7 @@ void drain_all_pages(struct zone *zone)
>  	for_each_cpu(cpu, &cpus_with_pcps) {
>  		struct work_struct *work = per_cpu_ptr(&pcpu_drain, cpu);
>  		INIT_WORK(work, drain_local_pages_wq);
> -		schedule_work_on(cpu, work);
> +		queue_work_on(cpu, vmstat_wq, work);
>  	}
>  	for_each_cpu(cpu, &cpus_with_pcps)
>  		flush_work(per_cpu_ptr(&pcpu_drain, cpu));
> diff --git a/mm/swap.c b/mm/swap.c
> index c4910f14f957..23f09d6dd212 100644
> --- a/mm/swap.c
> +++ b/mm/swap.c
> @@ -670,24 +670,6 @@ static void lru_add_drain_per_cpu(struct work_struct *dummy)
>
>  static DEFINE_PER_CPU(struct work_struct, lru_add_drain_work);
>
> -/*
> - * lru_add_drain_wq is used to do lru_add_drain_all() from a WQ_MEM_RECLAIM
> - * workqueue, aiding in getting memory freed.
> - */
> -static struct workqueue_struct *lru_add_drain_wq;
> -
> -static int __init lru_init(void)
> -{
> -	lru_add_drain_wq = alloc_workqueue("lru-add-drain", WQ_MEM_RECLAIM, 0);
> -
> -	if (WARN(!lru_add_drain_wq,
> -		"Failed to create workqueue lru_add_drain_wq"))
> -		return -ENOMEM;
> -
> -	return 0;
> -}
> -early_initcall(lru_init);
> -
>  void lru_add_drain_all(void)
>  {
>  	static DEFINE_MUTEX(lock);
> @@ -707,7 +689,7 @@ void lru_add_drain_all(void)
>  		    pagevec_count(&per_cpu(lru_deactivate_pvecs, cpu)) ||
>  		    need_activate_page_drain(cpu)) {
>  			INIT_WORK(work, lru_add_drain_per_cpu);
> -			queue_work_on(cpu, lru_add_drain_wq, work);
> +			queue_work_on(cpu, vmstat_wq, work);
>  			cpumask_set_cpu(cpu, &has_work);
>  		}
>  	}
> diff --git a/mm/vmstat.c b/mm/vmstat.c
> index 69f9aff39a2e..fc9c2d9f014b 100644
> --- a/mm/vmstat.c
> +++ b/mm/vmstat.c
> @@ -1548,8 +1548,8 @@ static const struct file_operations proc_vmstat_file_operations = {
>  };
>  #endif /* CONFIG_PROC_FS */
>
> +struct workqueue_struct *vmstat_wq;
>  #ifdef CONFIG_SMP
> -static struct workqueue_struct *vmstat_wq;
>  static DEFINE_PER_CPU(struct delayed_work, vmstat_work);
>  int sysctl_stat_interval __read_mostly = HZ;
>
> @@ -1715,7 +1715,6 @@ static void __init start_shepherd_timer(void)
>  		INIT_DEFERRABLE_WORK(per_cpu_ptr(&vmstat_work, cpu),
>  			vmstat_update);
>
> -	vmstat_wq = alloc_workqueue("vmstat", WQ_FREEZABLE|WQ_MEM_RECLAIM, 0);
>  	schedule_delayed_work(&shepherd,
>  		round_jiffies_relative(sysctl_stat_interval));
>  }
> @@ -1763,9 +1762,11 @@ static int vmstat_cpu_dead(unsigned int cpu)
>
>  static int __init setup_vmstat(void)
>  {
> -#ifdef CONFIG_SMP
> -	int ret;
> +	int ret = 0;
> +
> +	vmstat_wq = alloc_workqueue("vmstat", WQ_FREEZABLE|WQ_MEM_RECLAIM, 0);

Did you want to set ret to -ENOMEM if the alloc fails, or something? Otherwise I 
don't see why the changes.

>
> +#ifdef CONFIG_SMP
>  	ret = cpuhp_setup_state_nocalls(CPUHP_MM_VMSTAT_DEAD, "mm/vmstat:dead",
>  					NULL, vmstat_cpu_dead);
>  	if (ret < 0)
> @@ -1789,7 +1790,7 @@ static int __init setup_vmstat(void)
>  	proc_create("vmstat", S_IRUGO, NULL, &proc_vmstat_file_operations);
>  	proc_create("zoneinfo", S_IRUGO, NULL, &proc_zoneinfo_file_operations);
>  #endif
> -	return 0;
> +	return ret;
>  }
>  module_init(setup_vmstat)
>
>

Re: [RFC PATCH] mm: move pcp and lru-pcp drainging into vmstat_wq

[Vlastimil Babka]

On 02/07/2017 10:09 PM, Michal Hocko wrote:
> From: Michal Hocko <mhocko@suse.com>
>
> We currently have 2 specific WQ_RECLAIM workqueues. One for updating
> pcp stats vmstat_wq and one dedicated to drain per cpu lru caches. This
> seems more than necessary because both can run on a single WQ. Both
> do not block on locks requiring a memory allocation nor perform any
> allocations themselves. We will save one rescuer thread this way.
>
> On the other hand drain_all_pages queues work on the system wq which
> doesn't have rescuer and so this depend on memory allocation (when all
> workers are stuck allocating and new ones cannot be created). This is
> not critical as there should be somebody invoking the OOM killer (e.g.
> the forking worker) and get the situation unstuck and eventually
> performs the draining. Quite annoying though. This worker should be
> using WQ_RECLAIM as well. We can reuse the same one as for lru draining
> and vmstat.
>
> Suggested-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Signed-off-by: Michal Hocko <mhocko@suse.com>
> ---
>
> Hi,
> Tetsuo has noted that drain_all_pages doesn't use WQ_RECLAIM [1]
> and asked whether we can move the worker to the vmstat_wq which is
> WQ_RECLAIM. I think the deadlock he has described shouldn't happen but
> it would be really better to have the rescuer. I also think that we do
> not really need 2 or more workqueues and also pull lru draining in.
>
> What do you think? Please note I haven't tested it yet.

Why not, I guess, of course I may be overlooking some subtlety. You could have 
CC'd Tejun and Christoph.
Watch out for the init order though, maybe? Is there no caller of the lru/pcp 
drain before module_init(setup_vmstat) happens?

Also one nit below.


> [1] http://lkml.kernel.org/r/201702031957.AGH86961.MLtOQVFOSHJFFO@I-love.SAKURA.ne.jp
>
>  mm/internal.h   |  6 ++++++
>  mm/page_alloc.c |  2 +-
>  mm/swap.c       | 20 +-------------------
>  mm/vmstat.c     | 11 ++++++-----
>  4 files changed, 14 insertions(+), 25 deletions(-)
>
> diff --git a/mm/internal.h b/mm/internal.h
> index ccfc2a2969f4..9ecafefe33ba 100644
> --- a/mm/internal.h
> +++ b/mm/internal.h
> @@ -498,4 +498,10 @@ extern const struct trace_print_flags pageflag_names[];
>  extern const struct trace_print_flags vmaflag_names[];
>  extern const struct trace_print_flags gfpflag_names[];
>
> +/*
> + * only for MM internal work items which do not depend on
> + * any allocations or locks which might depend on allocations
> + */
> +extern struct workqueue_struct *vmstat_wq;
> +
>  #endif	/* __MM_INTERNAL_H */
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 6c48053bcd81..0c0a7c38cd91 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -2419,7 +2419,7 @@ void drain_all_pages(struct zone *zone)
>  	for_each_cpu(cpu, &cpus_with_pcps) {
>  		struct work_struct *work = per_cpu_ptr(&pcpu_drain, cpu);
>  		INIT_WORK(work, drain_local_pages_wq);
> -		schedule_work_on(cpu, work);
> +		queue_work_on(cpu, vmstat_wq, work);
>  	}
>  	for_each_cpu(cpu, &cpus_with_pcps)
>  		flush_work(per_cpu_ptr(&pcpu_drain, cpu));
> diff --git a/mm/swap.c b/mm/swap.c
> index c4910f14f957..23f09d6dd212 100644
> --- a/mm/swap.c
> +++ b/mm/swap.c
> @@ -670,24 +670,6 @@ static void lru_add_drain_per_cpu(struct work_struct *dummy)
>
>  static DEFINE_PER_CPU(struct work_struct, lru_add_drain_work);
>
> -/*
> - * lru_add_drain_wq is used to do lru_add_drain_all() from a WQ_MEM_RECLAIM
> - * workqueue, aiding in getting memory freed.
> - */
> -static struct workqueue_struct *lru_add_drain_wq;
> -
> -static int __init lru_init(void)
> -{
> -	lru_add_drain_wq = alloc_workqueue("lru-add-drain", WQ_MEM_RECLAIM, 0);
> -
> -	if (WARN(!lru_add_drain_wq,
> -		"Failed to create workqueue lru_add_drain_wq"))
> -		return -ENOMEM;
> -
> -	return 0;
> -}
> -early_initcall(lru_init);
> -
>  void lru_add_drain_all(void)
>  {
>  	static DEFINE_MUTEX(lock);
> @@ -707,7 +689,7 @@ void lru_add_drain_all(void)
>  		    pagevec_count(&per_cpu(lru_deactivate_pvecs, cpu)) ||
>  		    need_activate_page_drain(cpu)) {
>  			INIT_WORK(work, lru_add_drain_per_cpu);
> -			queue_work_on(cpu, lru_add_drain_wq, work);
> +			queue_work_on(cpu, vmstat_wq, work);
>  			cpumask_set_cpu(cpu, &has_work);
>  		}
>  	}
> diff --git a/mm/vmstat.c b/mm/vmstat.c
> index 69f9aff39a2e..fc9c2d9f014b 100644
> --- a/mm/vmstat.c
> +++ b/mm/vmstat.c
> @@ -1548,8 +1548,8 @@ static const struct file_operations proc_vmstat_file_operations = {
>  };
>  #endif /* CONFIG_PROC_FS */
>
> +struct workqueue_struct *vmstat_wq;
>  #ifdef CONFIG_SMP
> -static struct workqueue_struct *vmstat_wq;
>  static DEFINE_PER_CPU(struct delayed_work, vmstat_work);
>  int sysctl_stat_interval __read_mostly = HZ;
>
> @@ -1715,7 +1715,6 @@ static void __init start_shepherd_timer(void)
>  		INIT_DEFERRABLE_WORK(per_cpu_ptr(&vmstat_work, cpu),
>  			vmstat_update);
>
> -	vmstat_wq = alloc_workqueue("vmstat", WQ_FREEZABLE|WQ_MEM_RECLAIM, 0);
>  	schedule_delayed_work(&shepherd,
>  		round_jiffies_relative(sysctl_stat_interval));
>  }
> @@ -1763,9 +1762,11 @@ static int vmstat_cpu_dead(unsigned int cpu)
>
>  static int __init setup_vmstat(void)
>  {
> -#ifdef CONFIG_SMP
> -	int ret;
> +	int ret = 0;
> +
> +	vmstat_wq = alloc_workqueue("vmstat", WQ_FREEZABLE|WQ_MEM_RECLAIM, 0);

Did you want to set ret to -ENOMEM if the alloc fails, or something? Otherwise I 
don't see why the changes.

>
> +#ifdef CONFIG_SMP
>  	ret = cpuhp_setup_state_nocalls(CPUHP_MM_VMSTAT_DEAD, "mm/vmstat:dead",
>  					NULL, vmstat_cpu_dead);
>  	if (ret < 0)
> @@ -1789,7 +1790,7 @@ static int __init setup_vmstat(void)
>  	proc_create("vmstat", S_IRUGO, NULL, &proc_vmstat_file_operations);
>  	proc_create("zoneinfo", S_IRUGO, NULL, &proc_zoneinfo_file_operations);
>  #endif
> -	return 0;
> +	return ret;
>  }
>  module_init(setup_vmstat)
>
>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC PATCH] mm: move pcp and lru-pcp drainging into vmstat_wq

[Mel Gorman]

On Tue, Feb 07, 2017 at 10:09:08PM +0100, Michal Hocko wrote:
> From: Michal Hocko <mhocko@suse.com>
> 
> We currently have 2 specific WQ_RECLAIM workqueues. One for updating
> pcp stats vmstat_wq and one dedicated to drain per cpu lru caches. This
> seems more than necessary because both can run on a single WQ. Both
> do not block on locks requiring a memory allocation nor perform any
> allocations themselves. We will save one rescuer thread this way.
> 

True.

> On the other hand drain_all_pages queues work on the system wq which
> doesn't have rescuer and so this depend on memory allocation (when all
> workers are stuck allocating and new ones cannot be created). This is
> not critical as there should be somebody invoking the OOM killer (e.g.
> the forking worker) and get the situation unstuck and eventually
> performs the draining. Quite annoying though. This worker should be
> using WQ_RECLAIM as well. We can reuse the same one as for lru draining
> and vmstat.
> 

That was still debatable which is why I didn't go that route. The drain
itself is unlikely to fix anything with the possible exception of high-order
pages. There are just too many reasons why direct reclaim can return 0
reclaimed pages making the drain is redundant but I couldn't decide what
a better alternative would be and more importantly, how to measure it.
The fact it allocates in that path is currently unfortunate but I couldn't
convince myself it deserved a dedicated rescuer.

I don't object to it being actually moved. I have a slight concern that
it could somehow starve a vmstat update while frequent drains happen
during reclaim though which potentially compounds the problem. It could
be offset by a variety of other factors but if it ever is an issue,
it'll show up and the paths that really matter check the vmstats
directly instead of waiting for an update.

> Suggested-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Signed-off-by: Michal Hocko <mhocko@suse.com>
> ---
> 
> Hi,
> Tetsuo has noted that drain_all_pages doesn't use WQ_RECLAIM [1]
> and asked whether we can move the worker to the vmstat_wq which is
> WQ_RECLAIM. I think the deadlock he has described shouldn't happen but
> it would be really better to have the rescuer. I also think that we do
> not really need 2 or more workqueues and also pull lru draining in.
> 
> What do you think? Please note I haven't tested it yet.
> 

As an aside, the LRU drain could also avoid a get_online_cpus() which is
surprisingly heavy handed for an operation that can happen quite
frequently during compaction or migration. Maybe not enough to make a
big deal of but it's relatively low hanging fruit.

The altering of the return value in setup_vmstat was mildly surprising as
it increases the severity of registering the vmstat callback for memory
hotplug so maybe split that out and appears unrelated.

It also feels like vmstat is now a misleading name for something that
handles vmstat, lru drains and per-cpu drains but that's cosmetic.

Fundamentally I have nothing against the patch.

-- 
Mel Gorman
SUSE Labs

Re: [RFC PATCH] mm: move pcp and lru-pcp drainging into vmstat_wq

[Mel Gorman]

On Tue, Feb 07, 2017 at 10:09:08PM +0100, Michal Hocko wrote:
> From: Michal Hocko <mhocko@suse.com>
> 
> We currently have 2 specific WQ_RECLAIM workqueues. One for updating
> pcp stats vmstat_wq and one dedicated to drain per cpu lru caches. This
> seems more than necessary because both can run on a single WQ. Both
> do not block on locks requiring a memory allocation nor perform any
> allocations themselves. We will save one rescuer thread this way.
> 

True.

> On the other hand drain_all_pages queues work on the system wq which
> doesn't have rescuer and so this depend on memory allocation (when all
> workers are stuck allocating and new ones cannot be created). This is
> not critical as there should be somebody invoking the OOM killer (e.g.
> the forking worker) and get the situation unstuck and eventually
> performs the draining. Quite annoying though. This worker should be
> using WQ_RECLAIM as well. We can reuse the same one as for lru draining
> and vmstat.
> 

That was still debatable which is why I didn't go that route. The drain
itself is unlikely to fix anything with the possible exception of high-order
pages. There are just too many reasons why direct reclaim can return 0
reclaimed pages making the drain is redundant but I couldn't decide what
a better alternative would be and more importantly, how to measure it.
The fact it allocates in that path is currently unfortunate but I couldn't
convince myself it deserved a dedicated rescuer.

I don't object to it being actually moved. I have a slight concern that
it could somehow starve a vmstat update while frequent drains happen
during reclaim though which potentially compounds the problem. It could
be offset by a variety of other factors but if it ever is an issue,
it'll show up and the paths that really matter check the vmstats
directly instead of waiting for an update.

> Suggested-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Signed-off-by: Michal Hocko <mhocko@suse.com>
> ---
> 
> Hi,
> Tetsuo has noted that drain_all_pages doesn't use WQ_RECLAIM [1]
> and asked whether we can move the worker to the vmstat_wq which is
> WQ_RECLAIM. I think the deadlock he has described shouldn't happen but
> it would be really better to have the rescuer. I also think that we do
> not really need 2 or more workqueues and also pull lru draining in.
> 
> What do you think? Please note I haven't tested it yet.
> 

As an aside, the LRU drain could also avoid a get_online_cpus() which is
surprisingly heavy handed for an operation that can happen quite
frequently during compaction or migration. Maybe not enough to make a
big deal of but it's relatively low hanging fruit.

The altering of the return value in setup_vmstat was mildly surprising as
it increases the severity of registering the vmstat callback for memory
hotplug so maybe split that out and appears unrelated.

It also feels like vmstat is now a misleading name for something that
handles vmstat, lru drains and per-cpu drains but that's cosmetic.

Fundamentally I have nothing against the patch.

-- 
Mel Gorman
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC PATCH] mm: move pcp and lru-pcp drainging into vmstat_wq

[Michal Hocko]

On Wed 08-02-17 11:49:11, Vlastimil Babka wrote:
> On 02/07/2017 10:09 PM, Michal Hocko wrote:
> > From: Michal Hocko <mhocko@suse.com>
> > 
> > We currently have 2 specific WQ_RECLAIM workqueues. One for updating
> > pcp stats vmstat_wq and one dedicated to drain per cpu lru caches. This
> > seems more than necessary because both can run on a single WQ. Both
> > do not block on locks requiring a memory allocation nor perform any
> > allocations themselves. We will save one rescuer thread this way.
> > 
> > On the other hand drain_all_pages queues work on the system wq which
> > doesn't have rescuer and so this depend on memory allocation (when all
> > workers are stuck allocating and new ones cannot be created). This is
> > not critical as there should be somebody invoking the OOM killer (e.g.
> > the forking worker) and get the situation unstuck and eventually
> > performs the draining. Quite annoying though. This worker should be
> > using WQ_RECLAIM as well. We can reuse the same one as for lru draining
> > and vmstat.
> > 
> > Suggested-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> > Signed-off-by: Michal Hocko <mhocko@suse.com>
> > ---
> > 
> > Hi,
> > Tetsuo has noted that drain_all_pages doesn't use WQ_RECLAIM [1]
> > and asked whether we can move the worker to the vmstat_wq which is
> > WQ_RECLAIM. I think the deadlock he has described shouldn't happen but
> > it would be really better to have the rescuer. I also think that we do
> > not really need 2 or more workqueues and also pull lru draining in.
> > 
> > What do you think? Please note I haven't tested it yet.
> 
> Why not, I guess, of course I may be overlooking some subtlety. You could
> have CC'd Tejun and Christoph.

will do on the next submission

> Watch out for the init order though, maybe? Is there no caller of the
> lru/pcp drain before module_init(setup_vmstat) happens?

Hard to tell. I expect that there shouldn't be but I will add
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 0c0a7c38cd91..73018f07bcc9 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -2370,6 +2370,13 @@ void drain_all_pages(struct zone *zone)
 	 */
 	static cpumask_t cpus_with_pcps;
 
+	/*
+	 * Make sure nobody triggers this path before vmstat_wq is fully
+	 * initialized.
+	 */
+	if (WARN_ON(!vmstat_wq))
+		return;
+
 	/* Workqueues cannot recurse */
 	if (current->flags & PF_WQ_WORKER)
 		return;
diff --git a/mm/swap.c b/mm/swap.c
index 23f09d6dd212..39c240fc9d48 100644
--- a/mm/swap.c
+++ b/mm/swap.c
@@ -676,6 +676,13 @@ void lru_add_drain_all(void)
 	static struct cpumask has_work;
 	int cpu;
 
+	/*
+	 * Make sure nobody triggers this path before vmstat_wq is fully
+	 * initialized.
+	 */
+	if (WARN_ON(!vmstat_wq))
+		return;
+
 	mutex_lock(&lock);
 	get_online_cpus();
 	cpumask_clear(&has_work);

to be sure.
[...]

> > @@ -1763,9 +1762,11 @@ static int vmstat_cpu_dead(unsigned int cpu)
> > 
> >  static int __init setup_vmstat(void)
> >  {
> > -#ifdef CONFIG_SMP
> > -	int ret;
> > +	int ret = 0;
> > +
> > +	vmstat_wq = alloc_workqueue("vmstat", WQ_FREEZABLE|WQ_MEM_RECLAIM, 0);
> 
> Did you want to set ret to -ENOMEM if the alloc fails, or something?
> Otherwise I don't see why the changes.

no I just didn't want to get defined but not used warning for
CONFIG_SMP=n

> 
> > 
> > +#ifdef CONFIG_SMP
> >  	ret = cpuhp_setup_state_nocalls(CPUHP_MM_VMSTAT_DEAD, "mm/vmstat:dead",
> >  					NULL, vmstat_cpu_dead);
> >  	if (ret < 0)
> > @@ -1789,7 +1790,7 @@ static int __init setup_vmstat(void)
> >  	proc_create("vmstat", S_IRUGO, NULL, &proc_vmstat_file_operations);
> >  	proc_create("zoneinfo", S_IRUGO, NULL, &proc_zoneinfo_file_operations);
> >  #endif
> > -	return 0;
> > +	return ret;
> >  }
> >  module_init(setup_vmstat)
> > 
> > 

-- 
Michal Hocko
SUSE Labs

Re: [RFC PATCH] mm: move pcp and lru-pcp drainging into vmstat_wq

[Michal Hocko]

On Wed 08-02-17 11:49:11, Vlastimil Babka wrote:
> On 02/07/2017 10:09 PM, Michal Hocko wrote:
> > From: Michal Hocko <mhocko@suse.com>
> > 
> > We currently have 2 specific WQ_RECLAIM workqueues. One for updating
> > pcp stats vmstat_wq and one dedicated to drain per cpu lru caches. This
> > seems more than necessary because both can run on a single WQ. Both
> > do not block on locks requiring a memory allocation nor perform any
> > allocations themselves. We will save one rescuer thread this way.
> > 
> > On the other hand drain_all_pages queues work on the system wq which
> > doesn't have rescuer and so this depend on memory allocation (when all
> > workers are stuck allocating and new ones cannot be created). This is
> > not critical as there should be somebody invoking the OOM killer (e.g.
> > the forking worker) and get the situation unstuck and eventually
> > performs the draining. Quite annoying though. This worker should be
> > using WQ_RECLAIM as well. We can reuse the same one as for lru draining
> > and vmstat.
> > 
> > Suggested-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> > Signed-off-by: Michal Hocko <mhocko@suse.com>
> > ---
> > 
> > Hi,
> > Tetsuo has noted that drain_all_pages doesn't use WQ_RECLAIM [1]
> > and asked whether we can move the worker to the vmstat_wq which is
> > WQ_RECLAIM. I think the deadlock he has described shouldn't happen but
> > it would be really better to have the rescuer. I also think that we do
> > not really need 2 or more workqueues and also pull lru draining in.
> > 
> > What do you think? Please note I haven't tested it yet.
> 
> Why not, I guess, of course I may be overlooking some subtlety. You could
> have CC'd Tejun and Christoph.

will do on the next submission

> Watch out for the init order though, maybe? Is there no caller of the
> lru/pcp drain before module_init(setup_vmstat) happens?

Hard to tell. I expect that there shouldn't be but I will add
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 0c0a7c38cd91..73018f07bcc9 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -2370,6 +2370,13 @@ void drain_all_pages(struct zone *zone)
 	 */
 	static cpumask_t cpus_with_pcps;
 
+	/*
+	 * Make sure nobody triggers this path before vmstat_wq is fully
+	 * initialized.
+	 */
+	if (WARN_ON(!vmstat_wq))
+		return;
+
 	/* Workqueues cannot recurse */
 	if (current->flags & PF_WQ_WORKER)
 		return;
diff --git a/mm/swap.c b/mm/swap.c
index 23f09d6dd212..39c240fc9d48 100644
--- a/mm/swap.c
+++ b/mm/swap.c
@@ -676,6 +676,13 @@ void lru_add_drain_all(void)
 	static struct cpumask has_work;
 	int cpu;
 
+	/*
+	 * Make sure nobody triggers this path before vmstat_wq is fully
+	 * initialized.
+	 */
+	if (WARN_ON(!vmstat_wq))
+		return;
+
 	mutex_lock(&lock);
 	get_online_cpus();
 	cpumask_clear(&has_work);

to be sure.
[...]

> > @@ -1763,9 +1762,11 @@ static int vmstat_cpu_dead(unsigned int cpu)
> > 
> >  static int __init setup_vmstat(void)
> >  {
> > -#ifdef CONFIG_SMP
> > -	int ret;
> > +	int ret = 0;
> > +
> > +	vmstat_wq = alloc_workqueue("vmstat", WQ_FREEZABLE|WQ_MEM_RECLAIM, 0);
> 
> Did you want to set ret to -ENOMEM if the alloc fails, or something?
> Otherwise I don't see why the changes.

no I just didn't want to get defined but not used warning for
CONFIG_SMP=n

> 
> > 
> > +#ifdef CONFIG_SMP
> >  	ret = cpuhp_setup_state_nocalls(CPUHP_MM_VMSTAT_DEAD, "mm/vmstat:dead",
> >  					NULL, vmstat_cpu_dead);
> >  	if (ret < 0)
> > @@ -1789,7 +1790,7 @@ static int __init setup_vmstat(void)
> >  	proc_create("vmstat", S_IRUGO, NULL, &proc_vmstat_file_operations);
> >  	proc_create("zoneinfo", S_IRUGO, NULL, &proc_zoneinfo_file_operations);
> >  #endif
> > -	return 0;
> > +	return ret;
> >  }
> >  module_init(setup_vmstat)
> > 
> > 

-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC PATCH] mm: move pcp and lru-pcp drainging into vmstat_wq

[Michal Hocko]

On Wed 08-02-17 10:53:34, Mel Gorman wrote:
> On Tue, Feb 07, 2017 at 10:09:08PM +0100, Michal Hocko wrote:
> > From: Michal Hocko <mhocko@suse.com>
> > 
> > We currently have 2 specific WQ_RECLAIM workqueues. One for updating
> > pcp stats vmstat_wq and one dedicated to drain per cpu lru caches. This
> > seems more than necessary because both can run on a single WQ. Both
> > do not block on locks requiring a memory allocation nor perform any
> > allocations themselves. We will save one rescuer thread this way.
> > 
> 
> True.
> 
> > On the other hand drain_all_pages queues work on the system wq which
> > doesn't have rescuer and so this depend on memory allocation (when all
> > workers are stuck allocating and new ones cannot be created). This is
> > not critical as there should be somebody invoking the OOM killer (e.g.
> > the forking worker) and get the situation unstuck and eventually
> > performs the draining. Quite annoying though. This worker should be
> > using WQ_RECLAIM as well. We can reuse the same one as for lru draining
> > and vmstat.
> > 
> 
> That was still debatable which is why I didn't go that route. The drain
> itself is unlikely to fix anything with the possible exception of high-order
> pages. There are just too many reasons why direct reclaim can return 0
> reclaimed pages making the drain is redundant but I couldn't decide what
> a better alternative would be and more importantly, how to measure it.
> The fact it allocates in that path is currently unfortunate but I couldn't
> convince myself it deserved a dedicated rescuer.

agreed

> I don't object to it being actually moved. I have a slight concern that
> it could somehow starve a vmstat update while frequent drains happen
> during reclaim though which potentially compounds the problem. It could
> be offset by a variety of other factors but if it ever is an issue,
> it'll show up and the paths that really matter check the vmstats
> directly instead of waiting for an update.

vmstat updates can tolared delays, that's we we are using deferable
scheduling in the first place so I am not really worried about that. Any
user which needs a better precision should use *_snapshot API.

> > Suggested-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> > Signed-off-by: Michal Hocko <mhocko@suse.com>
> > ---
> > 
> > Hi,
> > Tetsuo has noted that drain_all_pages doesn't use WQ_RECLAIM [1]
> > and asked whether we can move the worker to the vmstat_wq which is
> > WQ_RECLAIM. I think the deadlock he has described shouldn't happen but
> > it would be really better to have the rescuer. I also think that we do
> > not really need 2 or more workqueues and also pull lru draining in.
> > 
> > What do you think? Please note I haven't tested it yet.
> > 
> 
> As an aside, the LRU drain could also avoid a get_online_cpus() which is
> surprisingly heavy handed for an operation that can happen quite
> frequently during compaction or migration. Maybe not enough to make a
> big deal of but it's relatively low hanging fruit.

Yeah, this is sitting on my todo list already, I just didn't give it a
priority.

> The altering of the return value in setup_vmstat was mildly surprising as
> it increases the severity of registering the vmstat callback for memory
> hotplug so maybe split that out and appears unrelated.

not sure I understand. What do you mean?

> It also feels like vmstat is now a misleading name for something that
> handles vmstat, lru drains and per-cpu drains but that's cosmetic.

yeah a better name sounds like a good thing. mm_nonblock_wq?

> Fundamentally I have nothing against the patch.

Thanks for the review. I will sit on it and give it some testing to see
how it behaves and post after I get back from vacation (after 20th)
-- 
Michal Hocko
SUSE Labs

Re: [RFC PATCH] mm: move pcp and lru-pcp drainging into vmstat_wq

[Michal Hocko]

On Wed 08-02-17 10:53:34, Mel Gorman wrote:
> On Tue, Feb 07, 2017 at 10:09:08PM +0100, Michal Hocko wrote:
> > From: Michal Hocko <mhocko@suse.com>
> > 
> > We currently have 2 specific WQ_RECLAIM workqueues. One for updating
> > pcp stats vmstat_wq and one dedicated to drain per cpu lru caches. This
> > seems more than necessary because both can run on a single WQ. Both
> > do not block on locks requiring a memory allocation nor perform any
> > allocations themselves. We will save one rescuer thread this way.
> > 
> 
> True.
> 
> > On the other hand drain_all_pages queues work on the system wq which
> > doesn't have rescuer and so this depend on memory allocation (when all
> > workers are stuck allocating and new ones cannot be created). This is
> > not critical as there should be somebody invoking the OOM killer (e.g.
> > the forking worker) and get the situation unstuck and eventually
> > performs the draining. Quite annoying though. This worker should be
> > using WQ_RECLAIM as well. We can reuse the same one as for lru draining
> > and vmstat.
> > 
> 
> That was still debatable which is why I didn't go that route. The drain
> itself is unlikely to fix anything with the possible exception of high-order
> pages. There are just too many reasons why direct reclaim can return 0
> reclaimed pages making the drain is redundant but I couldn't decide what
> a better alternative would be and more importantly, how to measure it.
> The fact it allocates in that path is currently unfortunate but I couldn't
> convince myself it deserved a dedicated rescuer.

agreed

> I don't object to it being actually moved. I have a slight concern that
> it could somehow starve a vmstat update while frequent drains happen
> during reclaim though which potentially compounds the problem. It could
> be offset by a variety of other factors but if it ever is an issue,
> it'll show up and the paths that really matter check the vmstats
> directly instead of waiting for an update.

vmstat updates can tolared delays, that's we we are using deferable
scheduling in the first place so I am not really worried about that. Any
user which needs a better precision should use *_snapshot API.

> > Suggested-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> > Signed-off-by: Michal Hocko <mhocko@suse.com>
> > ---
> > 
> > Hi,
> > Tetsuo has noted that drain_all_pages doesn't use WQ_RECLAIM [1]
> > and asked whether we can move the worker to the vmstat_wq which is
> > WQ_RECLAIM. I think the deadlock he has described shouldn't happen but
> > it would be really better to have the rescuer. I also think that we do
> > not really need 2 or more workqueues and also pull lru draining in.
> > 
> > What do you think? Please note I haven't tested it yet.
> > 
> 
> As an aside, the LRU drain could also avoid a get_online_cpus() which is
> surprisingly heavy handed for an operation that can happen quite
> frequently during compaction or migration. Maybe not enough to make a
> big deal of but it's relatively low hanging fruit.

Yeah, this is sitting on my todo list already, I just didn't give it a
priority.

> The altering of the return value in setup_vmstat was mildly surprising as
> it increases the severity of registering the vmstat callback for memory
> hotplug so maybe split that out and appears unrelated.

not sure I understand. What do you mean?

> It also feels like vmstat is now a misleading name for something that
> handles vmstat, lru drains and per-cpu drains but that's cosmetic.

yeah a better name sounds like a good thing. mm_nonblock_wq?

> Fundamentally I have nothing against the patch.

Thanks for the review. I will sit on it and give it some testing to see
how it behaves and post after I get back from vacation (after 20th)
-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC PATCH] mm: move pcp and lru-pcp drainging into vmstat_wq

[Mel Gorman]

On Wed, Feb 08, 2017 at 01:03:55PM +0100, Michal Hocko wrote:
> > I don't object to it being actually moved. I have a slight concern that
> > it could somehow starve a vmstat update while frequent drains happen
> > during reclaim though which potentially compounds the problem. It could
> > be offset by a variety of other factors but if it ever is an issue,
> > it'll show up and the paths that really matter check the vmstats
> > directly instead of waiting for an update.
> 
> vmstat updates can tolared delays, that's we we are using deferable
> scheduling in the first place so I am not really worried about that. Any
> user which needs a better precision should use *_snapshot API.
> 

Agreed, we already had cases where deferred vmstat updates had problems
and were resolved by using _snapshot. It's a slight concern only and I'd
be surprised if the _snapshot usage didn't cover it.

> > The altering of the return value in setup_vmstat was mildly surprising as
> > it increases the severity of registering the vmstat callback for memory
> > hotplug so maybe split that out and appears unrelated.
> 
> not sure I understand. What do you mean?
> 

This hunk

@@ -1763,9 +1762,11 @@ static int vmstat_cpu_dead(unsigned int cpu)

 static int __init setup_vmstat(void)
 {
-#ifdef CONFIG_SMP
-       int ret;
+       int ret = 0;
+
+       vmstat_wq = alloc_workqueue("vmstat", WQ_FREEZABLE|WQ_MEM_RECLAIM, 0);

+#ifdef CONFIG_SMP
        ret = cpuhp_setup_state_nocalls(CPUHP_MM_VMSTAT_DEAD, "mm/vmstat:dead",
                                        NULL, vmstat_cpu_dead);
        if (ret < 0)
@@ -1789,7 +1790,7 @@ static int __init setup_vmstat(void)
        proc_create("vmstat", S_IRUGO, NULL, &proc_vmstat_file_operations);
        proc_create("zoneinfo", S_IRUGO, NULL, &proc_zoneinfo_file_operations);
 #endif
-       return 0;
+       return ret;


A failed register of vmstat_cpu_dead is returning the failure code in an
init function now. Chances are it'll never hit but it didn't seem related
to the patches general intent.

> > It also feels like vmstat is now a misleading name for something that
> > handles vmstat, lru drains and per-cpu drains but that's cosmetic.
> 
> yeah a better name sounds like a good thing. mm_nonblock_wq?
> 

it's not always non-blocking. Maybe mm_percpu_wq to describev a workqueue
that handles a variety of MM-related per-cpu updates?

-- 
Mel Gorman
SUSE Labs

Re: [RFC PATCH] mm: move pcp and lru-pcp drainging into vmstat_wq

[Mel Gorman]

On Wed, Feb 08, 2017 at 01:03:55PM +0100, Michal Hocko wrote:
> > I don't object to it being actually moved. I have a slight concern that
> > it could somehow starve a vmstat update while frequent drains happen
> > during reclaim though which potentially compounds the problem. It could
> > be offset by a variety of other factors but if it ever is an issue,
> > it'll show up and the paths that really matter check the vmstats
> > directly instead of waiting for an update.
> 
> vmstat updates can tolared delays, that's we we are using deferable
> scheduling in the first place so I am not really worried about that. Any
> user which needs a better precision should use *_snapshot API.
> 

Agreed, we already had cases where deferred vmstat updates had problems
and were resolved by using _snapshot. It's a slight concern only and I'd
be surprised if the _snapshot usage didn't cover it.

> > The altering of the return value in setup_vmstat was mildly surprising as
> > it increases the severity of registering the vmstat callback for memory
> > hotplug so maybe split that out and appears unrelated.
> 
> not sure I understand. What do you mean?
> 

This hunk

@@ -1763,9 +1762,11 @@ static int vmstat_cpu_dead(unsigned int cpu)

 static int __init setup_vmstat(void)
 {
-#ifdef CONFIG_SMP
-       int ret;
+       int ret = 0;
+
+       vmstat_wq = alloc_workqueue("vmstat", WQ_FREEZABLE|WQ_MEM_RECLAIM, 0);

+#ifdef CONFIG_SMP
        ret = cpuhp_setup_state_nocalls(CPUHP_MM_VMSTAT_DEAD, "mm/vmstat:dead",
                                        NULL, vmstat_cpu_dead);
        if (ret < 0)
@@ -1789,7 +1790,7 @@ static int __init setup_vmstat(void)
        proc_create("vmstat", S_IRUGO, NULL, &proc_vmstat_file_operations);
        proc_create("zoneinfo", S_IRUGO, NULL, &proc_zoneinfo_file_operations);
 #endif
-       return 0;
+       return ret;


A failed register of vmstat_cpu_dead is returning the failure code in an
init function now. Chances are it'll never hit but it didn't seem related
to the patches general intent.

> > It also feels like vmstat is now a misleading name for something that
> > handles vmstat, lru drains and per-cpu drains but that's cosmetic.
> 
> yeah a better name sounds like a good thing. mm_nonblock_wq?
> 

it's not always non-blocking. Maybe mm_percpu_wq to describev a workqueue
that handles a variety of MM-related per-cpu updates?

-- 
Mel Gorman
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC PATCH] mm: move pcp and lru-pcp drainging into vmstat_wq

[Tetsuo Handa]

Mel Gorman wrote:
> > > It also feels like vmstat is now a misleading name for something that
> > > handles vmstat, lru drains and per-cpu drains but that's cosmetic.
> > 
> > yeah a better name sounds like a good thing. mm_nonblock_wq?
> > 
> 
> it's not always non-blocking. Maybe mm_percpu_wq to describev a workqueue
> that handles a variety of MM-related per-cpu updates?
> 

Why not make it global like ones created by workqueue_init_early() ?

Re: [RFC PATCH] mm: move pcp and lru-pcp drainging into vmstat_wq

[Tetsuo Handa]

Mel Gorman wrote:
> > > It also feels like vmstat is now a misleading name for something that
> > > handles vmstat, lru drains and per-cpu drains but that's cosmetic.
> > 
> > yeah a better name sounds like a good thing. mm_nonblock_wq?
> > 
> 
> it's not always non-blocking. Maybe mm_percpu_wq to describev a workqueue
> that handles a variety of MM-related per-cpu updates?
> 

Why not make it global like ones created by workqueue_init_early() ?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC PATCH] mm: move pcp and lru-pcp drainging into vmstat_wq

[Michal Hocko]

On Wed 08-02-17 12:31:13, Mel Gorman wrote:
> On Wed, Feb 08, 2017 at 01:03:55PM +0100, Michal Hocko wrote:
> > > I don't object to it being actually moved. I have a slight concern that
> > > it could somehow starve a vmstat update while frequent drains happen
> > > during reclaim though which potentially compounds the problem. It could
> > > be offset by a variety of other factors but if it ever is an issue,
> > > it'll show up and the paths that really matter check the vmstats
> > > directly instead of waiting for an update.
> > 
> > vmstat updates can tolared delays, that's we we are using deferable
> > scheduling in the first place so I am not really worried about that. Any
> > user which needs a better precision should use *_snapshot API.
> > 
> 
> Agreed, we already had cases where deferred vmstat updates had problems
> and were resolved by using _snapshot. It's a slight concern only and I'd
> be surprised if the _snapshot usage didn't cover it.
> 
> > > The altering of the return value in setup_vmstat was mildly surprising as
> > > it increases the severity of registering the vmstat callback for memory
> > > hotplug so maybe split that out and appears unrelated.
> > 
> > not sure I understand. What do you mean?
> > 
> 
> This hunk
> 
> @@ -1763,9 +1762,11 @@ static int vmstat_cpu_dead(unsigned int cpu)
> 
>  static int __init setup_vmstat(void)
>  {
> -#ifdef CONFIG_SMP
> -       int ret;
> +       int ret = 0;
> +
> +       vmstat_wq = alloc_workqueue("vmstat", WQ_FREEZABLE|WQ_MEM_RECLAIM, 0);
> 
> +#ifdef CONFIG_SMP
>         ret = cpuhp_setup_state_nocalls(CPUHP_MM_VMSTAT_DEAD, "mm/vmstat:dead",
>                                         NULL, vmstat_cpu_dead);
>         if (ret < 0)
> @@ -1789,7 +1790,7 @@ static int __init setup_vmstat(void)
>         proc_create("vmstat", S_IRUGO, NULL, &proc_vmstat_file_operations);
>         proc_create("zoneinfo", S_IRUGO, NULL, &proc_zoneinfo_file_operations);
>  #endif
> -       return 0;
> +       return ret;
> 
> 
> A failed register of vmstat_cpu_dead is returning the failure code in an
> init function now. Chances are it'll never hit but it didn't seem related
> to the patches general intent.

Ohh, I see now. I will keep the original behavior.

> > > It also feels like vmstat is now a misleading name for something that
> > > handles vmstat, lru drains and per-cpu drains but that's cosmetic.
> > 
> > yeah a better name sounds like a good thing. mm_nonblock_wq?
> > 
> 
> it's not always non-blocking. Maybe mm_percpu_wq to describev a workqueue
> that handles a variety of MM-related per-cpu updates?

Why not, I do not have a strong preference. The WQ is already documented
for its requirements on workers so the name doesn't really have to be
explicit about blocking on allocations.

Thanks!

-- 
Michal Hocko
SUSE Labs

Re: [RFC PATCH] mm: move pcp and lru-pcp drainging into vmstat_wq

[Michal Hocko]

On Wed 08-02-17 12:31:13, Mel Gorman wrote:
> On Wed, Feb 08, 2017 at 01:03:55PM +0100, Michal Hocko wrote:
> > > I don't object to it being actually moved. I have a slight concern that
> > > it could somehow starve a vmstat update while frequent drains happen
> > > during reclaim though which potentially compounds the problem. It could
> > > be offset by a variety of other factors but if it ever is an issue,
> > > it'll show up and the paths that really matter check the vmstats
> > > directly instead of waiting for an update.
> > 
> > vmstat updates can tolared delays, that's we we are using deferable
> > scheduling in the first place so I am not really worried about that. Any
> > user which needs a better precision should use *_snapshot API.
> > 
> 
> Agreed, we already had cases where deferred vmstat updates had problems
> and were resolved by using _snapshot. It's a slight concern only and I'd
> be surprised if the _snapshot usage didn't cover it.
> 
> > > The altering of the return value in setup_vmstat was mildly surprising as
> > > it increases the severity of registering the vmstat callback for memory
> > > hotplug so maybe split that out and appears unrelated.
> > 
> > not sure I understand. What do you mean?
> > 
> 
> This hunk
> 
> @@ -1763,9 +1762,11 @@ static int vmstat_cpu_dead(unsigned int cpu)
> 
>  static int __init setup_vmstat(void)
>  {
> -#ifdef CONFIG_SMP
> -       int ret;
> +       int ret = 0;
> +
> +       vmstat_wq = alloc_workqueue("vmstat", WQ_FREEZABLE|WQ_MEM_RECLAIM, 0);
> 
> +#ifdef CONFIG_SMP
>         ret = cpuhp_setup_state_nocalls(CPUHP_MM_VMSTAT_DEAD, "mm/vmstat:dead",
>                                         NULL, vmstat_cpu_dead);
>         if (ret < 0)
> @@ -1789,7 +1790,7 @@ static int __init setup_vmstat(void)
>         proc_create("vmstat", S_IRUGO, NULL, &proc_vmstat_file_operations);
>         proc_create("zoneinfo", S_IRUGO, NULL, &proc_zoneinfo_file_operations);
>  #endif
> -       return 0;
> +       return ret;
> 
> 
> A failed register of vmstat_cpu_dead is returning the failure code in an
> init function now. Chances are it'll never hit but it didn't seem related
> to the patches general intent.

Ohh, I see now. I will keep the original behavior.

> > > It also feels like vmstat is now a misleading name for something that
> > > handles vmstat, lru drains and per-cpu drains but that's cosmetic.
> > 
> > yeah a better name sounds like a good thing. mm_nonblock_wq?
> > 
> 
> it's not always non-blocking. Maybe mm_percpu_wq to describev a workqueue
> that handles a variety of MM-related per-cpu updates?

Why not, I do not have a strong preference. The WQ is already documented
for its requirements on workers so the name doesn't really have to be
explicit about blocking on allocations.

Thanks!

-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC PATCH] mm: move pcp and lru-pcp drainging into vmstat_wq

[Michal Hocko]

On Wed 08-02-17 21:44:36, Tetsuo Handa wrote:
> Mel Gorman wrote:
> > > > It also feels like vmstat is now a misleading name for something that
> > > > handles vmstat, lru drains and per-cpu drains but that's cosmetic.
> > > 
> > > yeah a better name sounds like a good thing. mm_nonblock_wq?
> > > 
> > 
> > it's not always non-blocking. Maybe mm_percpu_wq to describev a workqueue
> > that handles a variety of MM-related per-cpu updates?
> > 
> 
> Why not make it global like ones created by workqueue_init_early() ?

I can see alloc_workqueue_attrs in that path so we can hit the page
allocator and if unlucky try to drain_all_pages. We might have more
even before this. So I think we still need a check for the WQ being
initialized already. I do not have a strong preference to when to
allocate it. Moving the initialization out to workqueue_init_early wold
mean that the WQ would have to be visible outside of the mm proper which
is not ideal. We can live with that, though, so I will move it there if
this is a prevalent opinion.
-- 
Michal Hocko
SUSE Labs

Re: [RFC PATCH] mm: move pcp and lru-pcp drainging into vmstat_wq

[Michal Hocko]

On Wed 08-02-17 21:44:36, Tetsuo Handa wrote:
> Mel Gorman wrote:
> > > > It also feels like vmstat is now a misleading name for something that
> > > > handles vmstat, lru drains and per-cpu drains but that's cosmetic.
> > > 
> > > yeah a better name sounds like a good thing. mm_nonblock_wq?
> > > 
> > 
> > it's not always non-blocking. Maybe mm_percpu_wq to describev a workqueue
> > that handles a variety of MM-related per-cpu updates?
> > 
> 
> Why not make it global like ones created by workqueue_init_early() ?

I can see alloc_workqueue_attrs in that path so we can hit the page
allocator and if unlucky try to drain_all_pages. We might have more
even before this. So I think we still need a check for the WQ being
initialized already. I do not have a strong preference to when to
allocate it. Moving the initialization out to workqueue_init_early wold
mean that the WQ would have to be visible outside of the mm proper which
is not ideal. We can live with that, though, so I will move it there if
this is a prevalent opinion.
-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>