* [PATCH] mm, page_alloc: Use static global work_struct for draining per-cpu pages
@ 2017-01-25 8:30 Mel Gorman
2017-01-25 9:24 ` Vlastimil Babka
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Mel Gorman @ 2017-01-25 8:30 UTC (permalink / raw)
To: Andrew Morton
Cc: Tejun Heo, Vlastimil Babka, Linux Kernel, Linux-MM, Hillf Danton,
Jesper Dangaard Brouer
As suggested by Vlastimil Babka and Tejun Heo, this patch uses a static
work_struct to co-ordinate the draining of per-cpu pages on the workqueue.
Only one task can drain at a time but this is better than the previous
scheme that allowed multiple tasks to send IPIs at a time.
One consideration is whether parallel requests should synchronise against
each other. This patch does not synchronise for a global drain as the common
case for such callers is expected to be multiple parallel direct reclaimers
competing for pages when the watermark is close to min. Draining the per-cpu
list is unlikely to make much progress and serialising the drain is of
dubious merit. Drains are synchonrised for callers such as memory hotplug
and CMA that care about the drain being complete when the function returns.
Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
---
mm/page_alloc.c | 41 +++++++++++++++++++++++------------------
1 file changed, 23 insertions(+), 18 deletions(-)
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index e87508ffa759..da6be2a5ff7a 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -92,6 +92,10 @@ EXPORT_PER_CPU_SYMBOL(_numa_mem_);
int _node_numa_mem_[MAX_NUMNODES];
#endif
+/* work_structs for global per-cpu drains */
+DEFINE_MUTEX(pcpu_drain_mutex);
+DEFINE_PER_CPU(struct work_struct, pcpu_drain);
+
#ifdef CONFIG_GCC_PLUGIN_LATENT_ENTROPY
volatile unsigned long latent_entropy __latent_entropy;
EXPORT_SYMBOL(latent_entropy);
@@ -2351,7 +2355,6 @@ static void drain_local_pages_wq(struct work_struct *work)
*/
void drain_all_pages(struct zone *zone)
{
- struct work_struct __percpu *works;
int cpu;
/*
@@ -2365,11 +2368,21 @@ void drain_all_pages(struct zone *zone)
return;
/*
+ * Do not drain if one is already in progress unless it's specific to
+ * a zone. Such callers are primarily CMA and memory hotplug and need
+ * the drain to be complete when the call returns.
+ */
+ if (unlikely(!mutex_trylock(&pcpu_drain_mutex))) {
+ if (!zone)
+ return;
+ mutex_lock(&pcpu_drain_mutex);
+ }
+
+ /*
* As this can be called from reclaim context, do not reenter reclaim.
* An allocation failure can be handled, it's simply slower
*/
get_online_cpus();
- works = alloc_percpu_gfp(struct work_struct, GFP_ATOMIC);
/*
* We don't care about racing with CPU hotplug event
@@ -2402,24 +2415,16 @@ void drain_all_pages(struct zone *zone)
cpumask_clear_cpu(cpu, &cpus_with_pcps);
}
- if (works) {
- for_each_cpu(cpu, &cpus_with_pcps) {
- struct work_struct *work = per_cpu_ptr(works, cpu);
- INIT_WORK(work, drain_local_pages_wq);
- schedule_work_on(cpu, work);
- }
- for_each_cpu(cpu, &cpus_with_pcps)
- flush_work(per_cpu_ptr(works, cpu));
- } else {
- for_each_cpu(cpu, &cpus_with_pcps) {
- struct work_struct work;
-
- INIT_WORK(&work, drain_local_pages_wq);
- schedule_work_on(cpu, &work);
- flush_work(&work);
- }
+ 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);
}
+ for_each_cpu(cpu, &cpus_with_pcps)
+ flush_work(per_cpu_ptr(&pcpu_drain, cpu));
+
put_online_cpus();
+ mutex_unlock(&pcpu_drain_mutex);
}
#ifdef CONFIG_HIBERNATION
--
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>
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] mm, page_alloc: Use static global work_struct for draining per-cpu pages
2017-01-25 8:30 [PATCH] mm, page_alloc: Use static global work_struct for draining per-cpu pages Mel Gorman
@ 2017-01-25 9:24 ` Vlastimil Babka
2017-01-25 9:33 ` Hillf Danton
2017-01-26 0:08 ` Andrew Morton
2 siblings, 0 replies; 6+ messages in thread
From: Vlastimil Babka @ 2017-01-25 9:24 UTC (permalink / raw)
To: Mel Gorman, Andrew Morton
Cc: Tejun Heo, Linux Kernel, Linux-MM, Hillf Danton,
Jesper Dangaard Brouer
On 01/25/2017 09:30 AM, Mel Gorman wrote:
> As suggested by Vlastimil Babka and Tejun Heo, this patch uses a static
> work_struct to co-ordinate the draining of per-cpu pages on the workqueue.
> Only one task can drain at a time but this is better than the previous
> scheme that allowed multiple tasks to send IPIs at a time.
>
> One consideration is whether parallel requests should synchronise against
> each other. This patch does not synchronise for a global drain as the common
> case for such callers is expected to be multiple parallel direct reclaimers
> competing for pages when the watermark is close to min. Draining the per-cpu
> list is unlikely to make much progress and serialising the drain is of
> dubious merit. Drains are synchonrised for callers such as memory hotplug
> and CMA that care about the drain being complete when the function returns.
>
> Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
Acked-by: Vlastimil Babka <vbabka@suse.cz>
--
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>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mm, page_alloc: Use static global work_struct for draining per-cpu pages
2017-01-25 8:30 [PATCH] mm, page_alloc: Use static global work_struct for draining per-cpu pages Mel Gorman
2017-01-25 9:24 ` Vlastimil Babka
@ 2017-01-25 9:33 ` Hillf Danton
2017-01-26 0:08 ` Andrew Morton
2 siblings, 0 replies; 6+ messages in thread
From: Hillf Danton @ 2017-01-25 9:33 UTC (permalink / raw)
To: 'Mel Gorman', 'Andrew Morton'
Cc: 'Tejun Heo', 'Vlastimil Babka',
'Linux Kernel', 'Linux-MM',
'Jesper Dangaard Brouer'
On Wednesday, January 25, 2017 4:31 PM Mel Gorman wrote:
>
> As suggested by Vlastimil Babka and Tejun Heo, this patch uses a static
> work_struct to co-ordinate the draining of per-cpu pages on the workqueue.
> Only one task can drain at a time but this is better than the previous
> scheme that allowed multiple tasks to send IPIs at a time.
>
> One consideration is whether parallel requests should synchronise against
> each other. This patch does not synchronise for a global drain as the common
> case for such callers is expected to be multiple parallel direct reclaimers
> competing for pages when the watermark is close to min. Draining the per-cpu
> list is unlikely to make much progress and serialising the drain is of
> dubious merit. Drains are synchonrised for callers such as memory hotplug
> and CMA that care about the drain being complete when the function returns.
>
> Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
> ---
Acked-by: Hillf Danton <hillf.zj@alibaba-inc.com>
--
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>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mm, page_alloc: Use static global work_struct for draining per-cpu pages
2017-01-25 8:30 [PATCH] mm, page_alloc: Use static global work_struct for draining per-cpu pages Mel Gorman
2017-01-25 9:24 ` Vlastimil Babka
2017-01-25 9:33 ` Hillf Danton
@ 2017-01-26 0:08 ` Andrew Morton
2017-01-26 10:47 ` Mel Gorman
2 siblings, 1 reply; 6+ messages in thread
From: Andrew Morton @ 2017-01-26 0:08 UTC (permalink / raw)
To: Mel Gorman
Cc: Tejun Heo, Vlastimil Babka, Linux Kernel, Linux-MM, Hillf Danton,
Jesper Dangaard Brouer
On Wed, 25 Jan 2017 08:30:38 +0000 Mel Gorman <mgorman@techsingularity.net> wrote:
> As suggested by Vlastimil Babka and Tejun Heo, this patch uses a static
> work_struct to co-ordinate the draining of per-cpu pages on the workqueue.
> Only one task can drain at a time but this is better than the previous
> scheme that allowed multiple tasks to send IPIs at a time.
>
> One consideration is whether parallel requests should synchronise against
> each other. This patch does not synchronise for a global drain as the common
> case for such callers is expected to be multiple parallel direct reclaimers
> competing for pages when the watermark is close to min. Draining the per-cpu
> list is unlikely to make much progress and serialising the drain is of
> dubious merit. Drains are synchonrised for callers such as memory hotplug
> and CMA that care about the drain being complete when the function returns.
>
> ...
>
> @@ -2402,24 +2415,16 @@ void drain_all_pages(struct zone *zone)
> cpumask_clear_cpu(cpu, &cpus_with_pcps);
> }
>
> - if (works) {
> - for_each_cpu(cpu, &cpus_with_pcps) {
> - struct work_struct *work = per_cpu_ptr(works, cpu);
> - INIT_WORK(work, drain_local_pages_wq);
> - schedule_work_on(cpu, work);
> - }
> - for_each_cpu(cpu, &cpus_with_pcps)
> - flush_work(per_cpu_ptr(works, cpu));
> - } else {
> - for_each_cpu(cpu, &cpus_with_pcps) {
> - struct work_struct work;
> -
> - INIT_WORK(&work, drain_local_pages_wq);
> - schedule_work_on(cpu, &work);
> - flush_work(&work);
> - }
> + for_each_cpu(cpu, &cpus_with_pcps) {
> + struct work_struct *work = per_cpu_ptr(&pcpu_drain, cpu);
> + INIT_WORK(work, drain_local_pages_wq);
It's strange to repeatedly run INIT_WORK() in this fashion.
Overwriting an atomic_t which should already be zero, initializing a
list_head which should already be in the initialized state...
Can we instead do this a single time in init code?
> + schedule_work_on(cpu, work);
> }
> + for_each_cpu(cpu, &cpus_with_pcps)
> + flush_work(per_cpu_ptr(&pcpu_drain, cpu));
> +
> put_online_cpus();
> + mutex_unlock(&pcpu_drain_mutex);
> }
--
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>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] mm, page_alloc: Use static global work_struct for draining per-cpu pages
2017-01-26 0:08 ` Andrew Morton
@ 2017-01-26 10:47 ` Mel Gorman
2017-01-26 19:21 ` Tejun Heo
0 siblings, 1 reply; 6+ messages in thread
From: Mel Gorman @ 2017-01-26 10:47 UTC (permalink / raw)
To: Andrew Morton
Cc: Tejun Heo, Vlastimil Babka, Linux Kernel, Linux-MM, Hillf Danton,
Jesper Dangaard Brouer
On Wed, Jan 25, 2017 at 04:08:02PM -0800, Andrew Morton wrote:
> > + for_each_cpu(cpu, &cpus_with_pcps) {
> > + struct work_struct *work = per_cpu_ptr(&pcpu_drain, cpu);
> > + INIT_WORK(work, drain_local_pages_wq);
>
> It's strange to repeatedly run INIT_WORK() in this fashion.
> Overwriting an atomic_t which should already be zero, initializing a
> list_head which should already be in the initialized state...
>
> Can we instead do this a single time in init code?
>
INIT_WORK does different things depending on whether LOCKDEP is enabled or
not and also whether object debugging is enabled. I'd worry that it's not
functionally equivalent or some future change would break the assumptions
about what INIT_WORK does internally. The init cost is there, but it's
insignicant in comparison to the whole workqueue operation or the old
cost of sending IPIs for that matter.
--
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>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] mm, page_alloc: Use static global work_struct for draining per-cpu pages
2017-01-26 10:47 ` Mel Gorman
@ 2017-01-26 19:21 ` Tejun Heo
0 siblings, 0 replies; 6+ messages in thread
From: Tejun Heo @ 2017-01-26 19:21 UTC (permalink / raw)
To: Mel Gorman
Cc: Andrew Morton, Vlastimil Babka, Linux Kernel, Linux-MM,
Hillf Danton, Jesper Dangaard Brouer
Hello,
On Thu, Jan 26, 2017 at 10:47:32AM +0000, Mel Gorman wrote:
> On Wed, Jan 25, 2017 at 04:08:02PM -0800, Andrew Morton wrote:
> > > + for_each_cpu(cpu, &cpus_with_pcps) {
> > > + struct work_struct *work = per_cpu_ptr(&pcpu_drain, cpu);
> > > + INIT_WORK(work, drain_local_pages_wq);
> >
> > It's strange to repeatedly run INIT_WORK() in this fashion.
> > Overwriting an atomic_t which should already be zero, initializing a
> > list_head which should already be in the initialized state...
> >
> > Can we instead do this a single time in init code?
> >
>
> INIT_WORK does different things depending on whether LOCKDEP is enabled or
> not and also whether object debugging is enabled. I'd worry that it's not
> functionally equivalent or some future change would break the assumptions
> about what INIT_WORK does internally. The init cost is there, but it's
> insignicant in comparison to the whole workqueue operation or the old
> cost of sending IPIs for that matter.
Both initing once or per each invocation are perfectly valid and
guaranteed to work. idk, I don't have a strong opinion hereag.
Thanks.
--
tejun
--
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>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-01-26 19:21 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-01-25 8:30 [PATCH] mm, page_alloc: Use static global work_struct for draining per-cpu pages Mel Gorman
2017-01-25 9:24 ` Vlastimil Babka
2017-01-25 9:33 ` Hillf Danton
2017-01-26 0:08 ` Andrew Morton
2017-01-26 10:47 ` Mel Gorman
2017-01-26 19:21 ` Tejun Heo
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).