mm, page_alloc: Use static global work_struct for draining per-cpu pages

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

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

---

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.

...

mm/page_alloc.c +0 -9

@@ -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 {

mm/page_alloc.c void drain_all_pages(struct zone *zone) · line 2420

Quoted 3 lines

mm/page_alloc.c +3 -0

+	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?

Quoted 10 lines · Andrew Morton

mm/page_alloc.c void drain_all_pages(struct zone *zone) · line 2420

Quoted 3 lines

mm/page_alloc.c +3 -0

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