Skip to contentSkip to conversation

vmstat: Speedup and Cleanup

PATCH In review Christoph Lameter 3 participants 6 messages Feb 22, 2016 – Feb 24, 2016

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

cover vmstat: Speedup and Cleanup

I have not been too satisfied with the recent code changes and in particular not fond of the races and the performance regressions I have seen. So this is an attempt to address those by making some small code changes and by removing the cpu_stat_off cpumask which was the reason for the races.

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

#1 lore raw

PATCH 1/2 vmstat: Optimize refresh_cpu_vmstat()

Create a new function zone_needs_update() that uses a memchr to check all diffs for being nonzero first.

If we use this function in refresh_cpu_vm_stat() then we can avoid the this_cpu_xchg() loop over all differentials. This becomes in particular important as the number of counters keeps on increasing.

This also avoids modifying the cachelines with the differentials unnecessarily.

Also add some likely()s to ensure that the icache requirements are low when we do not have any updates to process.

mm/vmstat.c +25 -18
Index: linux/mm/vmstat.c
===================================================================
@@ -444,6 +444,18 @@ static int fold_diff(int *diff)
 	return changes;
 }
 
+bool zone_needs_update(struct per_cpu_pageset *p)
+{
+
+	BUILD_BUG_ON(sizeof(p->vm_stat_diff[0]) != 1);
+	/*
+	 * The fast way of checking if there are any vmstat diffs.
+	 * This works because the diffs are byte sized items.
+	 */
+	return memchr_inv(p->vm_stat_diff, 0,
+			NR_VM_ZONE_STAT_ITEMS) != NULL;
+}
+
 /*
  * Update the zone counters for the current cpu.
  *
@@ -470,18 +482,20 @@ static int refresh_cpu_vm_stats(bool do_
 	for_each_populated_zone(zone) {
 		struct per_cpu_pageset __percpu *p = zone->pageset;
 
-		for (i = 0; i < NR_VM_ZONE_STAT_ITEMS; i++) {
-			int v;
+		if (unlikely(zone_needs_update(this_cpu_ptr(p)))) {
+			for (i = 0; i < NR_VM_ZONE_STAT_ITEMS; i++) {
+				int v;
 
-			v = this_cpu_xchg(p->vm_stat_diff[i], 0);
-			if (v) {
+				v = this_cpu_xchg(p->vm_stat_diff[i], 0);
+				if (v) {
 
-				atomic_long_add(v, &zone->vm_stat[i]);
-				global_diff[i] += v;
+					atomic_long_add(v, &zone->vm_stat[i]);
+					global_diff[i] += v;
 #ifdef CONFIG_NUMA
-				/* 3 seconds idle till flush */
-				__this_cpu_write(p->expire, 3);
+					/* 3 seconds idle till flush */
+					__this_cpu_write(p->expire, 3);
 #endif
+				}
 			}
 		}
 #ifdef CONFIG_NUMA
@@ -494,8 +508,8 @@ static int refresh_cpu_vm_stats(bool do_
 			 * Check if there are pages remaining in this pageset
 			 * if not then there is nothing to expire.
 			 */
-			if (!__this_cpu_read(p->expire) ||
-			       !__this_cpu_read(p->pcp.count))
+			if (likely(!__this_cpu_read(p->expire) ||
+			       !__this_cpu_read(p->pcp.count)))
 				continue;
 
 			/*
@@ -1440,19 +1454,12 @@ static bool need_update(int cpu)
 	for_each_populated_zone(zone) {
 		struct per_cpu_pageset *p = per_cpu_ptr(zone->pageset, cpu);
 
-		BUILD_BUG_ON(sizeof(p->vm_stat_diff[0]) != 1);
-		/*
-		 * The fast way of checking if there are any vmstat diffs.
-		 * This works because the diffs are byte sized items.
-		 */
-		if (memchr_inv(p->vm_stat_diff, 0, NR_VM_ZONE_STAT_ITEMS))
+		if (zone_needs_update(p))
 			return true;
-
 	}
 	return false;
 }
 
