| Message ID | 20240609082726.32742-2-Julia.Lawall@inria.fr (mailing list archive) |
|---|---|
| State | Deferred |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | replace call_rcu by kfree_rcu for simple kmem_cache_free callback | expand |
Hi Julia & Vlastimil, On Sun, Jun 09, 2024 at 10:27:13AM +0200, Julia Lawall wrote: > Since SLOB was removed, it is not necessary to use call_rcu > when the callback only performs kmem_cache_free. Use > kfree_rcu() directly. Thanks, I applied this to the wireguard tree, and I'll send this out as a fix for 6.10. Let me know if this is unfavorable to you and if you'd like to take this somewhere yourself, in which case I'll give you my ack. Just a question, though, for Vlastimil -- I know that with the SLOB removal, kfree() is now allowed on kmemcache'd objects. Do you plan to do a blanket s/kmem_cache_free/kfree/g at some point, and then remove kmem_cache_free all together? Jason
On Sun, 9 Jun 2024, Jason A. Donenfeld wrote: > Hi Julia & Vlastimil, > > On Sun, Jun 09, 2024 at 10:27:13AM +0200, Julia Lawall wrote: > > Since SLOB was removed, it is not necessary to use call_rcu > > when the callback only performs kmem_cache_free. Use > > kfree_rcu() directly. > > Thanks, I applied this to the wireguard tree, and I'll send this out as > a fix for 6.10. Let me know if this is unfavorable to you and if you'd > like to take this somewhere yourself, in which case I'll give you my > ack. Please push it onward. julia > > Just a question, though, for Vlastimil -- I know that with the SLOB > removal, kfree() is now allowed on kmemcache'd objects. Do you plan to > do a blanket s/kmem_cache_free/kfree/g at some point, and then remove > kmem_cache_free all together? > > Jason >
On 6/9/24 4:32 PM, Jason A. Donenfeld wrote: > Hi Julia & Vlastimil, > > On Sun, Jun 09, 2024 at 10:27:13AM +0200, Julia Lawall wrote: >> Since SLOB was removed, it is not necessary to use call_rcu >> when the callback only performs kmem_cache_free. Use >> kfree_rcu() directly. > > Thanks, I applied this to the wireguard tree, and I'll send this out as > a fix for 6.10. Let me know if this is unfavorable to you and if you'd > like to take this somewhere yourself, in which case I'll give you my > ack. > > Just a question, though, for Vlastimil -- I know that with the SLOB > removal, kfree() is now allowed on kmemcache'd objects. Do you plan to > do a blanket s/kmem_cache_free/kfree/g at some point, and then remove > kmem_cache_free all together? Hmm, not really, but obligatory Cc for willy who'd love to have "one free() to rule them all" IIRC. My current thinking is that kmem_cache_free() can save the kmem_cache lookup, or serve as a double check if debugging is enabled, and doesn't have much downside. If someone wants to not care about the kmem_cache pointer, they can use kfree(). Even convert their subsystem at will. But a mass conversion of everything would be rather lot of churn for not much of a benefit, IMHO.
Hi Vlastimil, On Mon, Jun 10, 2024 at 10:38:08PM +0200, Vlastimil Babka wrote: > On 6/9/24 4:32 PM, Jason A. Donenfeld wrote: > > Hi Julia & Vlastimil, > > > > On Sun, Jun 09, 2024 at 10:27:13AM +0200, Julia Lawall wrote: > >> Since SLOB was removed, it is not necessary to use call_rcu > >> when the callback only performs kmem_cache_free. Use > >> kfree_rcu() directly. > > > > Thanks, I applied this to the wireguard tree, and I'll send this out as > > a fix for 6.10. Let me know if this is unfavorable to you and if you'd > > like to take this somewhere yourself, in which case I'll give you my > > ack. > > > > Just a question, though, for Vlastimil -- I know that with the SLOB > > removal, kfree() is now allowed on kmemcache'd objects. Do you plan to > > do a blanket s/kmem_cache_free/kfree/g at some point, and then remove > > kmem_cache_free all together? > > Hmm, not really, but obligatory Cc for willy who'd love to have "one free() > to rule them all" IIRC. > > My current thinking is that kmem_cache_free() can save the kmem_cache > lookup, or serve as a double check if debugging is enabled, and doesn't have > much downside. If someone wants to not care about the kmem_cache pointer, > they can use kfree(). Even convert their subsystem at will. But a mass > conversion of everything would be rather lot of churn for not much of a > benefit, IMHO. Huh, interesting. I can see the practical sense in that, not causing unnecessary churn and such. At the same time, this doesn't appeal much to some sort of orderly part of my mind. Either all kmalloc/kmem_cache memory is kfree()d as the rule for what is best, or a kmalloc pairs with a kfree and a kmem_cache_alloc pairs with a kmem_cache_free and that's the rule. And those can be checked and enforced and so forth. But saying, "oh, well, they might work a bit different, but whatever you want is basically fine; there's no rhyme or reason" is somehow dissatisfying. Maybe the rule is actually, "use kmem_cache_free if you can because it saves a pointer lookup, but don't go out of your way to do that and certainly don't bloat .text to make it happen," then maybe that makes sense? But I dunno, I find myself wanting a rule and consistency. (Did you find it annoying that in this paragraph, I used () on only one function mention but not on the others? If so, maybe you're like me.) Maybe I should just chill though. Anyway, only my 2ยข, and my opinion here isn't worth much, so please regard this as only a gut statement from a bystander. Jason
diff --git a/drivers/net/wireguard/allowedips.c b/drivers/net/wireguard/allowedips.c index 0ba714ca5185..e4e1638fce1b 100644 --- a/drivers/net/wireguard/allowedips.c +++ b/drivers/net/wireguard/allowedips.c @@ -48,11 +48,6 @@ static void push_rcu(struct allowedips_node **stack, } } -static void node_free_rcu(struct rcu_head *rcu) -{ - kmem_cache_free(node_cache, container_of(rcu, struct allowedips_node, rcu)); -} - static void root_free_rcu(struct rcu_head *rcu) { struct allowedips_node *node, *stack[MAX_ALLOWEDIPS_DEPTH] = { @@ -330,13 +325,13 @@ void wg_allowedips_remove_by_peer(struct allowedips *table, child = rcu_dereference_protected( parent->bit[!(node->parent_bit_packed & 1)], lockdep_is_held(lock)); - call_rcu(&node->rcu, node_free_rcu); + kfree_rcu(node, rcu); if (!free_parent) continue; if (child) child->parent_bit_packed = parent->parent_bit_packed; *(struct allowedips_node **)(parent->parent_bit_packed & ~3UL) = child; - call_rcu(&parent->rcu, node_free_rcu); + kfree_rcu(parent, rcu); } }
Since SLOB was removed, it is not necessary to use call_rcu when the callback only performs kmem_cache_free. Use kfree_rcu() directly. The changes were done using the following Coccinelle semantic patch. This semantic patch is designed to ignore cases where the callback function is used in another way. // <smpl> @r@ expression e; local idexpression e2; identifier cb,f; position p; @@ ( call_rcu(...,e2) | call_rcu(&e->f,cb@p) ) @r1@ type T; identifier x,r.cb; @@ cb(...) { ( kmem_cache_free(...); | T x = ...; kmem_cache_free(...,x); | T x; x = ...; kmem_cache_free(...,x); ) } @s depends on r1@ position p != r.p; identifier r.cb; @@ cb@p @script:ocaml@ cb << r.cb; p << s.p; @@ Printf.eprintf "Other use of %s at %s:%d\n" cb (List.hd p).file (List.hd p).line @depends on r1 && !s@ expression e; identifier r.cb,f; position r.p; @@ - call_rcu(&e->f,cb@p) + kfree_rcu(e,f) @r1a depends on !s@ type T; identifier x,r.cb; @@ - cb(...) { ( - kmem_cache_free(...); | - T x = ...; - kmem_cache_free(...,x); | - T x; - x = ...; - kmem_cache_free(...,x); ) - } // </smpl> Signed-off-by: Julia Lawall <Julia.Lawall@inria.fr> Reviewed-by: Paul E. McKenney <paulmck@kernel.org> Reviewed-by: Vlastimil Babka <vbabka@suse.cz> --- drivers/net/wireguard/allowedips.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-)