| Message ID | 20210217001322.2226796-6-shy828301@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Make shrinker's nr_deferred memcg aware | expand |
On Tue, Feb 16, 2021 at 04:13:14PM -0800, Yang Shi wrote: > Using kvfree_rcu() to free the old shrinker_maps instead of call_rcu(). > We don't have to define a dedicated callback for call_rcu() anymore. > > Signed-off-by: Yang Shi <shy828301@gmail.com> Acked-by: Roman Gushchin <guro@fb.com> Thanks! > --- > mm/vmscan.c | 7 +------ > 1 file changed, 1 insertion(+), 6 deletions(-) > > diff --git a/mm/vmscan.c b/mm/vmscan.c > index 2e753c2516fa..c2a309acd86b 100644 > --- a/mm/vmscan.c > +++ b/mm/vmscan.c > @@ -192,11 +192,6 @@ static inline int shrinker_map_size(int nr_items) > return (DIV_ROUND_UP(nr_items, BITS_PER_LONG) * sizeof(unsigned long)); > } > > -static void free_shrinker_map_rcu(struct rcu_head *head) > -{ > - kvfree(container_of(head, struct memcg_shrinker_map, rcu)); > -} > - > static int expand_one_shrinker_map(struct mem_cgroup *memcg, > int size, int old_size) > { > @@ -219,7 +214,7 @@ static int expand_one_shrinker_map(struct mem_cgroup *memcg, > 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); > + kvfree_rcu(old); > } > > return 0; > -- > 2.26.2 >
On 17.02.2021 03:13, Yang Shi wrote: > Using kvfree_rcu() to free the old shrinker_maps instead of call_rcu(). > We don't have to define a dedicated callback for call_rcu() anymore. > > Signed-off-by: Yang Shi <shy828301@gmail.com> Acked-by: Kirill Tkhai <ktkhai@virtuozzo.com> > --- > mm/vmscan.c | 7 +------ > 1 file changed, 1 insertion(+), 6 deletions(-) > > diff --git a/mm/vmscan.c b/mm/vmscan.c > index 2e753c2516fa..c2a309acd86b 100644 > --- a/mm/vmscan.c > +++ b/mm/vmscan.c > @@ -192,11 +192,6 @@ static inline int shrinker_map_size(int nr_items) > return (DIV_ROUND_UP(nr_items, BITS_PER_LONG) * sizeof(unsigned long)); > } > > -static void free_shrinker_map_rcu(struct rcu_head *head) > -{ > - kvfree(container_of(head, struct memcg_shrinker_map, rcu)); > -} > - > static int expand_one_shrinker_map(struct mem_cgroup *memcg, > int size, int old_size) > { > @@ -219,7 +214,7 @@ static int expand_one_shrinker_map(struct mem_cgroup *memcg, > 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); > + kvfree_rcu(old); > } > > return 0; >
On Tue, Feb 16, 2021 at 4:13 PM Yang Shi <shy828301@gmail.com> wrote: > > Using kvfree_rcu() to free the old shrinker_maps instead of call_rcu(). > We don't have to define a dedicated callback for call_rcu() anymore. > > Signed-off-by: Yang Shi <shy828301@gmail.com> > --- > mm/vmscan.c | 7 +------ > 1 file changed, 1 insertion(+), 6 deletions(-) > > diff --git a/mm/vmscan.c b/mm/vmscan.c > index 2e753c2516fa..c2a309acd86b 100644 > --- a/mm/vmscan.c > +++ b/mm/vmscan.c > @@ -192,11 +192,6 @@ static inline int shrinker_map_size(int nr_items) > return (DIV_ROUND_UP(nr_items, BITS_PER_LONG) * sizeof(unsigned long)); > } > > -static void free_shrinker_map_rcu(struct rcu_head *head) > -{ > - kvfree(container_of(head, struct memcg_shrinker_map, rcu)); > -} > - > static int expand_one_shrinker_map(struct mem_cgroup *memcg, > int size, int old_size) > { > @@ -219,7 +214,7 @@ static int expand_one_shrinker_map(struct mem_cgroup *memcg, > 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); > + kvfree_rcu(old); Please use kvfree_rcu(old, rcu) instead of kvfree_rcu(old). The single param can call synchronize_rcu().
On Sun, Mar 07, 2021 at 10:13:04PM -0800, Shakeel Butt wrote: > On Tue, Feb 16, 2021 at 4:13 PM Yang Shi <shy828301@gmail.com> wrote: > > > > Using kvfree_rcu() to free the old shrinker_maps instead of call_rcu(). > > We don't have to define a dedicated callback for call_rcu() anymore. > > > > Signed-off-by: Yang Shi <shy828301@gmail.com> > > --- > > mm/vmscan.c | 7 +------ > > 1 file changed, 1 insertion(+), 6 deletions(-) > > > > diff --git a/mm/vmscan.c b/mm/vmscan.c > > index 2e753c2516fa..c2a309acd86b 100644 > > --- a/mm/vmscan.c > > +++ b/mm/vmscan.c > > @@ -192,11 +192,6 @@ static inline int shrinker_map_size(int nr_items) > > return (DIV_ROUND_UP(nr_items, BITS_PER_LONG) * sizeof(unsigned long)); > > } > > > > -static void free_shrinker_map_rcu(struct rcu_head *head) > > -{ > > - kvfree(container_of(head, struct memcg_shrinker_map, rcu)); > > -} > > - > > static int expand_one_shrinker_map(struct mem_cgroup *memcg, > > int size, int old_size) > > { > > @@ -219,7 +214,7 @@ static int expand_one_shrinker_map(struct mem_cgroup *memcg, > > 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); > > + kvfree_rcu(old); > > Please use kvfree_rcu(old, rcu) instead of kvfree_rcu(old). The single > param can call synchronize_rcu(). Especially given that you already have the ->rcu field that the two-argument form requires. The reason for using the single-argument form is when you have lots of little data structures, such that getting rid of that rcu_head structure is valuable enough to be worth the occasional call to synchronize_rcu(). However, please note that this call to synchronize_rcu() happens only under OOM conditions. Thanx, Paul
On Sun, Mar 07, 2021 at 10:13:04PM -0800, Shakeel Butt wrote: > On Tue, Feb 16, 2021 at 4:13 PM Yang Shi <shy828301@gmail.com> wrote: > > > > Using kvfree_rcu() to free the old shrinker_maps instead of call_rcu(). > > We don't have to define a dedicated callback for call_rcu() anymore. > > > > Signed-off-by: Yang Shi <shy828301@gmail.com> > > --- > > mm/vmscan.c | 7 +------ > > 1 file changed, 1 insertion(+), 6 deletions(-) > > > > diff --git a/mm/vmscan.c b/mm/vmscan.c > > index 2e753c2516fa..c2a309acd86b 100644 > > --- a/mm/vmscan.c > > +++ b/mm/vmscan.c > > @@ -192,11 +192,6 @@ static inline int shrinker_map_size(int nr_items) > > return (DIV_ROUND_UP(nr_items, BITS_PER_LONG) * sizeof(unsigned long)); > > } > > > > -static void free_shrinker_map_rcu(struct rcu_head *head) > > -{ > > - kvfree(container_of(head, struct memcg_shrinker_map, rcu)); > > -} > > - > > static int expand_one_shrinker_map(struct mem_cgroup *memcg, > > int size, int old_size) > > { > > @@ -219,7 +214,7 @@ static int expand_one_shrinker_map(struct mem_cgroup *memcg, > > 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); > > + kvfree_rcu(old); > > Please use kvfree_rcu(old, rcu) instead of kvfree_rcu(old). The single > param can call synchronize_rcu(). Oh, I didn't know about this difference. Thank you for noticing!
On Mon, Mar 8, 2021 at 6:54 AM Paul E. McKenney <paulmck@kernel.org> wrote: > > On Sun, Mar 07, 2021 at 10:13:04PM -0800, Shakeel Butt wrote: > > On Tue, Feb 16, 2021 at 4:13 PM Yang Shi <shy828301@gmail.com> wrote: > > > > > > Using kvfree_rcu() to free the old shrinker_maps instead of call_rcu(). > > > We don't have to define a dedicated callback for call_rcu() anymore. > > > > > > Signed-off-by: Yang Shi <shy828301@gmail.com> > > > --- > > > mm/vmscan.c | 7 +------ > > > 1 file changed, 1 insertion(+), 6 deletions(-) > > > > > > diff --git a/mm/vmscan.c b/mm/vmscan.c > > > index 2e753c2516fa..c2a309acd86b 100644 > > > --- a/mm/vmscan.c > > > +++ b/mm/vmscan.c > > > @@ -192,11 +192,6 @@ static inline int shrinker_map_size(int nr_items) > > > return (DIV_ROUND_UP(nr_items, BITS_PER_LONG) * sizeof(unsigned long)); > > > } > > > > > > -static void free_shrinker_map_rcu(struct rcu_head *head) > > > -{ > > > - kvfree(container_of(head, struct memcg_shrinker_map, rcu)); > > > -} > > > - > > > static int expand_one_shrinker_map(struct mem_cgroup *memcg, > > > int size, int old_size) > > > { > > > @@ -219,7 +214,7 @@ static int expand_one_shrinker_map(struct mem_cgroup *memcg, > > > 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); > > > + kvfree_rcu(old); > > > > Please use kvfree_rcu(old, rcu) instead of kvfree_rcu(old). The single > > param can call synchronize_rcu(). > > Especially given that you already have the ->rcu field that the > two-argument form requires. > > The reason for using the single-argument form is when you have lots of > little data structures, such that getting rid of that rcu_head structure > is valuable enough to be worth the occasional call to synchronize_rcu(). > However, please note that this call to synchronize_rcu() happens only > under OOM conditions. Thanks, Shakeel and Paul. I didn't realize the difference. Will use the two params form in the new version. > > Thanx, Paul
On Mon, Mar 8, 2021 at 8:49 AM Roman Gushchin <guro@fb.com> wrote: > > On Sun, Mar 07, 2021 at 10:13:04PM -0800, Shakeel Butt wrote: > > On Tue, Feb 16, 2021 at 4:13 PM Yang Shi <shy828301@gmail.com> wrote: > > > > > > Using kvfree_rcu() to free the old shrinker_maps instead of call_rcu(). > > > We don't have to define a dedicated callback for call_rcu() anymore. > > > > > > Signed-off-by: Yang Shi <shy828301@gmail.com> > > > --- > > > mm/vmscan.c | 7 +------ > > > 1 file changed, 1 insertion(+), 6 deletions(-) > > > > > > diff --git a/mm/vmscan.c b/mm/vmscan.c > > > index 2e753c2516fa..c2a309acd86b 100644 > > > --- a/mm/vmscan.c > > > +++ b/mm/vmscan.c > > > @@ -192,11 +192,6 @@ static inline int shrinker_map_size(int nr_items) > > > return (DIV_ROUND_UP(nr_items, BITS_PER_LONG) * sizeof(unsigned long)); > > > } > > > > > > -static void free_shrinker_map_rcu(struct rcu_head *head) > > > -{ > > > - kvfree(container_of(head, struct memcg_shrinker_map, rcu)); > > > -} > > > - > > > static int expand_one_shrinker_map(struct mem_cgroup *memcg, > > > int size, int old_size) > > > { > > > @@ -219,7 +214,7 @@ static int expand_one_shrinker_map(struct mem_cgroup *memcg, > > > 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); > > > + kvfree_rcu(old); > > > > Please use kvfree_rcu(old, rcu) instead of kvfree_rcu(old). The single > > param can call synchronize_rcu(). > > Oh, I didn't know about this difference. Thank you for noticing! BTW, I think I could keep you and Kirill's acked-by with this change (using two params form kvfree_rcu) since the change seems trivial.
On Mon, Mar 8, 2021 at 12:22 PM Yang Shi <shy828301@gmail.com> wrote: > > On Mon, Mar 8, 2021 at 8:49 AM Roman Gushchin <guro@fb.com> wrote: > > > > On Sun, Mar 07, 2021 at 10:13:04PM -0800, Shakeel Butt wrote: > > > On Tue, Feb 16, 2021 at 4:13 PM Yang Shi <shy828301@gmail.com> wrote: > > > > > > > > Using kvfree_rcu() to free the old shrinker_maps instead of call_rcu(). > > > > We don't have to define a dedicated callback for call_rcu() anymore. > > > > > > > > Signed-off-by: Yang Shi <shy828301@gmail.com> > > > > --- > > > > mm/vmscan.c | 7 +------ > > > > 1 file changed, 1 insertion(+), 6 deletions(-) > > > > > > > > diff --git a/mm/vmscan.c b/mm/vmscan.c > > > > index 2e753c2516fa..c2a309acd86b 100644 > > > > --- a/mm/vmscan.c > > > > +++ b/mm/vmscan.c > > > > @@ -192,11 +192,6 @@ static inline int shrinker_map_size(int nr_items) > > > > return (DIV_ROUND_UP(nr_items, BITS_PER_LONG) * sizeof(unsigned long)); > > > > } > > > > > > > > -static void free_shrinker_map_rcu(struct rcu_head *head) > > > > -{ > > > > - kvfree(container_of(head, struct memcg_shrinker_map, rcu)); > > > > -} > > > > - > > > > static int expand_one_shrinker_map(struct mem_cgroup *memcg, > > > > int size, int old_size) > > > > { > > > > @@ -219,7 +214,7 @@ static int expand_one_shrinker_map(struct mem_cgroup *memcg, > > > > 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); > > > > + kvfree_rcu(old); > > > > > > Please use kvfree_rcu(old, rcu) instead of kvfree_rcu(old). The single > > > param can call synchronize_rcu(). > > > > Oh, I didn't know about this difference. Thank you for noticing! > > BTW, I think I could keep you and Kirill's acked-by with this change > (using two params form kvfree_rcu) since the change seems trivial. Once you change, you can add: Reviewed-by: Shakeel Butt <shakeelb@google.com>
diff --git a/mm/vmscan.c b/mm/vmscan.c index 2e753c2516fa..c2a309acd86b 100644 --- a/mm/vmscan.c +++ b/mm/vmscan.c @@ -192,11 +192,6 @@ static inline int shrinker_map_size(int nr_items) return (DIV_ROUND_UP(nr_items, BITS_PER_LONG) * sizeof(unsigned long)); } -static void free_shrinker_map_rcu(struct rcu_head *head) -{ - kvfree(container_of(head, struct memcg_shrinker_map, rcu)); -} - static int expand_one_shrinker_map(struct mem_cgroup *memcg, int size, int old_size) { @@ -219,7 +214,7 @@ static int expand_one_shrinker_map(struct mem_cgroup *memcg, 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); + kvfree_rcu(old); } return 0;
Using kvfree_rcu() to free the old shrinker_maps instead of call_rcu(). We don't have to define a dedicated callback for call_rcu() anymore. Signed-off-by: Yang Shi <shy828301@gmail.com> --- mm/vmscan.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)