| Message ID | 20201202182725.265020-6-shy828301@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Make shrinker's nr_deferred memcg aware | expand |
On Wed, Dec 02, 2020 at 10:27:21AM -0800, Yang Shi wrote: > Currently the number of deferred objects are per shrinker, but some slabs, for example, > vfs inode/dentry cache are per memcg, this would result in poor isolation among memcgs. > > The deferred objects typically are generated by __GFP_NOFS allocations, one memcg with > excessive __GFP_NOFS allocations may blow up deferred objects, then other innocent memcgs > may suffer from over shrink, excessive reclaim latency, etc. > > For example, two workloads run in memcgA and memcgB respectively, workload in B is vfs > heavy workload. Workload in A generates excessive deferred objects, then B's vfs cache > might be hit heavily (drop half of caches) by B's limit reclaim or global reclaim. > > We observed this hit in our production environment which was running vfs heavy workload > shown as the below tracing log: > > <...>-409454 [016] .... 28286961.747146: mm_shrink_slab_start: super_cache_scan+0x0/0x1a0 ffff9a83046f3458: > nid: 1 objects to shrink 3641681686040 gfp_flags GFP_HIGHUSER_MOVABLE|__GFP_ZERO pgs_scanned 1 lru_pgs 15721 > cache items 246404277 delta 31345 total_scan 123202138 > <...>-409454 [022] .... 28287105.928018: mm_shrink_slab_end: super_cache_scan+0x0/0x1a0 ffff9a83046f3458: > nid: 1 unused scan count 3641681686040 new scan count 3641798379189 total_scan 602 > last shrinker return val 123186855 > > The vfs cache and page cache ration was 10:1 on this machine, and half of caches were dropped. > This also resulted in significant amount of page caches were dropped due to inodes eviction. > > Make nr_deferred per memcg for memcg aware shrinkers would solve the unfairness and bring > better isolation. > > When memcg is not enabled (!CONFIG_MEMCG or memcg disabled), the shrinker's nr_deferred > would be used. And non memcg aware shrinkers use shrinker's nr_deferred all the time. > > Signed-off-by: Yang Shi <shy828301@gmail.com> > --- > include/linux/memcontrol.h | 9 +++ > mm/memcontrol.c | 112 ++++++++++++++++++++++++++++++++++++- > mm/vmscan.c | 4 ++ > 3 files changed, 123 insertions(+), 2 deletions(-) > > diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h > index 922a7f600465..1b343b268359 100644 > --- a/include/linux/memcontrol.h > +++ b/include/linux/memcontrol.h > @@ -92,6 +92,13 @@ struct lruvec_stat { > long count[NR_VM_NODE_STAT_ITEMS]; > }; > > + > +/* Shrinker::id indexed nr_deferred of memcg-aware shrinkers. */ > +struct memcg_shrinker_deferred { > + struct rcu_head rcu; > + atomic_long_t nr_deferred[]; > +}; The idea makes total sense to me. But I wonder if we can add nr_deferred to struct list_lru_one, instead of adding another per-memcg per-shrinker entity? I guess it can simplify the code quite a lot. What do you think?
On Wed, Dec 2, 2020 at 7:06 PM Roman Gushchin <guro@fb.com> wrote: > > On Wed, Dec 02, 2020 at 10:27:21AM -0800, Yang Shi wrote: > > Currently the number of deferred objects are per shrinker, but some slabs, for example, > > vfs inode/dentry cache are per memcg, this would result in poor isolation among memcgs. > > > > The deferred objects typically are generated by __GFP_NOFS allocations, one memcg with > > excessive __GFP_NOFS allocations may blow up deferred objects, then other innocent memcgs > > may suffer from over shrink, excessive reclaim latency, etc. > > > > For example, two workloads run in memcgA and memcgB respectively, workload in B is vfs > > heavy workload. Workload in A generates excessive deferred objects, then B's vfs cache > > might be hit heavily (drop half of caches) by B's limit reclaim or global reclaim. > > > > We observed this hit in our production environment which was running vfs heavy workload > > shown as the below tracing log: > > > > <...>-409454 [016] .... 28286961.747146: mm_shrink_slab_start: super_cache_scan+0x0/0x1a0 ffff9a83046f3458: > > nid: 1 objects to shrink 3641681686040 gfp_flags GFP_HIGHUSER_MOVABLE|__GFP_ZERO pgs_scanned 1 lru_pgs 15721 > > cache items 246404277 delta 31345 total_scan 123202138 > > <...>-409454 [022] .... 28287105.928018: mm_shrink_slab_end: super_cache_scan+0x0/0x1a0 ffff9a83046f3458: > > nid: 1 unused scan count 3641681686040 new scan count 3641798379189 total_scan 602 > > last shrinker return val 123186855 > > > > The vfs cache and page cache ration was 10:1 on this machine, and half of caches were dropped. > > This also resulted in significant amount of page caches were dropped due to inodes eviction. > > > > Make nr_deferred per memcg for memcg aware shrinkers would solve the unfairness and bring > > better isolation. > > > > When memcg is not enabled (!CONFIG_MEMCG or memcg disabled), the shrinker's nr_deferred > > would be used. And non memcg aware shrinkers use shrinker's nr_deferred all the time. > > > > Signed-off-by: Yang Shi <shy828301@gmail.com> > > --- > > include/linux/memcontrol.h | 9 +++ > > mm/memcontrol.c | 112 ++++++++++++++++++++++++++++++++++++- > > mm/vmscan.c | 4 ++ > > 3 files changed, 123 insertions(+), 2 deletions(-) > > > > diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h > > index 922a7f600465..1b343b268359 100644 > > --- a/include/linux/memcontrol.h > > +++ b/include/linux/memcontrol.h > > @@ -92,6 +92,13 @@ struct lruvec_stat { > > long count[NR_VM_NODE_STAT_ITEMS]; > > }; > > > > + > > +/* Shrinker::id indexed nr_deferred of memcg-aware shrinkers. */ > > +struct memcg_shrinker_deferred { > > + struct rcu_head rcu; > > + atomic_long_t nr_deferred[]; > > +}; > > The idea makes total sense to me. But I wonder if we can add nr_deferred to > struct list_lru_one, instead of adding another per-memcg per-shrinker entity? > I guess it can simplify the code quite a lot. What do you think? Aha, actually this exactly was what I did at the first place. But Dave NAK'ed this approach. You can find the discussion at: https://lore.kernel.org/linux-mm/20200930073152.GH12096@dread.disaster.area/.
On Wed, Dec 2, 2020 at 8:54 PM Yang Shi <shy828301@gmail.com> wrote: > > On Wed, Dec 2, 2020 at 7:06 PM Roman Gushchin <guro@fb.com> wrote: > > > > On Wed, Dec 02, 2020 at 10:27:21AM -0800, Yang Shi wrote: > > > Currently the number of deferred objects are per shrinker, but some slabs, for example, > > > vfs inode/dentry cache are per memcg, this would result in poor isolation among memcgs. > > > > > > The deferred objects typically are generated by __GFP_NOFS allocations, one memcg with > > > excessive __GFP_NOFS allocations may blow up deferred objects, then other innocent memcgs > > > may suffer from over shrink, excessive reclaim latency, etc. > > > > > > For example, two workloads run in memcgA and memcgB respectively, workload in B is vfs > > > heavy workload. Workload in A generates excessive deferred objects, then B's vfs cache > > > might be hit heavily (drop half of caches) by B's limit reclaim or global reclaim. > > > > > > We observed this hit in our production environment which was running vfs heavy workload > > > shown as the below tracing log: > > > > > > <...>-409454 [016] .... 28286961.747146: mm_shrink_slab_start: super_cache_scan+0x0/0x1a0 ffff9a83046f3458: > > > nid: 1 objects to shrink 3641681686040 gfp_flags GFP_HIGHUSER_MOVABLE|__GFP_ZERO pgs_scanned 1 lru_pgs 15721 > > > cache items 246404277 delta 31345 total_scan 123202138 > > > <...>-409454 [022] .... 28287105.928018: mm_shrink_slab_end: super_cache_scan+0x0/0x1a0 ffff9a83046f3458: > > > nid: 1 unused scan count 3641681686040 new scan count 3641798379189 total_scan 602 > > > last shrinker return val 123186855 > > > > > > The vfs cache and page cache ration was 10:1 on this machine, and half of caches were dropped. > > > This also resulted in significant amount of page caches were dropped due to inodes eviction. > > > > > > Make nr_deferred per memcg for memcg aware shrinkers would solve the unfairness and bring > > > better isolation. > > > > > > When memcg is not enabled (!CONFIG_MEMCG or memcg disabled), the shrinker's nr_deferred > > > would be used. And non memcg aware shrinkers use shrinker's nr_deferred all the time. > > > > > > Signed-off-by: Yang Shi <shy828301@gmail.com> > > > --- > > > include/linux/memcontrol.h | 9 +++ > > > mm/memcontrol.c | 112 ++++++++++++++++++++++++++++++++++++- > > > mm/vmscan.c | 4 ++ > > > 3 files changed, 123 insertions(+), 2 deletions(-) > > > > > > diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h > > > index 922a7f600465..1b343b268359 100644 > > > --- a/include/linux/memcontrol.h > > > +++ b/include/linux/memcontrol.h > > > @@ -92,6 +92,13 @@ struct lruvec_stat { > > > long count[NR_VM_NODE_STAT_ITEMS]; > > > }; > > > > > > + > > > +/* Shrinker::id indexed nr_deferred of memcg-aware shrinkers. */ > > > +struct memcg_shrinker_deferred { > > > + struct rcu_head rcu; > > > + atomic_long_t nr_deferred[]; > > > +}; > > > > The idea makes total sense to me. But I wonder if we can add nr_deferred to > > struct list_lru_one, instead of adding another per-memcg per-shrinker entity? > > I guess it can simplify the code quite a lot. What do you think? > > Aha, actually this exactly was what I did at the first place. But Dave > NAK'ed this approach. You can find the discussion at: > https://lore.kernel.org/linux-mm/20200930073152.GH12096@dread.disaster.area/. I did prototypes for both approaches (move nr_deferred to list_lru or to memcg). I preferred the list_lru approach at the first place. But Dave's opinion does make perfect sense to me. So I dropped that list_lru one. That email elaborated why moving nr_deferred to list_lru is not appropriate.
On Thu, Dec 03, 2020 at 10:03:44AM -0800, Yang Shi wrote: > On Wed, Dec 2, 2020 at 8:54 PM Yang Shi <shy828301@gmail.com> wrote: > > > > On Wed, Dec 2, 2020 at 7:06 PM Roman Gushchin <guro@fb.com> wrote: > > > > > > On Wed, Dec 02, 2020 at 10:27:21AM -0800, Yang Shi wrote: > > > > Currently the number of deferred objects are per shrinker, but some slabs, for example, > > > > vfs inode/dentry cache are per memcg, this would result in poor isolation among memcgs. > > > > > > > > The deferred objects typically are generated by __GFP_NOFS allocations, one memcg with > > > > excessive __GFP_NOFS allocations may blow up deferred objects, then other innocent memcgs > > > > may suffer from over shrink, excessive reclaim latency, etc. > > > > > > > > For example, two workloads run in memcgA and memcgB respectively, workload in B is vfs > > > > heavy workload. Workload in A generates excessive deferred objects, then B's vfs cache > > > > might be hit heavily (drop half of caches) by B's limit reclaim or global reclaim. > > > > > > > > We observed this hit in our production environment which was running vfs heavy workload > > > > shown as the below tracing log: > > > > > > > > <...>-409454 [016] .... 28286961.747146: mm_shrink_slab_start: super_cache_scan+0x0/0x1a0 ffff9a83046f3458: > > > > nid: 1 objects to shrink 3641681686040 gfp_flags GFP_HIGHUSER_MOVABLE|__GFP_ZERO pgs_scanned 1 lru_pgs 15721 > > > > cache items 246404277 delta 31345 total_scan 123202138 > > > > <...>-409454 [022] .... 28287105.928018: mm_shrink_slab_end: super_cache_scan+0x0/0x1a0 ffff9a83046f3458: > > > > nid: 1 unused scan count 3641681686040 new scan count 3641798379189 total_scan 602 > > > > last shrinker return val 123186855 > > > > > > > > The vfs cache and page cache ration was 10:1 on this machine, and half of caches were dropped. > > > > This also resulted in significant amount of page caches were dropped due to inodes eviction. > > > > > > > > Make nr_deferred per memcg for memcg aware shrinkers would solve the unfairness and bring > > > > better isolation. > > > > > > > > When memcg is not enabled (!CONFIG_MEMCG or memcg disabled), the shrinker's nr_deferred > > > > would be used. And non memcg aware shrinkers use shrinker's nr_deferred all the time. > > > > > > > > Signed-off-by: Yang Shi <shy828301@gmail.com> > > > > --- > > > > include/linux/memcontrol.h | 9 +++ > > > > mm/memcontrol.c | 112 ++++++++++++++++++++++++++++++++++++- > > > > mm/vmscan.c | 4 ++ > > > > 3 files changed, 123 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h > > > > index 922a7f600465..1b343b268359 100644 > > > > --- a/include/linux/memcontrol.h > > > > +++ b/include/linux/memcontrol.h > > > > @@ -92,6 +92,13 @@ struct lruvec_stat { > > > > long count[NR_VM_NODE_STAT_ITEMS]; > > > > }; > > > > > > > > + > > > > +/* Shrinker::id indexed nr_deferred of memcg-aware shrinkers. */ > > > > +struct memcg_shrinker_deferred { > > > > + struct rcu_head rcu; > > > > + atomic_long_t nr_deferred[]; > > > > +}; > > > > > > The idea makes total sense to me. But I wonder if we can add nr_deferred to > > > struct list_lru_one, instead of adding another per-memcg per-shrinker entity? > > > I guess it can simplify the code quite a lot. What do you think? > > > > Aha, actually this exactly was what I did at the first place. But Dave > > NAK'ed this approach. You can find the discussion at: > > https://lore.kernel.org/linux-mm/20200930073152.GH12096@dread.disaster.area/. Yes, this makes sense for me. Thank you for the link! > > I did prototypes for both approaches (move nr_deferred to list_lru or > to memcg). I preferred the list_lru approach at the first place. But > Dave's opinion does make perfect sense to me. So I dropped that > list_lru one. That email elaborated why moving nr_deferred to list_lru > is not appropriate. Hm, shouldn't we move list_lru to memcg then? It's not directly related to your patchset, but maybe it's something we should consider in the future. What worries me is that with your patchset we'll have 3 separate per-memcg (per-node) per-shrinker entity, each with slightly different approach to allocate/extend/reparent/release. So it begs for some unification. I don't think it's a showstopper for your work though, it can be done later.
On Thu, Dec 3, 2020 at 12:07 PM Roman Gushchin <guro@fb.com> wrote: > > On Thu, Dec 03, 2020 at 10:03:44AM -0800, Yang Shi wrote: > > On Wed, Dec 2, 2020 at 8:54 PM Yang Shi <shy828301@gmail.com> wrote: > > > > > > On Wed, Dec 2, 2020 at 7:06 PM Roman Gushchin <guro@fb.com> wrote: > > > > > > > > On Wed, Dec 02, 2020 at 10:27:21AM -0800, Yang Shi wrote: > > > > > Currently the number of deferred objects are per shrinker, but some slabs, for example, > > > > > vfs inode/dentry cache are per memcg, this would result in poor isolation among memcgs. > > > > > > > > > > The deferred objects typically are generated by __GFP_NOFS allocations, one memcg with > > > > > excessive __GFP_NOFS allocations may blow up deferred objects, then other innocent memcgs > > > > > may suffer from over shrink, excessive reclaim latency, etc. > > > > > > > > > > For example, two workloads run in memcgA and memcgB respectively, workload in B is vfs > > > > > heavy workload. Workload in A generates excessive deferred objects, then B's vfs cache > > > > > might be hit heavily (drop half of caches) by B's limit reclaim or global reclaim. > > > > > > > > > > We observed this hit in our production environment which was running vfs heavy workload > > > > > shown as the below tracing log: > > > > > > > > > > <...>-409454 [016] .... 28286961.747146: mm_shrink_slab_start: super_cache_scan+0x0/0x1a0 ffff9a83046f3458: > > > > > nid: 1 objects to shrink 3641681686040 gfp_flags GFP_HIGHUSER_MOVABLE|__GFP_ZERO pgs_scanned 1 lru_pgs 15721 > > > > > cache items 246404277 delta 31345 total_scan 123202138 > > > > > <...>-409454 [022] .... 28287105.928018: mm_shrink_slab_end: super_cache_scan+0x0/0x1a0 ffff9a83046f3458: > > > > > nid: 1 unused scan count 3641681686040 new scan count 3641798379189 total_scan 602 > > > > > last shrinker return val 123186855 > > > > > > > > > > The vfs cache and page cache ration was 10:1 on this machine, and half of caches were dropped. > > > > > This also resulted in significant amount of page caches were dropped due to inodes eviction. > > > > > > > > > > Make nr_deferred per memcg for memcg aware shrinkers would solve the unfairness and bring > > > > > better isolation. > > > > > > > > > > When memcg is not enabled (!CONFIG_MEMCG or memcg disabled), the shrinker's nr_deferred > > > > > would be used. And non memcg aware shrinkers use shrinker's nr_deferred all the time. > > > > > > > > > > Signed-off-by: Yang Shi <shy828301@gmail.com> > > > > > --- > > > > > include/linux/memcontrol.h | 9 +++ > > > > > mm/memcontrol.c | 112 ++++++++++++++++++++++++++++++++++++- > > > > > mm/vmscan.c | 4 ++ > > > > > 3 files changed, 123 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h > > > > > index 922a7f600465..1b343b268359 100644 > > > > > --- a/include/linux/memcontrol.h > > > > > +++ b/include/linux/memcontrol.h > > > > > @@ -92,6 +92,13 @@ struct lruvec_stat { > > > > > long count[NR_VM_NODE_STAT_ITEMS]; > > > > > }; > > > > > > > > > > + > > > > > +/* Shrinker::id indexed nr_deferred of memcg-aware shrinkers. */ > > > > > +struct memcg_shrinker_deferred { > > > > > + struct rcu_head rcu; > > > > > + atomic_long_t nr_deferred[]; > > > > > +}; > > > > > > > > The idea makes total sense to me. But I wonder if we can add nr_deferred to > > > > struct list_lru_one, instead of adding another per-memcg per-shrinker entity? > > > > I guess it can simplify the code quite a lot. What do you think? > > > > > > Aha, actually this exactly was what I did at the first place. But Dave > > > NAK'ed this approach. You can find the discussion at: > > > https://lore.kernel.org/linux-mm/20200930073152.GH12096@dread.disaster.area/. > > Yes, this makes sense for me. Thank you for the link! > > > > > I did prototypes for both approaches (move nr_deferred to list_lru or > > to memcg). I preferred the list_lru approach at the first place. But > > Dave's opinion does make perfect sense to me. So I dropped that > > list_lru one. That email elaborated why moving nr_deferred to list_lru > > is not appropriate. > > Hm, shouldn't we move list_lru to memcg then? It's not directly related > to your patchset, but maybe it's something we should consider in the future. I haven't thought about this yet. I agree we could look into it further later on. > > What worries me is that with your patchset we'll have 3 separate > per-memcg (per-node) per-shrinker entity, each with slightly different > approach to allocate/extend/reparent/release. So it begs for some > unification. I don't think it's a showstopper for your work though, it > can be done later. Off the top of my head, we may be able to have shrinker_info struct, it should look like: struct shrinker_info { atomic_long_t nr_deferred; /* Just one bit is used now */ u8 map:1; } struct memcg_shrinker_info { struct rcu_head rcu; /* Indexed by shrinker ID */ struct shrinker_info info[]; } Then in struct mem_cgroup_per_node, we could have: struct mem_cgroup_per_node { .... struct memcg_shrinker_info __rcu *shrinker_info; .... } In this way shrinker_info should be allocated to all memcgs, including root. But shrinker could ignore root's map bit. We may waste a little bit memory, but we get unification. Would that work?
On Thu, Dec 03, 2020 at 02:49:00PM -0800, Yang Shi wrote: > On Thu, Dec 3, 2020 at 12:07 PM Roman Gushchin <guro@fb.com> wrote: > > > > On Thu, Dec 03, 2020 at 10:03:44AM -0800, Yang Shi wrote: > > > On Wed, Dec 2, 2020 at 8:54 PM Yang Shi <shy828301@gmail.com> wrote: > > > > > > > > On Wed, Dec 2, 2020 at 7:06 PM Roman Gushchin <guro@fb.com> wrote: > > > > > > > > > > On Wed, Dec 02, 2020 at 10:27:21AM -0800, Yang Shi wrote: > > > > > > Currently the number of deferred objects are per shrinker, but some slabs, for example, > > > > > > vfs inode/dentry cache are per memcg, this would result in poor isolation among memcgs. > > > > > > > > > > > > The deferred objects typically are generated by __GFP_NOFS allocations, one memcg with > > > > > > excessive __GFP_NOFS allocations may blow up deferred objects, then other innocent memcgs > > > > > > may suffer from over shrink, excessive reclaim latency, etc. > > > > > > > > > > > > For example, two workloads run in memcgA and memcgB respectively, workload in B is vfs > > > > > > heavy workload. Workload in A generates excessive deferred objects, then B's vfs cache > > > > > > might be hit heavily (drop half of caches) by B's limit reclaim or global reclaim. > > > > > > > > > > > > We observed this hit in our production environment which was running vfs heavy workload > > > > > > shown as the below tracing log: > > > > > > > > > > > > <...>-409454 [016] .... 28286961.747146: mm_shrink_slab_start: super_cache_scan+0x0/0x1a0 ffff9a83046f3458: > > > > > > nid: 1 objects to shrink 3641681686040 gfp_flags GFP_HIGHUSER_MOVABLE|__GFP_ZERO pgs_scanned 1 lru_pgs 15721 > > > > > > cache items 246404277 delta 31345 total_scan 123202138 > > > > > > <...>-409454 [022] .... 28287105.928018: mm_shrink_slab_end: super_cache_scan+0x0/0x1a0 ffff9a83046f3458: > > > > > > nid: 1 unused scan count 3641681686040 new scan count 3641798379189 total_scan 602 > > > > > > last shrinker return val 123186855 > > > > > > > > > > > > The vfs cache and page cache ration was 10:1 on this machine, and half of caches were dropped. > > > > > > This also resulted in significant amount of page caches were dropped due to inodes eviction. > > > > > > > > > > > > Make nr_deferred per memcg for memcg aware shrinkers would solve the unfairness and bring > > > > > > better isolation. > > > > > > > > > > > > When memcg is not enabled (!CONFIG_MEMCG or memcg disabled), the shrinker's nr_deferred > > > > > > would be used. And non memcg aware shrinkers use shrinker's nr_deferred all the time. > > > > > > > > > > > > Signed-off-by: Yang Shi <shy828301@gmail.com> > > > > > > --- > > > > > > include/linux/memcontrol.h | 9 +++ > > > > > > mm/memcontrol.c | 112 ++++++++++++++++++++++++++++++++++++- > > > > > > mm/vmscan.c | 4 ++ > > > > > > 3 files changed, 123 insertions(+), 2 deletions(-) > > > > > > > > > > > > diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h > > > > > > index 922a7f600465..1b343b268359 100644 > > > > > > --- a/include/linux/memcontrol.h > > > > > > +++ b/include/linux/memcontrol.h > > > > > > @@ -92,6 +92,13 @@ struct lruvec_stat { > > > > > > long count[NR_VM_NODE_STAT_ITEMS]; > > > > > > }; > > > > > > > > > > > > + > > > > > > +/* Shrinker::id indexed nr_deferred of memcg-aware shrinkers. */ > > > > > > +struct memcg_shrinker_deferred { > > > > > > + struct rcu_head rcu; > > > > > > + atomic_long_t nr_deferred[]; > > > > > > +}; > > > > > > > > > > The idea makes total sense to me. But I wonder if we can add nr_deferred to > > > > > struct list_lru_one, instead of adding another per-memcg per-shrinker entity? > > > > > I guess it can simplify the code quite a lot. What do you think? > > > > > > > > Aha, actually this exactly was what I did at the first place. But Dave > > > > NAK'ed this approach. You can find the discussion at: > > > > https://lore.kernel.org/linux-mm/20200930073152.GH12096@dread.disaster.area/. > > > > Yes, this makes sense for me. Thank you for the link! > > > > > > > > I did prototypes for both approaches (move nr_deferred to list_lru or > > > to memcg). I preferred the list_lru approach at the first place. But > > > Dave's opinion does make perfect sense to me. So I dropped that > > > list_lru one. That email elaborated why moving nr_deferred to list_lru > > > is not appropriate. > > > > Hm, shouldn't we move list_lru to memcg then? It's not directly related > > to your patchset, but maybe it's something we should consider in the future. > > I haven't thought about this yet. I agree we could look into it > further later on. > > > > > What worries me is that with your patchset we'll have 3 separate > > per-memcg (per-node) per-shrinker entity, each with slightly different > > approach to allocate/extend/reparent/release. So it begs for some > > unification. I don't think it's a showstopper for your work though, it > > can be done later. > > Off the top of my head, we may be able to have shrinker_info struct, > it should look like: > > struct shrinker_info { > atomic_long_t nr_deferred; > /* Just one bit is used now */ > u8 map:1; > } > > struct memcg_shrinker_info { > struct rcu_head rcu; > /* Indexed by shrinker ID */ > struct shrinker_info info[]; > } > > Then in struct mem_cgroup_per_node, we could have: > > struct mem_cgroup_per_node { > .... > struct memcg_shrinker_info __rcu *shrinker_info; > .... > } > > In this way shrinker_info should be allocated to all memcgs, including > root. But shrinker could ignore root's map bit. We may waste a little > bit memory, but we get unification. > > Would that work? Hm, not exactly, then you'll an ability to iterate with for_each_set_bit(i, map->map, shrinker_nr_max)... But you can probably do something like: struct shrinker_info { atomic_long_t nr_deferred; struct list_lru_one[]; /* optional, depends on the shrinker implementation */ }; struct memcg_shrinker_info { /* Indexed by shrinker ID */ unsigned long *map[]; struct shrinker_info *shrinker_info[]; } Then you'll be able to allocate individual shrinker_info structures on-demand. But, please, take this all with a grain of salt, I didn't check if it's all really possible or there are some obstacles. Thanks!
On Thu, Dec 3, 2020 at 3:31 PM Roman Gushchin <guro@fb.com> wrote: > > On Thu, Dec 03, 2020 at 02:49:00PM -0800, Yang Shi wrote: > > On Thu, Dec 3, 2020 at 12:07 PM Roman Gushchin <guro@fb.com> wrote: > > > > > > On Thu, Dec 03, 2020 at 10:03:44AM -0800, Yang Shi wrote: > > > > On Wed, Dec 2, 2020 at 8:54 PM Yang Shi <shy828301@gmail.com> wrote: > > > > > > > > > > On Wed, Dec 2, 2020 at 7:06 PM Roman Gushchin <guro@fb.com> wrote: > > > > > > > > > > > > On Wed, Dec 02, 2020 at 10:27:21AM -0800, Yang Shi wrote: > > > > > > > Currently the number of deferred objects are per shrinker, but some slabs, for example, > > > > > > > vfs inode/dentry cache are per memcg, this would result in poor isolation among memcgs. > > > > > > > > > > > > > > The deferred objects typically are generated by __GFP_NOFS allocations, one memcg with > > > > > > > excessive __GFP_NOFS allocations may blow up deferred objects, then other innocent memcgs > > > > > > > may suffer from over shrink, excessive reclaim latency, etc. > > > > > > > > > > > > > > For example, two workloads run in memcgA and memcgB respectively, workload in B is vfs > > > > > > > heavy workload. Workload in A generates excessive deferred objects, then B's vfs cache > > > > > > > might be hit heavily (drop half of caches) by B's limit reclaim or global reclaim. > > > > > > > > > > > > > > We observed this hit in our production environment which was running vfs heavy workload > > > > > > > shown as the below tracing log: > > > > > > > > > > > > > > <...>-409454 [016] .... 28286961.747146: mm_shrink_slab_start: super_cache_scan+0x0/0x1a0 ffff9a83046f3458: > > > > > > > nid: 1 objects to shrink 3641681686040 gfp_flags GFP_HIGHUSER_MOVABLE|__GFP_ZERO pgs_scanned 1 lru_pgs 15721 > > > > > > > cache items 246404277 delta 31345 total_scan 123202138 > > > > > > > <...>-409454 [022] .... 28287105.928018: mm_shrink_slab_end: super_cache_scan+0x0/0x1a0 ffff9a83046f3458: > > > > > > > nid: 1 unused scan count 3641681686040 new scan count 3641798379189 total_scan 602 > > > > > > > last shrinker return val 123186855 > > > > > > > > > > > > > > The vfs cache and page cache ration was 10:1 on this machine, and half of caches were dropped. > > > > > > > This also resulted in significant amount of page caches were dropped due to inodes eviction. > > > > > > > > > > > > > > Make nr_deferred per memcg for memcg aware shrinkers would solve the unfairness and bring > > > > > > > better isolation. > > > > > > > > > > > > > > When memcg is not enabled (!CONFIG_MEMCG or memcg disabled), the shrinker's nr_deferred > > > > > > > would be used. And non memcg aware shrinkers use shrinker's nr_deferred all the time. > > > > > > > > > > > > > > Signed-off-by: Yang Shi <shy828301@gmail.com> > > > > > > > --- > > > > > > > include/linux/memcontrol.h | 9 +++ > > > > > > > mm/memcontrol.c | 112 ++++++++++++++++++++++++++++++++++++- > > > > > > > mm/vmscan.c | 4 ++ > > > > > > > 3 files changed, 123 insertions(+), 2 deletions(-) > > > > > > > > > > > > > > diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h > > > > > > > index 922a7f600465..1b343b268359 100644 > > > > > > > --- a/include/linux/memcontrol.h > > > > > > > +++ b/include/linux/memcontrol.h > > > > > > > @@ -92,6 +92,13 @@ struct lruvec_stat { > > > > > > > long count[NR_VM_NODE_STAT_ITEMS]; > > > > > > > }; > > > > > > > > > > > > > > + > > > > > > > +/* Shrinker::id indexed nr_deferred of memcg-aware shrinkers. */ > > > > > > > +struct memcg_shrinker_deferred { > > > > > > > + struct rcu_head rcu; > > > > > > > + atomic_long_t nr_deferred[]; > > > > > > > +}; > > > > > > > > > > > > The idea makes total sense to me. But I wonder if we can add nr_deferred to > > > > > > struct list_lru_one, instead of adding another per-memcg per-shrinker entity? > > > > > > I guess it can simplify the code quite a lot. What do you think? > > > > > > > > > > Aha, actually this exactly was what I did at the first place. But Dave > > > > > NAK'ed this approach. You can find the discussion at: > > > > > https://lore.kernel.org/linux-mm/20200930073152.GH12096@dread.disaster.area/. > > > > > > Yes, this makes sense for me. Thank you for the link! > > > > > > > > > > > I did prototypes for both approaches (move nr_deferred to list_lru or > > > > to memcg). I preferred the list_lru approach at the first place. But > > > > Dave's opinion does make perfect sense to me. So I dropped that > > > > list_lru one. That email elaborated why moving nr_deferred to list_lru > > > > is not appropriate. > > > > > > Hm, shouldn't we move list_lru to memcg then? It's not directly related > > > to your patchset, but maybe it's something we should consider in the future. > > > > I haven't thought about this yet. I agree we could look into it > > further later on. > > > > > > > > What worries me is that with your patchset we'll have 3 separate > > > per-memcg (per-node) per-shrinker entity, each with slightly different > > > approach to allocate/extend/reparent/release. So it begs for some > > > unification. I don't think it's a showstopper for your work though, it > > > can be done later. > > > > Off the top of my head, we may be able to have shrinker_info struct, > > it should look like: > > > > struct shrinker_info { > > atomic_long_t nr_deferred; > > /* Just one bit is used now */ > > u8 map:1; > > } > > > > struct memcg_shrinker_info { > > struct rcu_head rcu; > > /* Indexed by shrinker ID */ > > struct shrinker_info info[]; > > } > > > > Then in struct mem_cgroup_per_node, we could have: > > > > struct mem_cgroup_per_node { > > .... > > struct memcg_shrinker_info __rcu *shrinker_info; > > .... > > } > > > > In this way shrinker_info should be allocated to all memcgs, including > > root. But shrinker could ignore root's map bit. We may waste a little > > bit memory, but we get unification. > > > > Would that work? > > Hm, not exactly, then you'll an ability to iterate with > for_each_set_bit(i, map->map, shrinker_nr_max)... Instead we could just iterate each shrinker_info struct to check if its map is set. > But you can probably do something like: > > struct shrinker_info { > atomic_long_t nr_deferred; > > struct list_lru_one[]; /* optional, depends on the shrinker implementation */ > }; > > struct memcg_shrinker_info { > /* Indexed by shrinker ID */ > unsigned long *map[]; > struct shrinker_info *shrinker_info[]; Both map and shrinker_info has to be extendable, so they have to be struct with rcu_head. So actually it is the same with separate shrinker_map and shrinker_deferred, but under one struct. Actually I tried this in my prototype, but I gave up it since it didn't simplify the code IMHO. > } > > Then you'll be able to allocate individual shrinker_info structures on-demand. > > But, please, take this all with a grain of salt, I didn't check if it's all really > possible or there are some obstacles. Thanks a lot for all the kind suggestions. I'd agree with you we could revisit the unification later on. It seems there is not a simple and straightforward way to unify them at the first glance. There might be more evils in the detail. > > Thanks!
On Wed, Dec 02, 2020 at 10:27:21AM -0800, Yang Shi wrote: > @@ -504,6 +577,34 @@ int memcg_expand_shrinker_maps(int new_id) > return ret; > } > > +int memcg_expand_shrinker_deferred(int new_id) > +{ > + int size, old_size, ret = 0; > + struct mem_cgroup *memcg; > + > + size = (new_id + 1) * sizeof(atomic_long_t); > + old_size = memcg_shrinker_deferred_size; > + if (size <= old_size) > + return 0; > + > + mutex_lock(&memcg_shrinker_mutex); The locking is somewhat confusing. I was wondering why we first read memcg_shrinker_deferred_size "locklessly", then change it while holding the &memcg_shrinker_mutex. memcg_shrinker_deferred_size only changes under shrinker_rwsem(write), correct? This should be documented in a comment, IMO. memcg_shrinker_mutex looks superfluous then. The memcg allocation path is the read-side of memcg_shrinker_deferred_size, and so simply needs to take shrinker_rwsem(read) to lock out shrinker (de)registration. Also, isn't memcg_shrinker_deferred_size just shrinker_nr_max? And memcg_expand_shrinker_deferred() is only called when size >= old_size in the first place (because id >= shrinker_nr_max)?
On Thu, Dec 10, 2020 at 7:36 AM Johannes Weiner <hannes@cmpxchg.org> wrote: > > On Wed, Dec 02, 2020 at 10:27:21AM -0800, Yang Shi wrote: > > @@ -504,6 +577,34 @@ int memcg_expand_shrinker_maps(int new_id) > > return ret; > > } > > > > +int memcg_expand_shrinker_deferred(int new_id) > > +{ > > + int size, old_size, ret = 0; > > + struct mem_cgroup *memcg; > > + > > + size = (new_id + 1) * sizeof(atomic_long_t); > > + old_size = memcg_shrinker_deferred_size; > > + if (size <= old_size) > > + return 0; > > + > > + mutex_lock(&memcg_shrinker_mutex); > > The locking is somewhat confusing. I was wondering why we first read > memcg_shrinker_deferred_size "locklessly", then change it while > holding the &memcg_shrinker_mutex. > > memcg_shrinker_deferred_size only changes under shrinker_rwsem(write), > correct? This should be documented in a comment, IMO. Yes, it is correct. > > memcg_shrinker_mutex looks superfluous then. The memcg allocation path > is the read-side of memcg_shrinker_deferred_size, and so simply needs > to take shrinker_rwsem(read) to lock out shrinker (de)registration. I see you point. Yes, it seems shrinker_{maps|deferred} allocation could be synchronized with shrinker registration by shrinker_rwsem. memcg_shrinker_mutex is just renamed from memcg_shrinker_map_mutex which was introduced by shrinker_maps patchset. I'm not quite sure why this mutex was introduced at the first place, I guess the main purpose is to *not* exacerbate the contention of shrinker_rwsem? If that contention is not a concern, we could remove that dedicated mutex. > > Also, isn't memcg_shrinker_deferred_size just shrinker_nr_max? And No, it is variable. It is nr * sizeof(atomit_long_t). The nr is the current last shrinker ID. If a new shrinker is registered, the nr may grow. > memcg_expand_shrinker_deferred() is only called when size >= old_size > in the first place (because id >= shrinker_nr_max)? Yes.
On Thu, Dec 10, 2020 at 7:36 AM Johannes Weiner <hannes@cmpxchg.org> wrote: > > On Wed, Dec 02, 2020 at 10:27:21AM -0800, Yang Shi wrote: > > @@ -504,6 +577,34 @@ int memcg_expand_shrinker_maps(int new_id) > > return ret; > > } > > > > +int memcg_expand_shrinker_deferred(int new_id) > > +{ > > + int size, old_size, ret = 0; > > + struct mem_cgroup *memcg; > > + > > + size = (new_id + 1) * sizeof(atomic_long_t); > > + old_size = memcg_shrinker_deferred_size; > > + if (size <= old_size) > > + return 0; > > + > > + mutex_lock(&memcg_shrinker_mutex); > > The locking is somewhat confusing. I was wondering why we first read > memcg_shrinker_deferred_size "locklessly", then change it while > holding the &memcg_shrinker_mutex. The concurrent shrinkers registration may have race. But, they should get different IDs, so it seems not matter. I agree it is a little bit confusing and not that straightforward, it does owe some comments in the code. > > memcg_shrinker_deferred_size only changes under shrinker_rwsem(write), > correct? This should be documented in a comment, IMO. > > memcg_shrinker_mutex looks superfluous then. The memcg allocation path > is the read-side of memcg_shrinker_deferred_size, and so simply needs > to take shrinker_rwsem(read) to lock out shrinker (de)registration. > > Also, isn't memcg_shrinker_deferred_size just shrinker_nr_max? And > memcg_expand_shrinker_deferred() is only called when size >= old_size > in the first place (because id >= shrinker_nr_max)?
On Thu, Dec 10, 2020 at 11:12 AM Yang Shi <shy828301@gmail.com> wrote: > > On Thu, Dec 10, 2020 at 7:36 AM Johannes Weiner <hannes@cmpxchg.org> wrote: > > > > On Wed, Dec 02, 2020 at 10:27:21AM -0800, Yang Shi wrote: > > > @@ -504,6 +577,34 @@ int memcg_expand_shrinker_maps(int new_id) > > > return ret; > > > } > > > > > > +int memcg_expand_shrinker_deferred(int new_id) > > > +{ > > > + int size, old_size, ret = 0; > > > + struct mem_cgroup *memcg; > > > + > > > + size = (new_id + 1) * sizeof(atomic_long_t); > > > + old_size = memcg_shrinker_deferred_size; > > > + if (size <= old_size) > > > + return 0; > > > + > > > + mutex_lock(&memcg_shrinker_mutex); > > > > The locking is somewhat confusing. I was wondering why we first read > > memcg_shrinker_deferred_size "locklessly", then change it while > > holding the &memcg_shrinker_mutex. > > > > memcg_shrinker_deferred_size only changes under shrinker_rwsem(write), > > correct? This should be documented in a comment, IMO. > > Yes, it is correct. > > > > > memcg_shrinker_mutex looks superfluous then. The memcg allocation path > > is the read-side of memcg_shrinker_deferred_size, and so simply needs > > to take shrinker_rwsem(read) to lock out shrinker (de)registration. > > I see you point. Yes, it seems shrinker_{maps|deferred} allocation > could be synchronized with shrinker registration by shrinker_rwsem. > > memcg_shrinker_mutex is just renamed from memcg_shrinker_map_mutex > which was introduced by shrinker_maps patchset. I'm not quite sure why > this mutex was introduced at the first place, I guess the main purpose > is to *not* exacerbate the contention of shrinker_rwsem? > > If that contention is not a concern, we could remove that dedicated mutex. It seems using shrinker_rwsem instead of dedicated mutex should not exacerbate the contention since we just add one read critical section. Will do it in v2. > > > > > Also, isn't memcg_shrinker_deferred_size just shrinker_nr_max? And > > No, it is variable. It is nr * sizeof(atomit_long_t). The nr is the > current last shrinker ID. If a new shrinker is registered, the nr may > grow. > > > memcg_expand_shrinker_deferred() is only called when size >= old_size > > in the first place (because id >= shrinker_nr_max)? > > Yes.
diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h index 922a7f600465..1b343b268359 100644 --- a/include/linux/memcontrol.h +++ b/include/linux/memcontrol.h @@ -92,6 +92,13 @@ struct lruvec_stat { long count[NR_VM_NODE_STAT_ITEMS]; }; + +/* Shrinker::id indexed nr_deferred of memcg-aware shrinkers. */ +struct memcg_shrinker_deferred { + struct rcu_head rcu; + atomic_long_t nr_deferred[]; +}; + /* * Bitmap of shrinker::id corresponding to memcg-aware shrinkers, * which have elements charged to this memcg. @@ -119,6 +126,7 @@ struct mem_cgroup_per_node { struct mem_cgroup_reclaim_iter iter; struct memcg_shrinker_map __rcu *shrinker_map; + struct memcg_shrinker_deferred __rcu *shrinker_deferred; struct rb_node tree_node; /* RB tree node */ unsigned long usage_in_excess;/* Set to the value by which */ @@ -1489,6 +1497,7 @@ static inline bool mem_cgroup_under_socket_pressure(struct mem_cgroup *memcg) } extern int memcg_expand_shrinker_maps(int new_id); +extern int memcg_expand_shrinker_deferred(int new_id); extern void memcg_set_shrinker_bit(struct mem_cgroup *memcg, int nid, int shrinker_id); diff --git a/mm/memcontrol.c b/mm/memcontrol.c index 19e41684c96b..d3d5c88db179 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -395,6 +395,8 @@ EXPORT_SYMBOL(memcg_kmem_enabled_key); #endif static int memcg_shrinker_map_size; +static int memcg_shrinker_deferred_size; + static DEFINE_MUTEX(memcg_shrinker_mutex); static void memcg_free_shrinker_map_rcu(struct rcu_head *head) @@ -402,6 +404,11 @@ static void memcg_free_shrinker_map_rcu(struct rcu_head *head) kvfree(container_of(head, struct memcg_shrinker_map, rcu)); } +static void memcg_free_shrinker_deferred_rcu(struct rcu_head *head) +{ + kvfree(container_of(head, struct memcg_shrinker_deferred, rcu)); +} + static int memcg_expand_one_shrinker_map(struct mem_cgroup *memcg, int size, int old_size) { @@ -432,6 +439,36 @@ static int memcg_expand_one_shrinker_map(struct mem_cgroup *memcg, return 0; } +static int memcg_expand_one_shrinker_deferred(struct mem_cgroup *memcg, + int size, int old_size) +{ + struct memcg_shrinker_deferred *new, *old; + int nid; + + lockdep_assert_held(&memcg_shrinker_mutex); + + for_each_node(nid) { + old = rcu_dereference_protected( + mem_cgroup_nodeinfo(memcg, nid)->shrinker_deferred, true); + /* Not yet online memcg */ + if (!old) + return 0; + + new = kvmalloc_node(sizeof(*new) + size, GFP_KERNEL, nid); + if (!new) + return -ENOMEM; + + /* Copy all old values, and clear all new ones */ + memcpy((void *)new->nr_deferred, (void *)old->nr_deferred, old_size); + memset((void *)new->nr_deferred + old_size, 0, size - old_size); + + rcu_assign_pointer(memcg->nodeinfo[nid]->shrinker_deferred, new); + call_rcu(&old->rcu, memcg_free_shrinker_deferred_rcu); + } + + return 0; +} + static void memcg_free_shrinker_maps(struct mem_cgroup *memcg) { struct mem_cgroup_per_node *pn; @@ -450,6 +487,21 @@ static void memcg_free_shrinker_maps(struct mem_cgroup *memcg) } } +static void memcg_free_shrinker_deferred(struct mem_cgroup *memcg) +{ + struct mem_cgroup_per_node *pn; + struct memcg_shrinker_deferred *deferred; + int nid; + + for_each_node(nid) { + pn = mem_cgroup_nodeinfo(memcg, nid); + deferred = rcu_dereference_protected(pn->shrinker_deferred, true); + if (deferred) + kvfree(deferred); + rcu_assign_pointer(pn->shrinker_deferred, NULL); + } +} + static int memcg_alloc_shrinker_maps(struct mem_cgroup *memcg) { struct memcg_shrinker_map *map; @@ -474,6 +526,27 @@ static int memcg_alloc_shrinker_maps(struct mem_cgroup *memcg) return ret; } +static int memcg_alloc_shrinker_deferred(struct mem_cgroup *memcg) +{ + struct memcg_shrinker_deferred *deferred; + int nid, size, ret = 0; + + mutex_lock(&memcg_shrinker_mutex); + size = memcg_shrinker_deferred_size; + for_each_node(nid) { + deferred = kvzalloc_node(sizeof(*deferred) + size, GFP_KERNEL, nid); + if (!deferred) { + memcg_free_shrinker_deferred(memcg); + ret = -ENOMEM; + break; + } + rcu_assign_pointer(memcg->nodeinfo[nid]->shrinker_deferred, deferred); + } + mutex_unlock(&memcg_shrinker_mutex); + + return ret; +} + int memcg_expand_shrinker_maps(int new_id) { int size, old_size, ret = 0; @@ -504,6 +577,34 @@ int memcg_expand_shrinker_maps(int new_id) return ret; } +int memcg_expand_shrinker_deferred(int new_id) +{ + int size, old_size, ret = 0; + struct mem_cgroup *memcg; + + size = (new_id + 1) * sizeof(atomic_long_t); + old_size = memcg_shrinker_deferred_size; + if (size <= old_size) + return 0; + + mutex_lock(&memcg_shrinker_mutex); + if (!root_mem_cgroup) + goto unlock; + + for_each_mem_cgroup(memcg) { + ret = memcg_expand_one_shrinker_deferred(memcg, size, old_size); + if (ret) { + mem_cgroup_iter_break(NULL, memcg); + goto unlock; + } + } +unlock: + if (!ret) + memcg_shrinker_deferred_size = size; + mutex_unlock(&memcg_shrinker_mutex); + return ret; +} + void memcg_set_shrinker_bit(struct mem_cgroup *memcg, int nid, int shrinker_id) { if (shrinker_id >= 0 && memcg && !mem_cgroup_is_root(memcg)) { @@ -5400,8 +5501,8 @@ static int mem_cgroup_css_online(struct cgroup_subsys_state *css) struct mem_cgroup *memcg = mem_cgroup_from_css(css); /* - * A memcg must be visible for memcg_expand_shrinker_maps() - * by the time the maps are allocated. So, we allocate maps + * A memcg must be visible for memcg_expand_shrinker_{maps|deferred}() + * by the time the maps are allocated. So, we allocate maps and deferred * here, when for_each_mem_cgroup() can't skip it. */ if (memcg_alloc_shrinker_maps(memcg)) { @@ -5409,6 +5510,12 @@ static int mem_cgroup_css_online(struct cgroup_subsys_state *css) return -ENOMEM; } + if (memcg_alloc_shrinker_deferred(memcg)) { + memcg_free_shrinker_maps(memcg); + mem_cgroup_id_remove(memcg); + return -ENOMEM; + } + /* Online state pins memcg ID, memcg ID pins CSS */ refcount_set(&memcg->id.ref, 1); css_get(css); @@ -5469,6 +5576,7 @@ static void mem_cgroup_css_free(struct cgroup_subsys_state *css) cancel_work_sync(&memcg->high_work); mem_cgroup_remove_from_trees(memcg); memcg_free_shrinker_maps(memcg); + memcg_free_shrinker_deferred(memcg); memcg_free_kmem(memcg); mem_cgroup_free(memcg); } diff --git a/mm/vmscan.c b/mm/vmscan.c index 0d628299e55c..cba0bc8d4661 100644 --- a/mm/vmscan.c +++ b/mm/vmscan.c @@ -219,6 +219,10 @@ static int prealloc_memcg_shrinker(struct shrinker *shrinker) goto unlock; } + if (memcg_expand_shrinker_deferred(id)) { + idr_remove(&shrinker_idr, id); + goto unlock; + } shrinker_nr_max = id + 1; } shrinker->id = id;
Currently the number of deferred objects are per shrinker, but some slabs, for example, vfs inode/dentry cache are per memcg, this would result in poor isolation among memcgs. The deferred objects typically are generated by __GFP_NOFS allocations, one memcg with excessive __GFP_NOFS allocations may blow up deferred objects, then other innocent memcgs may suffer from over shrink, excessive reclaim latency, etc. For example, two workloads run in memcgA and memcgB respectively, workload in B is vfs heavy workload. Workload in A generates excessive deferred objects, then B's vfs cache might be hit heavily (drop half of caches) by B's limit reclaim or global reclaim. We observed this hit in our production environment which was running vfs heavy workload shown as the below tracing log: <...>-409454 [016] .... 28286961.747146: mm_shrink_slab_start: super_cache_scan+0x0/0x1a0 ffff9a83046f3458: nid: 1 objects to shrink 3641681686040 gfp_flags GFP_HIGHUSER_MOVABLE|__GFP_ZERO pgs_scanned 1 lru_pgs 15721 cache items 246404277 delta 31345 total_scan 123202138 <...>-409454 [022] .... 28287105.928018: mm_shrink_slab_end: super_cache_scan+0x0/0x1a0 ffff9a83046f3458: nid: 1 unused scan count 3641681686040 new scan count 3641798379189 total_scan 602 last shrinker return val 123186855 The vfs cache and page cache ration was 10:1 on this machine, and half of caches were dropped. This also resulted in significant amount of page caches were dropped due to inodes eviction. Make nr_deferred per memcg for memcg aware shrinkers would solve the unfairness and bring better isolation. When memcg is not enabled (!CONFIG_MEMCG or memcg disabled), the shrinker's nr_deferred would be used. And non memcg aware shrinkers use shrinker's nr_deferred all the time. Signed-off-by: Yang Shi <shy828301@gmail.com> --- include/linux/memcontrol.h | 9 +++ mm/memcontrol.c | 112 ++++++++++++++++++++++++++++++++++++- mm/vmscan.c | 4 ++ 3 files changed, 123 insertions(+), 2 deletions(-)