| Message ID | 20210209174646.1310591-6-shy828301@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v7,01/12] mm: vmscan: use nid from shrink_control for tracepoint | expand |
On Tue, Feb 09, 2021 at 09:46:39AM -0800, Yang Shi wrote: > The following patch is going to add nr_deferred into shrinker_map, the change will > make shrinker_map not only include map anymore, so rename it to "memcg_shrinker_info". > And this should make the patch adding nr_deferred cleaner and readable and make > review easier. Also remove the "memcg_" prefix. > > Acked-by: Vlastimil Babka <vbabka@suse.cz> > Acked-by: Kirill Tkhai <ktkhai@virtuozzo.com> > Signed-off-by: Yang Shi <shy828301@gmail.com> > --- > include/linux/memcontrol.h | 8 ++--- > mm/memcontrol.c | 6 ++-- > mm/vmscan.c | 62 +++++++++++++++++++------------------- > 3 files changed, 38 insertions(+), 38 deletions(-) > > diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h > index 1739f17e0939..4c9253896e25 100644 > --- a/include/linux/memcontrol.h > +++ b/include/linux/memcontrol.h > @@ -96,7 +96,7 @@ struct lruvec_stat { > * Bitmap of shrinker::id corresponding to memcg-aware shrinkers, > * which have elements charged to this memcg. > */ > -struct memcg_shrinker_map { > +struct shrinker_info { > struct rcu_head rcu; > unsigned long map[]; > }; > @@ -118,7 +118,7 @@ struct mem_cgroup_per_node { > > struct mem_cgroup_reclaim_iter iter; > > - struct memcg_shrinker_map __rcu *shrinker_map; > + struct shrinker_info __rcu *shrinker_info; Nice! I really like how it looks now in comparison to the v1. Thank you for working on it! > > struct rb_node tree_node; /* RB tree node */ > unsigned long usage_in_excess;/* Set to the value by which */ > @@ -1581,8 +1581,8 @@ static inline bool mem_cgroup_under_socket_pressure(struct mem_cgroup *memcg) > return false; > } > > -int alloc_shrinker_maps(struct mem_cgroup *memcg); > -void free_shrinker_maps(struct mem_cgroup *memcg); > +int alloc_shrinker_info(struct mem_cgroup *memcg); > +void free_shrinker_info(struct mem_cgroup *memcg); > void set_shrinker_bit(struct mem_cgroup *memcg, int nid, int shrinker_id); > #else > #define mem_cgroup_sockets_enabled 0 > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index f5c9a0d2160b..f64ad0d044d9 100644 > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -5246,11 +5246,11 @@ 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 expand_shrinker_maps() > + * A memcg must be visible for expand_shrinker_info() > * by the time the maps are allocated. So, we allocate maps > * here, when for_each_mem_cgroup() can't skip it. > */ > - if (alloc_shrinker_maps(memcg)) { > + if (alloc_shrinker_info(memcg)) { > mem_cgroup_id_remove(memcg); > return -ENOMEM; > } > @@ -5314,7 +5314,7 @@ static void mem_cgroup_css_free(struct cgroup_subsys_state *css) > vmpressure_cleanup(&memcg->vmpressure); > cancel_work_sync(&memcg->high_work); > mem_cgroup_remove_from_trees(memcg); > - free_shrinker_maps(memcg); > + free_shrinker_info(memcg); > memcg_free_kmem(memcg); > mem_cgroup_free(memcg); > } > diff --git a/mm/vmscan.c b/mm/vmscan.c > index 641077b09e5d..9436f9246d32 100644 > --- a/mm/vmscan.c > +++ b/mm/vmscan.c > @@ -190,20 +190,20 @@ static int shrinker_nr_max; > #define NR_MAX_TO_SHR_MAP_SIZE(nr_max) \ > (DIV_ROUND_UP(nr_max, BITS_PER_LONG) * sizeof(unsigned long)) > > -static void free_shrinker_map_rcu(struct rcu_head *head) > +static void free_shrinker_info_rcu(struct rcu_head *head) > { > - kvfree(container_of(head, struct memcg_shrinker_map, rcu)); > + kvfree(container_of(head, struct shrinker_info, rcu)); > } > > -static int expand_one_shrinker_map(struct mem_cgroup *memcg, > +static int expand_one_shrinker_info(struct mem_cgroup *memcg, > int size, int old_size) > { > - struct memcg_shrinker_map *new, *old; > + struct shrinker_info *new, *old; > int nid; > > for_each_node(nid) { > old = rcu_dereference_protected( > - mem_cgroup_nodeinfo(memcg, nid)->shrinker_map, true); > + mem_cgroup_nodeinfo(memcg, nid)->shrinker_info, true); > /* Not yet online memcg */ > if (!old) > return 0; > @@ -216,17 +216,17 @@ static int expand_one_shrinker_map(struct mem_cgroup *memcg, > memset(new->map, (int)0xff, old_size); > memset((void *)new->map + old_size, 0, size - old_size); > > - rcu_assign_pointer(memcg->nodeinfo[nid]->shrinker_map, new); > - call_rcu(&old->rcu, free_shrinker_map_rcu); > + rcu_assign_pointer(memcg->nodeinfo[nid]->shrinker_info, new); > + call_rcu(&old->rcu, free_shrinker_info_rcu); Why not use kvfree_rcu() and get rid of free_shrinker_info_rcu() callback? Aside from this minor thing, the patch looks good to me. Please, feel free to add Acked-by: Roman Gushchin <guro@fb.com> Thanks!
On Tue, Feb 9, 2021 at 12:50 PM Roman Gushchin <guro@fb.com> wrote: > > On Tue, Feb 09, 2021 at 09:46:39AM -0800, Yang Shi wrote: > > The following patch is going to add nr_deferred into shrinker_map, the change will > > make shrinker_map not only include map anymore, so rename it to "memcg_shrinker_info". > > And this should make the patch adding nr_deferred cleaner and readable and make > > review easier. Also remove the "memcg_" prefix. > > > > Acked-by: Vlastimil Babka <vbabka@suse.cz> > > Acked-by: Kirill Tkhai <ktkhai@virtuozzo.com> > > Signed-off-by: Yang Shi <shy828301@gmail.com> > > --- > > include/linux/memcontrol.h | 8 ++--- > > mm/memcontrol.c | 6 ++-- > > mm/vmscan.c | 62 +++++++++++++++++++------------------- > > 3 files changed, 38 insertions(+), 38 deletions(-) > > > > diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h > > index 1739f17e0939..4c9253896e25 100644 > > --- a/include/linux/memcontrol.h > > +++ b/include/linux/memcontrol.h > > @@ -96,7 +96,7 @@ struct lruvec_stat { > > * Bitmap of shrinker::id corresponding to memcg-aware shrinkers, > > * which have elements charged to this memcg. > > */ > > -struct memcg_shrinker_map { > > +struct shrinker_info { > > struct rcu_head rcu; > > unsigned long map[]; > > }; > > @@ -118,7 +118,7 @@ struct mem_cgroup_per_node { > > > > struct mem_cgroup_reclaim_iter iter; > > > > - struct memcg_shrinker_map __rcu *shrinker_map; > > + struct shrinker_info __rcu *shrinker_info; > > Nice! > > I really like how it looks now in comparison to the v1. Thank you for > working on it! Thanks a lot for all the great comments from all of you. > > > > > struct rb_node tree_node; /* RB tree node */ > > unsigned long usage_in_excess;/* Set to the value by which */ > > @@ -1581,8 +1581,8 @@ static inline bool mem_cgroup_under_socket_pressure(struct mem_cgroup *memcg) > > return false; > > } > > > > -int alloc_shrinker_maps(struct mem_cgroup *memcg); > > -void free_shrinker_maps(struct mem_cgroup *memcg); > > +int alloc_shrinker_info(struct mem_cgroup *memcg); > > +void free_shrinker_info(struct mem_cgroup *memcg); > > void set_shrinker_bit(struct mem_cgroup *memcg, int nid, int shrinker_id); > > #else > > #define mem_cgroup_sockets_enabled 0 > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > > index f5c9a0d2160b..f64ad0d044d9 100644 > > --- a/mm/memcontrol.c > > +++ b/mm/memcontrol.c > > @@ -5246,11 +5246,11 @@ 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 expand_shrinker_maps() > > + * A memcg must be visible for expand_shrinker_info() > > * by the time the maps are allocated. So, we allocate maps > > * here, when for_each_mem_cgroup() can't skip it. > > */ > > - if (alloc_shrinker_maps(memcg)) { > > + if (alloc_shrinker_info(memcg)) { > > mem_cgroup_id_remove(memcg); > > return -ENOMEM; > > } > > @@ -5314,7 +5314,7 @@ static void mem_cgroup_css_free(struct cgroup_subsys_state *css) > > vmpressure_cleanup(&memcg->vmpressure); > > cancel_work_sync(&memcg->high_work); > > mem_cgroup_remove_from_trees(memcg); > > - free_shrinker_maps(memcg); > > + free_shrinker_info(memcg); > > memcg_free_kmem(memcg); > > mem_cgroup_free(memcg); > > } > > diff --git a/mm/vmscan.c b/mm/vmscan.c > > index 641077b09e5d..9436f9246d32 100644 > > --- a/mm/vmscan.c > > +++ b/mm/vmscan.c > > @@ -190,20 +190,20 @@ static int shrinker_nr_max; > > #define NR_MAX_TO_SHR_MAP_SIZE(nr_max) \ > > (DIV_ROUND_UP(nr_max, BITS_PER_LONG) * sizeof(unsigned long)) > > > > -static void free_shrinker_map_rcu(struct rcu_head *head) > > +static void free_shrinker_info_rcu(struct rcu_head *head) > > { > > - kvfree(container_of(head, struct memcg_shrinker_map, rcu)); > > + kvfree(container_of(head, struct shrinker_info, rcu)); > > } > > > > -static int expand_one_shrinker_map(struct mem_cgroup *memcg, > > +static int expand_one_shrinker_info(struct mem_cgroup *memcg, > > int size, int old_size) > > { > > - struct memcg_shrinker_map *new, *old; > > + struct shrinker_info *new, *old; > > int nid; > > > > for_each_node(nid) { > > old = rcu_dereference_protected( > > - mem_cgroup_nodeinfo(memcg, nid)->shrinker_map, true); > > + mem_cgroup_nodeinfo(memcg, nid)->shrinker_info, true); > > /* Not yet online memcg */ > > if (!old) > > return 0; > > @@ -216,17 +216,17 @@ static int expand_one_shrinker_map(struct mem_cgroup *memcg, > > memset(new->map, (int)0xff, old_size); > > memset((void *)new->map + old_size, 0, size - old_size); > > > > - rcu_assign_pointer(memcg->nodeinfo[nid]->shrinker_map, new); > > - call_rcu(&old->rcu, free_shrinker_map_rcu); > > + rcu_assign_pointer(memcg->nodeinfo[nid]->shrinker_info, new); > > + call_rcu(&old->rcu, free_shrinker_info_rcu); > > Why not use kvfree_rcu() and get rid of free_shrinker_info_rcu() callback? Just because this patch is aimed to rename the structure. I think it may be more preferred to have the cleanup in a separate patch? > > Aside from this minor thing, the patch looks good to me. Please, feel free to add > Acked-by: Roman Gushchin <guro@fb.com> > > Thanks!
On Tue, Feb 09, 2021 at 03:33:56PM -0800, Yang Shi wrote: > On Tue, Feb 9, 2021 at 12:50 PM Roman Gushchin <guro@fb.com> wrote: > > > > On Tue, Feb 09, 2021 at 09:46:39AM -0800, Yang Shi wrote: > > > The following patch is going to add nr_deferred into shrinker_map, the change will > > > make shrinker_map not only include map anymore, so rename it to "memcg_shrinker_info". > > > And this should make the patch adding nr_deferred cleaner and readable and make > > > review easier. Also remove the "memcg_" prefix. > > > > > > Acked-by: Vlastimil Babka <vbabka@suse.cz> > > > Acked-by: Kirill Tkhai <ktkhai@virtuozzo.com> > > > Signed-off-by: Yang Shi <shy828301@gmail.com> > > > --- > > > include/linux/memcontrol.h | 8 ++--- > > > mm/memcontrol.c | 6 ++-- > > > mm/vmscan.c | 62 +++++++++++++++++++------------------- > > > 3 files changed, 38 insertions(+), 38 deletions(-) > > > > > > diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h > > > index 1739f17e0939..4c9253896e25 100644 > > > --- a/include/linux/memcontrol.h > > > +++ b/include/linux/memcontrol.h > > > @@ -96,7 +96,7 @@ struct lruvec_stat { > > > * Bitmap of shrinker::id corresponding to memcg-aware shrinkers, > > > * which have elements charged to this memcg. > > > */ > > > -struct memcg_shrinker_map { > > > +struct shrinker_info { > > > struct rcu_head rcu; > > > unsigned long map[]; > > > }; > > > @@ -118,7 +118,7 @@ struct mem_cgroup_per_node { > > > > > > struct mem_cgroup_reclaim_iter iter; > > > > > > - struct memcg_shrinker_map __rcu *shrinker_map; > > > + struct shrinker_info __rcu *shrinker_info; > > > > Nice! > > > > I really like how it looks now in comparison to the v1. Thank you for > > working on it! > > Thanks a lot for all the great comments from all of you. > > > > > > > > > struct rb_node tree_node; /* RB tree node */ > > > unsigned long usage_in_excess;/* Set to the value by which */ > > > @@ -1581,8 +1581,8 @@ static inline bool mem_cgroup_under_socket_pressure(struct mem_cgroup *memcg) > > > return false; > > > } > > > > > > -int alloc_shrinker_maps(struct mem_cgroup *memcg); > > > -void free_shrinker_maps(struct mem_cgroup *memcg); > > > +int alloc_shrinker_info(struct mem_cgroup *memcg); > > > +void free_shrinker_info(struct mem_cgroup *memcg); > > > void set_shrinker_bit(struct mem_cgroup *memcg, int nid, int shrinker_id); > > > #else > > > #define mem_cgroup_sockets_enabled 0 > > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > > > index f5c9a0d2160b..f64ad0d044d9 100644 > > > --- a/mm/memcontrol.c > > > +++ b/mm/memcontrol.c > > > @@ -5246,11 +5246,11 @@ 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 expand_shrinker_maps() > > > + * A memcg must be visible for expand_shrinker_info() > > > * by the time the maps are allocated. So, we allocate maps > > > * here, when for_each_mem_cgroup() can't skip it. > > > */ > > > - if (alloc_shrinker_maps(memcg)) { > > > + if (alloc_shrinker_info(memcg)) { > > > mem_cgroup_id_remove(memcg); > > > return -ENOMEM; > > > } > > > @@ -5314,7 +5314,7 @@ static void mem_cgroup_css_free(struct cgroup_subsys_state *css) > > > vmpressure_cleanup(&memcg->vmpressure); > > > cancel_work_sync(&memcg->high_work); > > > mem_cgroup_remove_from_trees(memcg); > > > - free_shrinker_maps(memcg); > > > + free_shrinker_info(memcg); > > > memcg_free_kmem(memcg); > > > mem_cgroup_free(memcg); > > > } > > > diff --git a/mm/vmscan.c b/mm/vmscan.c > > > index 641077b09e5d..9436f9246d32 100644 > > > --- a/mm/vmscan.c > > > +++ b/mm/vmscan.c > > > @@ -190,20 +190,20 @@ static int shrinker_nr_max; > > > #define NR_MAX_TO_SHR_MAP_SIZE(nr_max) \ > > > (DIV_ROUND_UP(nr_max, BITS_PER_LONG) * sizeof(unsigned long)) > > > > > > -static void free_shrinker_map_rcu(struct rcu_head *head) > > > +static void free_shrinker_info_rcu(struct rcu_head *head) > > > { > > > - kvfree(container_of(head, struct memcg_shrinker_map, rcu)); > > > + kvfree(container_of(head, struct shrinker_info, rcu)); > > > } > > > > > > -static int expand_one_shrinker_map(struct mem_cgroup *memcg, > > > +static int expand_one_shrinker_info(struct mem_cgroup *memcg, > > > int size, int old_size) > > > { > > > - struct memcg_shrinker_map *new, *old; > > > + struct shrinker_info *new, *old; > > > int nid; > > > > > > for_each_node(nid) { > > > old = rcu_dereference_protected( > > > - mem_cgroup_nodeinfo(memcg, nid)->shrinker_map, true); > > > + mem_cgroup_nodeinfo(memcg, nid)->shrinker_info, true); > > > /* Not yet online memcg */ > > > if (!old) > > > return 0; > > > @@ -216,17 +216,17 @@ static int expand_one_shrinker_map(struct mem_cgroup *memcg, > > > memset(new->map, (int)0xff, old_size); > > > memset((void *)new->map + old_size, 0, size - old_size); > > > > > > - rcu_assign_pointer(memcg->nodeinfo[nid]->shrinker_map, new); > > > - call_rcu(&old->rcu, free_shrinker_map_rcu); > > > + rcu_assign_pointer(memcg->nodeinfo[nid]->shrinker_info, new); > > > + call_rcu(&old->rcu, free_shrinker_info_rcu); > > > > Why not use kvfree_rcu() and get rid of free_shrinker_info_rcu() callback? > > Just because this patch is aimed to rename the structure. I think it > may be more preferred to have the cleanup in a separate patch? Completely up to you, I'm fine with either option. Thanks!
On 10.02.2021 02:33, Yang Shi wrote: > On Tue, Feb 9, 2021 at 12:50 PM Roman Gushchin <guro@fb.com> wrote: >> >> On Tue, Feb 09, 2021 at 09:46:39AM -0800, Yang Shi wrote: >>> The following patch is going to add nr_deferred into shrinker_map, the change will >>> make shrinker_map not only include map anymore, so rename it to "memcg_shrinker_info". >>> And this should make the patch adding nr_deferred cleaner and readable and make >>> review easier. Also remove the "memcg_" prefix. >>> >>> Acked-by: Vlastimil Babka <vbabka@suse.cz> >>> Acked-by: Kirill Tkhai <ktkhai@virtuozzo.com> >>> Signed-off-by: Yang Shi <shy828301@gmail.com> >>> --- >>> include/linux/memcontrol.h | 8 ++--- >>> mm/memcontrol.c | 6 ++-- >>> mm/vmscan.c | 62 +++++++++++++++++++------------------- >>> 3 files changed, 38 insertions(+), 38 deletions(-) >>> >>> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h >>> index 1739f17e0939..4c9253896e25 100644 >>> --- a/include/linux/memcontrol.h >>> +++ b/include/linux/memcontrol.h >>> @@ -96,7 +96,7 @@ struct lruvec_stat { >>> * Bitmap of shrinker::id corresponding to memcg-aware shrinkers, >>> * which have elements charged to this memcg. >>> */ >>> -struct memcg_shrinker_map { >>> +struct shrinker_info { >>> struct rcu_head rcu; >>> unsigned long map[]; >>> }; >>> @@ -118,7 +118,7 @@ struct mem_cgroup_per_node { >>> >>> struct mem_cgroup_reclaim_iter iter; >>> >>> - struct memcg_shrinker_map __rcu *shrinker_map; >>> + struct shrinker_info __rcu *shrinker_info; >> >> Nice! >> >> I really like how it looks now in comparison to the v1. Thank you for >> working on it! > > Thanks a lot for all the great comments from all of you. > >> >>> >>> struct rb_node tree_node; /* RB tree node */ >>> unsigned long usage_in_excess;/* Set to the value by which */ >>> @@ -1581,8 +1581,8 @@ static inline bool mem_cgroup_under_socket_pressure(struct mem_cgroup *memcg) >>> return false; >>> } >>> >>> -int alloc_shrinker_maps(struct mem_cgroup *memcg); >>> -void free_shrinker_maps(struct mem_cgroup *memcg); >>> +int alloc_shrinker_info(struct mem_cgroup *memcg); >>> +void free_shrinker_info(struct mem_cgroup *memcg); >>> void set_shrinker_bit(struct mem_cgroup *memcg, int nid, int shrinker_id); >>> #else >>> #define mem_cgroup_sockets_enabled 0 >>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c >>> index f5c9a0d2160b..f64ad0d044d9 100644 >>> --- a/mm/memcontrol.c >>> +++ b/mm/memcontrol.c >>> @@ -5246,11 +5246,11 @@ 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 expand_shrinker_maps() >>> + * A memcg must be visible for expand_shrinker_info() >>> * by the time the maps are allocated. So, we allocate maps >>> * here, when for_each_mem_cgroup() can't skip it. >>> */ >>> - if (alloc_shrinker_maps(memcg)) { >>> + if (alloc_shrinker_info(memcg)) { >>> mem_cgroup_id_remove(memcg); >>> return -ENOMEM; >>> } >>> @@ -5314,7 +5314,7 @@ static void mem_cgroup_css_free(struct cgroup_subsys_state *css) >>> vmpressure_cleanup(&memcg->vmpressure); >>> cancel_work_sync(&memcg->high_work); >>> mem_cgroup_remove_from_trees(memcg); >>> - free_shrinker_maps(memcg); >>> + free_shrinker_info(memcg); >>> memcg_free_kmem(memcg); >>> mem_cgroup_free(memcg); >>> } >>> diff --git a/mm/vmscan.c b/mm/vmscan.c >>> index 641077b09e5d..9436f9246d32 100644 >>> --- a/mm/vmscan.c >>> +++ b/mm/vmscan.c >>> @@ -190,20 +190,20 @@ static int shrinker_nr_max; >>> #define NR_MAX_TO_SHR_MAP_SIZE(nr_max) \ >>> (DIV_ROUND_UP(nr_max, BITS_PER_LONG) * sizeof(unsigned long)) >>> >>> -static void free_shrinker_map_rcu(struct rcu_head *head) >>> +static void free_shrinker_info_rcu(struct rcu_head *head) >>> { >>> - kvfree(container_of(head, struct memcg_shrinker_map, rcu)); >>> + kvfree(container_of(head, struct shrinker_info, rcu)); >>> } >>> >>> -static int expand_one_shrinker_map(struct mem_cgroup *memcg, >>> +static int expand_one_shrinker_info(struct mem_cgroup *memcg, >>> int size, int old_size) >>> { >>> - struct memcg_shrinker_map *new, *old; >>> + struct shrinker_info *new, *old; >>> int nid; >>> >>> for_each_node(nid) { >>> old = rcu_dereference_protected( >>> - mem_cgroup_nodeinfo(memcg, nid)->shrinker_map, true); >>> + mem_cgroup_nodeinfo(memcg, nid)->shrinker_info, true); >>> /* Not yet online memcg */ >>> if (!old) >>> return 0; >>> @@ -216,17 +216,17 @@ static int expand_one_shrinker_map(struct mem_cgroup *memcg, >>> memset(new->map, (int)0xff, old_size); >>> memset((void *)new->map + old_size, 0, size - old_size); >>> >>> - rcu_assign_pointer(memcg->nodeinfo[nid]->shrinker_map, new); >>> - call_rcu(&old->rcu, free_shrinker_map_rcu); >>> + rcu_assign_pointer(memcg->nodeinfo[nid]->shrinker_info, new); >>> + call_rcu(&old->rcu, free_shrinker_info_rcu); >> >> Why not use kvfree_rcu() and get rid of free_shrinker_info_rcu() callback? > > Just because this patch is aimed to rename the structure. I think it > may be more preferred to have the cleanup in a separate patch? I'd voted for a separate patch
On Thu, Feb 11, 2021 at 8:47 AM Kirill Tkhai <ktkhai@virtuozzo.com> wrote: > > On 10.02.2021 02:33, Yang Shi wrote: > > On Tue, Feb 9, 2021 at 12:50 PM Roman Gushchin <guro@fb.com> wrote: > >> > >> On Tue, Feb 09, 2021 at 09:46:39AM -0800, Yang Shi wrote: > >>> The following patch is going to add nr_deferred into shrinker_map, the change will > >>> make shrinker_map not only include map anymore, so rename it to "memcg_shrinker_info". > >>> And this should make the patch adding nr_deferred cleaner and readable and make > >>> review easier. Also remove the "memcg_" prefix. > >>> > >>> Acked-by: Vlastimil Babka <vbabka@suse.cz> > >>> Acked-by: Kirill Tkhai <ktkhai@virtuozzo.com> > >>> Signed-off-by: Yang Shi <shy828301@gmail.com> > >>> --- > >>> include/linux/memcontrol.h | 8 ++--- > >>> mm/memcontrol.c | 6 ++-- > >>> mm/vmscan.c | 62 +++++++++++++++++++------------------- > >>> 3 files changed, 38 insertions(+), 38 deletions(-) > >>> > >>> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h > >>> index 1739f17e0939..4c9253896e25 100644 > >>> --- a/include/linux/memcontrol.h > >>> +++ b/include/linux/memcontrol.h > >>> @@ -96,7 +96,7 @@ struct lruvec_stat { > >>> * Bitmap of shrinker::id corresponding to memcg-aware shrinkers, > >>> * which have elements charged to this memcg. > >>> */ > >>> -struct memcg_shrinker_map { > >>> +struct shrinker_info { > >>> struct rcu_head rcu; > >>> unsigned long map[]; > >>> }; > >>> @@ -118,7 +118,7 @@ struct mem_cgroup_per_node { > >>> > >>> struct mem_cgroup_reclaim_iter iter; > >>> > >>> - struct memcg_shrinker_map __rcu *shrinker_map; > >>> + struct shrinker_info __rcu *shrinker_info; > >> > >> Nice! > >> > >> I really like how it looks now in comparison to the v1. Thank you for > >> working on it! > > > > Thanks a lot for all the great comments from all of you. > > > >> > >>> > >>> struct rb_node tree_node; /* RB tree node */ > >>> unsigned long usage_in_excess;/* Set to the value by which */ > >>> @@ -1581,8 +1581,8 @@ static inline bool mem_cgroup_under_socket_pressure(struct mem_cgroup *memcg) > >>> return false; > >>> } > >>> > >>> -int alloc_shrinker_maps(struct mem_cgroup *memcg); > >>> -void free_shrinker_maps(struct mem_cgroup *memcg); > >>> +int alloc_shrinker_info(struct mem_cgroup *memcg); > >>> +void free_shrinker_info(struct mem_cgroup *memcg); > >>> void set_shrinker_bit(struct mem_cgroup *memcg, int nid, int shrinker_id); > >>> #else > >>> #define mem_cgroup_sockets_enabled 0 > >>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c > >>> index f5c9a0d2160b..f64ad0d044d9 100644 > >>> --- a/mm/memcontrol.c > >>> +++ b/mm/memcontrol.c > >>> @@ -5246,11 +5246,11 @@ 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 expand_shrinker_maps() > >>> + * A memcg must be visible for expand_shrinker_info() > >>> * by the time the maps are allocated. So, we allocate maps > >>> * here, when for_each_mem_cgroup() can't skip it. > >>> */ > >>> - if (alloc_shrinker_maps(memcg)) { > >>> + if (alloc_shrinker_info(memcg)) { > >>> mem_cgroup_id_remove(memcg); > >>> return -ENOMEM; > >>> } > >>> @@ -5314,7 +5314,7 @@ static void mem_cgroup_css_free(struct cgroup_subsys_state *css) > >>> vmpressure_cleanup(&memcg->vmpressure); > >>> cancel_work_sync(&memcg->high_work); > >>> mem_cgroup_remove_from_trees(memcg); > >>> - free_shrinker_maps(memcg); > >>> + free_shrinker_info(memcg); > >>> memcg_free_kmem(memcg); > >>> mem_cgroup_free(memcg); > >>> } > >>> diff --git a/mm/vmscan.c b/mm/vmscan.c > >>> index 641077b09e5d..9436f9246d32 100644 > >>> --- a/mm/vmscan.c > >>> +++ b/mm/vmscan.c > >>> @@ -190,20 +190,20 @@ static int shrinker_nr_max; > >>> #define NR_MAX_TO_SHR_MAP_SIZE(nr_max) \ > >>> (DIV_ROUND_UP(nr_max, BITS_PER_LONG) * sizeof(unsigned long)) > >>> > >>> -static void free_shrinker_map_rcu(struct rcu_head *head) > >>> +static void free_shrinker_info_rcu(struct rcu_head *head) > >>> { > >>> - kvfree(container_of(head, struct memcg_shrinker_map, rcu)); > >>> + kvfree(container_of(head, struct shrinker_info, rcu)); > >>> } > >>> > >>> -static int expand_one_shrinker_map(struct mem_cgroup *memcg, > >>> +static int expand_one_shrinker_info(struct mem_cgroup *memcg, > >>> int size, int old_size) > >>> { > >>> - struct memcg_shrinker_map *new, *old; > >>> + struct shrinker_info *new, *old; > >>> int nid; > >>> > >>> for_each_node(nid) { > >>> old = rcu_dereference_protected( > >>> - mem_cgroup_nodeinfo(memcg, nid)->shrinker_map, true); > >>> + mem_cgroup_nodeinfo(memcg, nid)->shrinker_info, true); > >>> /* Not yet online memcg */ > >>> if (!old) > >>> return 0; > >>> @@ -216,17 +216,17 @@ static int expand_one_shrinker_map(struct mem_cgroup *memcg, > >>> memset(new->map, (int)0xff, old_size); > >>> memset((void *)new->map + old_size, 0, size - old_size); > >>> > >>> - rcu_assign_pointer(memcg->nodeinfo[nid]->shrinker_map, new); > >>> - call_rcu(&old->rcu, free_shrinker_map_rcu); > >>> + rcu_assign_pointer(memcg->nodeinfo[nid]->shrinker_info, new); > >>> + call_rcu(&old->rcu, free_shrinker_info_rcu); > >> > >> Why not use kvfree_rcu() and get rid of free_shrinker_info_rcu() callback? > > > > Just because this patch is aimed to rename the structure. I think it > > may be more preferred to have the cleanup in a separate patch? > > I'd voted for a separate patch Yes, I do agree. Will add a new patch in v8. >
diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h index 1739f17e0939..4c9253896e25 100644 --- a/include/linux/memcontrol.h +++ b/include/linux/memcontrol.h @@ -96,7 +96,7 @@ struct lruvec_stat { * Bitmap of shrinker::id corresponding to memcg-aware shrinkers, * which have elements charged to this memcg. */ -struct memcg_shrinker_map { +struct shrinker_info { struct rcu_head rcu; unsigned long map[]; }; @@ -118,7 +118,7 @@ struct mem_cgroup_per_node { struct mem_cgroup_reclaim_iter iter; - struct memcg_shrinker_map __rcu *shrinker_map; + struct shrinker_info __rcu *shrinker_info; struct rb_node tree_node; /* RB tree node */ unsigned long usage_in_excess;/* Set to the value by which */ @@ -1581,8 +1581,8 @@ static inline bool mem_cgroup_under_socket_pressure(struct mem_cgroup *memcg) return false; } -int alloc_shrinker_maps(struct mem_cgroup *memcg); -void free_shrinker_maps(struct mem_cgroup *memcg); +int alloc_shrinker_info(struct mem_cgroup *memcg); +void free_shrinker_info(struct mem_cgroup *memcg); void set_shrinker_bit(struct mem_cgroup *memcg, int nid, int shrinker_id); #else #define mem_cgroup_sockets_enabled 0 diff --git a/mm/memcontrol.c b/mm/memcontrol.c index f5c9a0d2160b..f64ad0d044d9 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -5246,11 +5246,11 @@ 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 expand_shrinker_maps() + * A memcg must be visible for expand_shrinker_info() * by the time the maps are allocated. So, we allocate maps * here, when for_each_mem_cgroup() can't skip it. */ - if (alloc_shrinker_maps(memcg)) { + if (alloc_shrinker_info(memcg)) { mem_cgroup_id_remove(memcg); return -ENOMEM; } @@ -5314,7 +5314,7 @@ static void mem_cgroup_css_free(struct cgroup_subsys_state *css) vmpressure_cleanup(&memcg->vmpressure); cancel_work_sync(&memcg->high_work); mem_cgroup_remove_from_trees(memcg); - free_shrinker_maps(memcg); + free_shrinker_info(memcg); memcg_free_kmem(memcg); mem_cgroup_free(memcg); } diff --git a/mm/vmscan.c b/mm/vmscan.c index 641077b09e5d..9436f9246d32 100644 --- a/mm/vmscan.c +++ b/mm/vmscan.c @@ -190,20 +190,20 @@ static int shrinker_nr_max; #define NR_MAX_TO_SHR_MAP_SIZE(nr_max) \ (DIV_ROUND_UP(nr_max, BITS_PER_LONG) * sizeof(unsigned long)) -static void free_shrinker_map_rcu(struct rcu_head *head) +static void free_shrinker_info_rcu(struct rcu_head *head) { - kvfree(container_of(head, struct memcg_shrinker_map, rcu)); + kvfree(container_of(head, struct shrinker_info, rcu)); } -static int expand_one_shrinker_map(struct mem_cgroup *memcg, +static int expand_one_shrinker_info(struct mem_cgroup *memcg, int size, int old_size) { - struct memcg_shrinker_map *new, *old; + struct shrinker_info *new, *old; int nid; for_each_node(nid) { old = rcu_dereference_protected( - mem_cgroup_nodeinfo(memcg, nid)->shrinker_map, true); + mem_cgroup_nodeinfo(memcg, nid)->shrinker_info, true); /* Not yet online memcg */ if (!old) return 0; @@ -216,17 +216,17 @@ static int expand_one_shrinker_map(struct mem_cgroup *memcg, memset(new->map, (int)0xff, old_size); memset((void *)new->map + old_size, 0, size - old_size); - rcu_assign_pointer(memcg->nodeinfo[nid]->shrinker_map, new); - call_rcu(&old->rcu, free_shrinker_map_rcu); + rcu_assign_pointer(memcg->nodeinfo[nid]->shrinker_info, new); + call_rcu(&old->rcu, free_shrinker_info_rcu); } return 0; } -void free_shrinker_maps(struct mem_cgroup *memcg) +void free_shrinker_info(struct mem_cgroup *memcg) { struct mem_cgroup_per_node *pn; - struct memcg_shrinker_map *map; + struct shrinker_info *info; int nid; if (mem_cgroup_is_root(memcg)) @@ -234,15 +234,15 @@ void free_shrinker_maps(struct mem_cgroup *memcg) for_each_node(nid) { pn = mem_cgroup_nodeinfo(memcg, nid); - map = rcu_dereference_protected(pn->shrinker_map, true); - kvfree(map); - rcu_assign_pointer(pn->shrinker_map, NULL); + info = rcu_dereference_protected(pn->shrinker_info, true); + kvfree(info); + rcu_assign_pointer(pn->shrinker_info, NULL); } } -int alloc_shrinker_maps(struct mem_cgroup *memcg) +int alloc_shrinker_info(struct mem_cgroup *memcg) { - struct memcg_shrinker_map *map; + struct shrinker_info *info; int nid, size, ret = 0; if (mem_cgroup_is_root(memcg)) @@ -251,20 +251,20 @@ int alloc_shrinker_maps(struct mem_cgroup *memcg) down_write(&shrinker_rwsem); size = NR_MAX_TO_SHR_MAP_SIZE(shrinker_nr_max); for_each_node(nid) { - map = kvzalloc_node(sizeof(*map) + size, GFP_KERNEL, nid); - if (!map) { - free_shrinker_maps(memcg); + info = kvzalloc_node(sizeof(*info) + size, GFP_KERNEL, nid); + if (!info) { + free_shrinker_info(memcg); ret = -ENOMEM; break; } - rcu_assign_pointer(memcg->nodeinfo[nid]->shrinker_map, map); + rcu_assign_pointer(memcg->nodeinfo[nid]->shrinker_info, info); } up_write(&shrinker_rwsem); return ret; } -static int expand_shrinker_maps(int new_id) +static int expand_shrinker_info(int new_id) { int size, old_size, ret = 0; int new_nr_max = new_id + 1; @@ -282,7 +282,7 @@ static int expand_shrinker_maps(int new_id) do { if (mem_cgroup_is_root(memcg)) continue; - ret = expand_one_shrinker_map(memcg, size, old_size); + ret = expand_one_shrinker_info(memcg, size, old_size); if (ret) { mem_cgroup_iter_break(NULL, memcg); goto out; @@ -298,13 +298,13 @@ static int expand_shrinker_maps(int new_id) void set_shrinker_bit(struct mem_cgroup *memcg, int nid, int shrinker_id) { if (shrinker_id >= 0 && memcg && !mem_cgroup_is_root(memcg)) { - struct memcg_shrinker_map *map; + struct shrinker_info *info; rcu_read_lock(); - map = rcu_dereference(memcg->nodeinfo[nid]->shrinker_map); + info = rcu_dereference(memcg->nodeinfo[nid]->shrinker_info); /* Pairs with smp mb in shrink_slab() */ smp_mb__before_atomic(); - set_bit(shrinker_id, map->map); + set_bit(shrinker_id, info->map); rcu_read_unlock(); } } @@ -335,7 +335,7 @@ static int prealloc_memcg_shrinker(struct shrinker *shrinker) goto unlock; if (id >= shrinker_nr_max) { - if (expand_shrinker_maps(id)) { + if (expand_shrinker_info(id)) { idr_remove(&shrinker_idr, id); goto unlock; } @@ -664,7 +664,7 @@ static unsigned long do_shrink_slab(struct shrink_control *shrinkctl, static unsigned long shrink_slab_memcg(gfp_t gfp_mask, int nid, struct mem_cgroup *memcg, int priority) { - struct memcg_shrinker_map *map; + struct shrinker_info *info; unsigned long ret, freed = 0; int i; @@ -674,12 +674,12 @@ static unsigned long shrink_slab_memcg(gfp_t gfp_mask, int nid, if (!down_read_trylock(&shrinker_rwsem)) return 0; - map = rcu_dereference_protected(memcg->nodeinfo[nid]->shrinker_map, - true); - if (unlikely(!map)) + info = rcu_dereference_protected(memcg->nodeinfo[nid]->shrinker_info, + true); + if (unlikely(!info)) goto unlock; - for_each_set_bit(i, map->map, shrinker_nr_max) { + for_each_set_bit(i, info->map, shrinker_nr_max) { struct shrink_control sc = { .gfp_mask = gfp_mask, .nid = nid, @@ -690,7 +690,7 @@ static unsigned long shrink_slab_memcg(gfp_t gfp_mask, int nid, shrinker = idr_find(&shrinker_idr, i); if (unlikely(!shrinker || shrinker == SHRINKER_REGISTERING)) { if (!shrinker) - clear_bit(i, map->map); + clear_bit(i, info->map); continue; } @@ -701,7 +701,7 @@ static unsigned long shrink_slab_memcg(gfp_t gfp_mask, int nid, ret = do_shrink_slab(&sc, shrinker, priority); if (ret == SHRINK_EMPTY) { - clear_bit(i, map->map); + clear_bit(i, info->map); /* * After the shrinker reported that it had no objects to * free, but before we cleared the corresponding bit in