| Message ID | 20241013201704.49576-7-Julia.Lawall@inria.fr (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 356c81b6c494a359ed6e25087931acc78c518fb9 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | replace call_rcu by kfree_rcu for simple kmem_cache_free callback | expand |
On Sunday, 13 October 2024 22:16:53 CEST Julia Lawall wrote: > Since SLOB was removed and since > commit 6c6c47b063b5 ("mm, slab: call kvfree_rcu_barrier() from kmem_cache_destroy()"), > it is not necessary to use call_rcu when the callback only performs > kmem_cache_free. Use kfree_rcu() directly. > > The changes were made using Coccinelle. > > Signed-off-by: Julia Lawall <Julia.Lawall@inria.fr> > > --- > net/batman-adv/translation-table.c | 47 ++----------------------------------- > 1 file changed, 3 insertions(+), 44 deletions(-) This was tried and we noticed that it is not safe [1]. So, I would get confirmation that commit 6c6c47b063b5 ("mm, slab: call kvfree_rcu_barrier() from kmem_cache_destroy()") is fixing the problem which we had at that time. The commit message sounds like it but I just want to be sure. Kind regards, Sven [1] https://lore.kernel.org/r/20240612133357.2596-1-linus.luessing@c0d3.blue > > diff --git a/net/batman-adv/translation-table.c b/net/batman-adv/translation-table.c > index 2243cec18ecc..b21ff3c36b07 100644 > --- a/net/batman-adv/translation-table.c > +++ b/net/batman-adv/translation-table.c > @@ -208,20 +208,6 @@ batadv_tt_global_hash_find(struct batadv_priv *bat_priv, const u8 *addr, > return tt_global_entry; > } > > -/** > - * batadv_tt_local_entry_free_rcu() - free the tt_local_entry > - * @rcu: rcu pointer of the tt_local_entry > - */ > -static void batadv_tt_local_entry_free_rcu(struct rcu_head *rcu) > -{ > - struct batadv_tt_local_entry *tt_local_entry; > - > - tt_local_entry = container_of(rcu, struct batadv_tt_local_entry, > - common.rcu); > - > - kmem_cache_free(batadv_tl_cache, tt_local_entry); > -} > - > /** > * batadv_tt_local_entry_release() - release tt_local_entry from lists and queue > * for free after rcu grace period > @@ -236,7 +222,7 @@ static void batadv_tt_local_entry_release(struct kref *ref) > > batadv_softif_vlan_put(tt_local_entry->vlan); > > - call_rcu(&tt_local_entry->common.rcu, batadv_tt_local_entry_free_rcu); > + kfree_rcu(tt_local_entry, common.rcu); > } > > /** > @@ -254,20 +240,6 @@ batadv_tt_local_entry_put(struct batadv_tt_local_entry *tt_local_entry) > batadv_tt_local_entry_release); > } > > -/** > - * batadv_tt_global_entry_free_rcu() - free the tt_global_entry > - * @rcu: rcu pointer of the tt_global_entry > - */ > -static void batadv_tt_global_entry_free_rcu(struct rcu_head *rcu) > -{ > - struct batadv_tt_global_entry *tt_global_entry; > - > - tt_global_entry = container_of(rcu, struct batadv_tt_global_entry, > - common.rcu); > - > - kmem_cache_free(batadv_tg_cache, tt_global_entry); > -} > - > /** > * batadv_tt_global_entry_release() - release tt_global_entry from lists and > * queue for free after rcu grace period > @@ -282,7 +254,7 @@ void batadv_tt_global_entry_release(struct kref *ref) > > batadv_tt_global_del_orig_list(tt_global_entry); > > - call_rcu(&tt_global_entry->common.rcu, batadv_tt_global_entry_free_rcu); > + kfree_rcu(tt_global_entry, common.rcu); > } > > /** > @@ -407,19 +379,6 @@ static void batadv_tt_global_size_dec(struct batadv_orig_node *orig_node, > batadv_tt_global_size_mod(orig_node, vid, -1); > } > > -/** > - * batadv_tt_orig_list_entry_free_rcu() - free the orig_entry > - * @rcu: rcu pointer of the orig_entry > - */ > -static void batadv_tt_orig_list_entry_free_rcu(struct rcu_head *rcu) > -{ > - struct batadv_tt_orig_list_entry *orig_entry; > - > - orig_entry = container_of(rcu, struct batadv_tt_orig_list_entry, rcu); > - > - kmem_cache_free(batadv_tt_orig_cache, orig_entry); > -} > - > /** > * batadv_tt_orig_list_entry_release() - release tt orig entry from lists and > * queue for free after rcu grace period > @@ -433,7 +392,7 @@ static void batadv_tt_orig_list_entry_release(struct kref *ref) > refcount); > > batadv_orig_node_put(orig_entry->orig_node); > - call_rcu(&orig_entry->rcu, batadv_tt_orig_list_entry_free_rcu); > + kfree_rcu(orig_entry, rcu); > } > > /** > >
On Mon, 14 Oct 2024, Sven Eckelmann wrote: > On Sunday, 13 October 2024 22:16:53 CEST Julia Lawall wrote: > > Since SLOB was removed and since > > commit 6c6c47b063b5 ("mm, slab: call kvfree_rcu_barrier() from kmem_cache_destroy()"), > > it is not necessary to use call_rcu when the callback only performs > > kmem_cache_free. Use kfree_rcu() directly. > > > > The changes were made using Coccinelle. > > > > Signed-off-by: Julia Lawall <Julia.Lawall@inria.fr> > > > > --- > > net/batman-adv/translation-table.c | 47 ++----------------------------------- > > 1 file changed, 3 insertions(+), 44 deletions(-) > > > This was tried and we noticed that it is not safe [1]. So, I would get > confirmation that commit 6c6c47b063b5 ("mm, slab: call kvfree_rcu_barrier() > from kmem_cache_destroy()") is fixing the problem which we had at that time. > The commit message sounds like it but I just want to be sure. Thanks for the feedback. I think that Vlastimil Babka can help with that. julia > > Kind regards, > Sven > > [1] https://lore.kernel.org/r/20240612133357.2596-1-linus.luessing@c0d3.blue > > > > > diff --git a/net/batman-adv/translation-table.c b/net/batman-adv/translation-table.c > > index 2243cec18ecc..b21ff3c36b07 100644 > > --- a/net/batman-adv/translation-table.c > > +++ b/net/batman-adv/translation-table.c > > @@ -208,20 +208,6 @@ batadv_tt_global_hash_find(struct batadv_priv *bat_priv, const u8 *addr, > > return tt_global_entry; > > } > > > > -/** > > - * batadv_tt_local_entry_free_rcu() - free the tt_local_entry > > - * @rcu: rcu pointer of the tt_local_entry > > - */ > > -static void batadv_tt_local_entry_free_rcu(struct rcu_head *rcu) > > -{ > > - struct batadv_tt_local_entry *tt_local_entry; > > - > > - tt_local_entry = container_of(rcu, struct batadv_tt_local_entry, > > - common.rcu); > > - > > - kmem_cache_free(batadv_tl_cache, tt_local_entry); > > -} > > - > > /** > > * batadv_tt_local_entry_release() - release tt_local_entry from lists and queue > > * for free after rcu grace period > > @@ -236,7 +222,7 @@ static void batadv_tt_local_entry_release(struct kref *ref) > > > > batadv_softif_vlan_put(tt_local_entry->vlan); > > > > - call_rcu(&tt_local_entry->common.rcu, batadv_tt_local_entry_free_rcu); > > + kfree_rcu(tt_local_entry, common.rcu); > > } > > > > /** > > @@ -254,20 +240,6 @@ batadv_tt_local_entry_put(struct batadv_tt_local_entry *tt_local_entry) > > batadv_tt_local_entry_release); > > } > > > > -/** > > - * batadv_tt_global_entry_free_rcu() - free the tt_global_entry > > - * @rcu: rcu pointer of the tt_global_entry > > - */ > > -static void batadv_tt_global_entry_free_rcu(struct rcu_head *rcu) > > -{ > > - struct batadv_tt_global_entry *tt_global_entry; > > - > > - tt_global_entry = container_of(rcu, struct batadv_tt_global_entry, > > - common.rcu); > > - > > - kmem_cache_free(batadv_tg_cache, tt_global_entry); > > -} > > - > > /** > > * batadv_tt_global_entry_release() - release tt_global_entry from lists and > > * queue for free after rcu grace period > > @@ -282,7 +254,7 @@ void batadv_tt_global_entry_release(struct kref *ref) > > > > batadv_tt_global_del_orig_list(tt_global_entry); > > > > - call_rcu(&tt_global_entry->common.rcu, batadv_tt_global_entry_free_rcu); > > + kfree_rcu(tt_global_entry, common.rcu); > > } > > > > /** > > @@ -407,19 +379,6 @@ static void batadv_tt_global_size_dec(struct batadv_orig_node *orig_node, > > batadv_tt_global_size_mod(orig_node, vid, -1); > > } > > > > -/** > > - * batadv_tt_orig_list_entry_free_rcu() - free the orig_entry > > - * @rcu: rcu pointer of the orig_entry > > - */ > > -static void batadv_tt_orig_list_entry_free_rcu(struct rcu_head *rcu) > > -{ > > - struct batadv_tt_orig_list_entry *orig_entry; > > - > > - orig_entry = container_of(rcu, struct batadv_tt_orig_list_entry, rcu); > > - > > - kmem_cache_free(batadv_tt_orig_cache, orig_entry); > > -} > > - > > /** > > * batadv_tt_orig_list_entry_release() - release tt orig entry from lists and > > * queue for free after rcu grace period > > @@ -433,7 +392,7 @@ static void batadv_tt_orig_list_entry_release(struct kref *ref) > > refcount); > > > > batadv_orig_node_put(orig_entry->orig_node); > > - call_rcu(&orig_entry->rcu, batadv_tt_orig_list_entry_free_rcu); > > + kfree_rcu(orig_entry, rcu); > > } > > > > /** > > > > > >
On 10/14/24 09:08, Julia Lawall wrote: > > > On Mon, 14 Oct 2024, Sven Eckelmann wrote: > >> On Sunday, 13 October 2024 22:16:53 CEST Julia Lawall wrote: >> > Since SLOB was removed and since >> > commit 6c6c47b063b5 ("mm, slab: call kvfree_rcu_barrier() from kmem_cache_destroy()"), >> > it is not necessary to use call_rcu when the callback only performs >> > kmem_cache_free. Use kfree_rcu() directly. >> > >> > The changes were made using Coccinelle. >> > >> > Signed-off-by: Julia Lawall <Julia.Lawall@inria.fr> >> > >> > --- >> > net/batman-adv/translation-table.c | 47 ++----------------------------------- >> > 1 file changed, 3 insertions(+), 44 deletions(-) >> >> >> This was tried and we noticed that it is not safe [1]. So, I would get >> confirmation that commit 6c6c47b063b5 ("mm, slab: call kvfree_rcu_barrier() >> from kmem_cache_destroy()") is fixing the problem which we had at that time. >> The commit message sounds like it but I just want to be sure. > > Thanks for the feedback. I think that Vlastimil Babka can help with that. Hi, yeah the batman-adv issue was how we learned about the problem and the series of commits leading to and including 6c6c47b063b5 was done exactly to address the kmem_cache_destroy() on module unload issue, and unblock the conversion to kfree_rcu(). Thanks, Vlastimil > julia
On Sun, 13 Oct 2024 22:16:53 +0200, Julia Lawall wrote: > Since SLOB was removed and since > commit 6c6c47b063b5 ("mm, slab: call kvfree_rcu_barrier() from kmem_cache_destroy()"), > it is not necessary to use call_rcu when the callback only performs > kmem_cache_free. Use kfree_rcu() directly. > > The changes were made using Coccinelle. > > [...] Applied, thanks! [06/17] batman-adv: replace call_rcu by kfree_rcu for simple kmem_cache_free callback commit: 356c81b6c494a359ed6e25087931acc78c518fb9 Best regards,
diff --git a/net/batman-adv/translation-table.c b/net/batman-adv/translation-table.c index 2243cec18ecc..b21ff3c36b07 100644 --- a/net/batman-adv/translation-table.c +++ b/net/batman-adv/translation-table.c @@ -208,20 +208,6 @@ batadv_tt_global_hash_find(struct batadv_priv *bat_priv, const u8 *addr, return tt_global_entry; } -/** - * batadv_tt_local_entry_free_rcu() - free the tt_local_entry - * @rcu: rcu pointer of the tt_local_entry - */ -static void batadv_tt_local_entry_free_rcu(struct rcu_head *rcu) -{ - struct batadv_tt_local_entry *tt_local_entry; - - tt_local_entry = container_of(rcu, struct batadv_tt_local_entry, - common.rcu); - - kmem_cache_free(batadv_tl_cache, tt_local_entry); -} - /** * batadv_tt_local_entry_release() - release tt_local_entry from lists and queue * for free after rcu grace period @@ -236,7 +222,7 @@ static void batadv_tt_local_entry_release(struct kref *ref) batadv_softif_vlan_put(tt_local_entry->vlan); - call_rcu(&tt_local_entry->common.rcu, batadv_tt_local_entry_free_rcu); + kfree_rcu(tt_local_entry, common.rcu); } /** @@ -254,20 +240,6 @@ batadv_tt_local_entry_put(struct batadv_tt_local_entry *tt_local_entry) batadv_tt_local_entry_release); } -/** - * batadv_tt_global_entry_free_rcu() - free the tt_global_entry - * @rcu: rcu pointer of the tt_global_entry - */ -static void batadv_tt_global_entry_free_rcu(struct rcu_head *rcu) -{ - struct batadv_tt_global_entry *tt_global_entry; - - tt_global_entry = container_of(rcu, struct batadv_tt_global_entry, - common.rcu); - - kmem_cache_free(batadv_tg_cache, tt_global_entry); -} - /** * batadv_tt_global_entry_release() - release tt_global_entry from lists and * queue for free after rcu grace period @@ -282,7 +254,7 @@ void batadv_tt_global_entry_release(struct kref *ref) batadv_tt_global_del_orig_list(tt_global_entry); - call_rcu(&tt_global_entry->common.rcu, batadv_tt_global_entry_free_rcu); + kfree_rcu(tt_global_entry, common.rcu); } /** @@ -407,19 +379,6 @@ static void batadv_tt_global_size_dec(struct batadv_orig_node *orig_node, batadv_tt_global_size_mod(orig_node, vid, -1); } -/** - * batadv_tt_orig_list_entry_free_rcu() - free the orig_entry - * @rcu: rcu pointer of the orig_entry - */ -static void batadv_tt_orig_list_entry_free_rcu(struct rcu_head *rcu) -{ - struct batadv_tt_orig_list_entry *orig_entry; - - orig_entry = container_of(rcu, struct batadv_tt_orig_list_entry, rcu); - - kmem_cache_free(batadv_tt_orig_cache, orig_entry); -} - /** * batadv_tt_orig_list_entry_release() - release tt orig entry from lists and * queue for free after rcu grace period @@ -433,7 +392,7 @@ static void batadv_tt_orig_list_entry_release(struct kref *ref) refcount); batadv_orig_node_put(orig_entry->orig_node); - call_rcu(&orig_entry->rcu, batadv_tt_orig_list_entry_free_rcu); + kfree_rcu(orig_entry, rcu); } /**
Since SLOB was removed and since commit 6c6c47b063b5 ("mm, slab: call kvfree_rcu_barrier() from kmem_cache_destroy()"), it is not necessary to use call_rcu when the callback only performs kmem_cache_free. Use kfree_rcu() directly. The changes were made using Coccinelle. Signed-off-by: Julia Lawall <Julia.Lawall@inria.fr> --- net/batman-adv/translation-table.c | 47 ++----------------------------------- 1 file changed, 3 insertions(+), 44 deletions(-)