-
 /*
  * Shepherd worker thread that checks the
  * differentials of processors that have their worker

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

#2 lore raw

Re: [patch 1/2] vmstat: Optimize refresh_cpu_vmstat()

Quoted 7 lines · Christoph Lameter

Create a new function zone_needs_update() that uses a memchr to check all diffs for being nonzero first.

If we use this function in refresh_cpu_vm_stat() then we can avoid the this_cpu_xchg() loop over all differentials. This becomes in particular important as the number of counters keeps on increasing.

Quoted 5 lines

This also avoids modifying the cachelines with the differentials unnecessarily.

Also add some likely()s to ensure that the icache requirements are low when we do not have any updates to process.

Do you have any numbers? Can you actually measure an interference of refresh_cpu_vmstat?

mm/vmstat.c static int fold_diff(int *diff) · line 447

Quoted 11 lines

Index: linux/mm/vmstat.c
===================================================================

mm/vmstat.c +1 -0
@@ -444,6 +444,18 @@ static int fold_diff(int *diff)
 	return changes;
 }
 
+bool zone_needs_update(struct per_cpu_pageset *p)

static bool ....

Quoted 15 lines
mm/vmstat.c +11 -0
+{
+
+	BUILD_BUG_ON(sizeof(p->vm_stat_diff[0]) != 1);
+	/*
+	 * The fast way of checking if there are any vmstat diffs.
+	 * This works because the diffs are byte sized items.
+	 */
+	return memchr_inv(p->vm_stat_diff, 0,
+			NR_VM_ZONE_STAT_ITEMS) != NULL;
+}
+
 /*
  * Update the zone counters for the current cpu.
  *

mm/vmstat.c static int refresh_cpu_vm_stats(bool do_ · line 485

Quoted 6 lines
mm/vmstat.c +1 -2
@@ -470,18 +482,20 @@ static int refresh_cpu_vm_stats(bool do_
 	for_each_populated_zone(zone) {
 		struct per_cpu_pageset __percpu *p = zone->pageset;
 
-		for (i = 0; i < NR_VM_ZONE_STAT_ITEMS; i++) {
-			int v;
+		if (unlikely(zone_needs_update(this_cpu_ptr(p)))) {

why unlikely? The generated code looks exactly same with or without it (same for the other likely annotation added by this patch).

[...]

Signature · 8 lines
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
#3 lore raw

PATCH 2/2 vmstat: Get rid of the ugly cpu_stat_off variable

In reply to Christoph Lameter

The cpu_stat_off variable is unecessary since we can check if a workqueue request is pending otherwise. This makes it pretty easy for the shepherd to ensure that the proper things happen.

Removing the state also removes all races related to it. Should a workqueue not be scheduled as needed for vmstat_update then the shepherd will notice and schedule it as needed. Should a workqueue be unecessarily scheduled then the vmstat updater will disable it.

Thus vmstat_idle can also be simplified.

mm/vmstat.c +7 -28
Index: linux/mm/vmstat.c
===================================================================
@@ -1401,7 +1401,6 @@ static const struct file_operations proc
 static struct workqueue_struct *vmstat_wq;
 static DEFINE_PER_CPU(struct delayed_work, vmstat_work);
 int sysctl_stat_interval __read_mostly = HZ;
-static cpumask_var_t cpu_stat_off;
 
 static void vmstat_update(struct work_struct *w)
 {
@@ -1414,15 +1413,6 @@ static void vmstat_update(struct work_st
 		queue_delayed_work_on(smp_processor_id(), vmstat_wq,
 			this_cpu_ptr(&vmstat_work),
 			round_jiffies_relative(sysctl_stat_interval));
-	} else {
-		/*
-		 * We did not update any counters so the app may be in
-		 * a mode where it does not cause counter updates.
-		 * We may be uselessly running vmstat_update.
-		 * Defer the checking for differentials to the
-		 * shepherd thread on a different processor.
-		 */
-		cpumask_set_cpu(smp_processor_id(), cpu_stat_off);
 	}
 }
 
@@ -1436,11 +1426,8 @@ void quiet_vmstat(void)
 	if (system_state != SYSTEM_RUNNING)
 		return;
 
-	do {
-		if (!cpumask_test_and_set_cpu(smp_processor_id(), cpu_stat_off))
-			cancel_delayed_work(this_cpu_ptr(&vmstat_work));
-
-	} while (refresh_cpu_vm_stats(false));
+	refresh_cpu_vm_stats(false);
+	cancel_delayed_work(this_cpu_ptr(&vmstat_work));
 }
 
 /*
@@ -1476,13 +1463,12 @@ static void vmstat_shepherd(struct work_
 
 	get_online_cpus();
 	/* Check processors whose vmstat worker threads have been disabled */
