Re: [v8 PATCH 05/13] mm: vmscan: use kvfree_rcu instead of call_rcu
On Mon, Mar 8, 2021 at 12:22 PM Yang Shi wrote: > > On Mon, Mar 8, 2021 at 8:49 AM Roman Gushchin 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 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 > > > > --- > > > > 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
Re: [v8 PATCH 05/13] mm: vmscan: use kvfree_rcu instead of call_rcu
On Mon, Mar 8, 2021 at 8:49 AM Roman Gushchin 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 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 > > > --- > > > 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.
Re: [v8 PATCH 05/13] mm: vmscan: use kvfree_rcu instead of call_rcu
On Mon, Mar 8, 2021 at 6:54 AM Paul E. McKenney 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 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 > > > --- > > > 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
Re: [v8 PATCH 05/13] mm: vmscan: use kvfree_rcu instead of call_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 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 > > --- > > 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!
Re: [v8 PATCH 05/13] mm: vmscan: use kvfree_rcu instead of call_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 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 > > --- > > 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
Re: [v8 PATCH 05/13] mm: vmscan: use kvfree_rcu instead of call_rcu
On Tue, Feb 16, 2021 at 4:13 PM 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 > --- > 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().
Re: [v8 PATCH 05/13] mm: vmscan: use kvfree_rcu instead of call_rcu
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 Acked-by: Kirill Tkhai > --- > 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; >
Re: [v8 PATCH 05/13] mm: vmscan: use kvfree_rcu instead of call_rcu
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 Acked-by: Roman Gushchin 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 >