Skip to contentSkip to conversation

NUMA memchr_inv() in mm/vmstat.c:need_update()?

DISCUSSION Janne Huttunen 2 participants 2 messages Nov 2, 2018 – Nov 6, 2018

Subsystems (2)
Memory management MM core
lore
Share & tools
lore

Janne Huttunen

author
#0 lore raw

Hi,

The commit 1d90ca897A changed the type of the vm_numa_stat_diff from s8 into u16. It also changed the associated BUILD_BUG_ON() in need_update(), but didn't touch the memchr_inv() call after it. Is the memchr_inv() call still supposed to cover the whole array or am I just misreading the code?

#1 lore raw
Quoted 7 lines · Janne Huttunen

Hi,

The commit 1d90ca897 changed the type of the vm_numa_stat_diff from s8 into u16. It also changed the associated BUILD_BUG_ON() in need_update(), but didn't touch the memchr_inv() call after it. Is the memchr_inv() call still supposed to cover the whole array or am I just misreading the code?

I guess you are not missing anything. We need something like

mm/vmstat.c +2 -2
index 6038ce593ce3..c42f01fbe964 100644
@@ -1829,10 +1829,10 @@ static bool need_update(int cpu)
 		 * 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 (memchr_inv(p->vm_stat_diff, 0, sizeof(*p->vm_stat_diff) * NR_VM_ZONE_STAT_ITEMS))
 			return true;
 #ifdef CONFIG_NUMA
-		if (memchr_inv(p->vm_numa_stat_diff, 0, NR_VM_NUMA_STAT_ITEMS))
+		if (memchr_inv(p->vm_numa_stat_diff, 0, sizeof(p->vm_numa_stat_diff) * NR_VM_NUMA_STAT_ITEMS))
 			return true;
 #endif
 	}

sizeof(p->vm_numa_stat_diff) would be shorter but more fragile if we ever decide to change the type to be a pointer rather than a fixed arrasy.

Care to send a full patch?

Signature · 2 lines
Michal Hocko
SUSE Labs