-	for_each_cpu(cpu, cpu_stat_off)
-		if (need_update(cpu) &&
-			cpumask_test_and_clear_cpu(cpu, cpu_stat_off))
-
-			queue_delayed_work_on(cpu, vmstat_wq,
-				&per_cpu(vmstat_work, cpu), 0);
+	for_each_online_cpu(cpu) {
+		struct delayed_work *worker = &per_cpu(vmstat_work, cpu);
 
+		if (!delayed_work_pending(worker) && need_update(cpu))
+			queue_delayed_work_on(cpu, vmstat_wq, worker, 0);
+	}
 	put_online_cpus();
 
 	schedule_delayed_work(&shepherd,
@@ -1498,10 +1484,6 @@ static void __init start_shepherd_timer(
 		INIT_DELAYED_WORK(per_cpu_ptr(&vmstat_work, cpu),
 			vmstat_update);
 
-	if (!alloc_cpumask_var(&cpu_stat_off, GFP_KERNEL))
-		BUG();
-	cpumask_copy(cpu_stat_off, cpu_online_mask);
-
 	vmstat_wq = alloc_workqueue("vmstat", WQ_FREEZABLE|WQ_MEM_RECLAIM, 0);
 	schedule_delayed_work(&shepherd,
 		round_jiffies_relative(sysctl_stat_interval));
@@ -1536,16 +1518,13 @@ static int vmstat_cpuup_callback(struct
 	case CPU_ONLINE_FROZEN:
 		refresh_zone_stat_thresholds();
 		node_set_state(cpu_to_node(cpu), N_CPU);
-		cpumask_set_cpu(cpu, cpu_stat_off);
 		break;
 	case CPU_DOWN_PREPARE:
 	case CPU_DOWN_PREPARE_FROZEN:
 		cancel_delayed_work_sync(&per_cpu(vmstat_work, cpu));
-		cpumask_clear_cpu(cpu, cpu_stat_off);
 		break;
 	case CPU_DOWN_FAILED:
 	case CPU_DOWN_FAILED_FROZEN:
-		cpumask_set_cpu(cpu, cpu_stat_off);
 		break;
 	case CPU_DEAD:
 	case CPU_DEAD_FROZEN:

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

maintainer
#4 lore raw

Re: [patch 2/2] vmstat: Get rid of the ugly cpu_stat_off variable

Quoted 11 lines · On Mon, 22 Feb 2016 12:10:42 -0600 Christoph Lameter <[email protected]> wrote:

The cpu_stat_off variable is unecessary since we can check if a workqueue request is pending otherwise. This makes it pretty easy for the shepherd to ensure that the proper things happen.

Removing the state also removes all races related to it. Should a workqueue not be scheduled as needed for vmstat_update then the shepherd will notice and schedule it as needed. Should a workqueue be unecessarily scheduled then the vmstat updater will disable it.

Thus vmstat_idle can also be simplified.

I'm getting rather a lot of rejects from this one.

mm/vmstat.c void quiet_vmstat(void) · line 1431

Quoted 13 lines
mm/vmstat.c +2 -5
@@ -1436,11 +1426,8 @@ void quiet_vmstat(void)
 	if (system_state != SYSTEM_RUNNING)
 		return;
 
-	do {
-		if (!cpumask_test_and_set_cpu(smp_processor_id(), cpu_stat_off))
-			cancel_delayed_work(this_cpu_ptr(&vmstat_work));
-
-	} while (refresh_cpu_vm_stats(false));
+	refresh_cpu_vm_stats(false);
+	cancel_delayed_work(this_cpu_ptr(&vmstat_work));
 }

I can't find a quiet_vmstat() which looks like this. What tree are you patching?

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

#5 lore raw

Re: [patch 2/2] vmstat: Get rid of the ugly cpu_stat_off variable

Quoted 21 lines · Andrew Morton
Quoted 11 lines · On Mon, 22 Feb 2016 12:10:42 -0600 Christoph Lameter <[email protected]> wrote:

The cpu_stat_off variable is unecessary since we can check if a workqueue request is pending otherwise. This makes it pretty easy for the shepherd to ensure that the proper things happen.

Removing the state also removes all races related to it. Should a workqueue not be scheduled as needed for vmstat_update then the shepherd will notice and schedule it as needed. Should a workqueue be unecessarily scheduled then the vmstat updater will disable it.

Thus vmstat_idle can also be simplified.

I'm getting rather a lot of rejects from this one.

Quoted 5 lines
mm/vmstat.c +0 -0
@@ -1436,11 +1426,8 @@ void quiet_vmstat(void)
 	if (system_state != SYSTEM_RUNNING)
 		return;
 
Quoted 11 lines

mm/vmstat.c void quiet_vmstat(void) · line 1431

Quoted 8 lines
mm/vmstat.c +2 -5
-	do {
-		if (!cpumask_test_and_set_cpu(smp_processor_id(), cpu_stat_off))
-			cancel_delayed_work(this_cpu_ptr(&vmstat_work));
-
-	} while (refresh_cpu_vm_stats(false));
+	refresh_cpu_vm_stats(false);
+	cancel_delayed_work(this_cpu_ptr(&vmstat_work));
 }

I can't find a quiet_vmstat() which looks like this. What tree are you patching?

This seems to be pre f01f17d3705b ("mm, vmstat: make quiet_vmstat lighter")

Signature · 8 lines
Michal Hocko
SUSE Labs

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