Messages in this thread Patch in this message | | | Date | Wed, 8 Feb 2017 12:31:13 +0000 | From | Mel Gorman <> | Subject | Re: [RFC PATCH] mm: move pcp and lru-pcp drainging into vmstat_wq |
| |
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
| |