[PATCH net-next] can: gw: use call_rcu() instead of costly synchronize_rcu()

[PATCH net-next] can: gw: use call_rcu() instead of costly synchronize_rcu()

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

Commit fb8696ab14ad ("can: gw: synchronize rcu operations
before removing gw job entry") added three synchronize_rcu() calls
to make sure one rcu grace period was observed before freeing
a "struct cgw_job" (which are tiny objects).

This should be converted to call_rcu() to avoid adding delays
in device / network dismantles.

Use the rcu_head that was already in struct cgw_job,
not yet used.

Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Oliver Hartkopp <socketcan@hartkopp.net>
---
 net/can/gw.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/net/can/gw.c b/net/can/gw.c
index d8861e862f157aec36c417b71eb7e8f59bd064b9..20e74fe7d0906dccc65732b8f9e7e14e2d1192c3 100644
--- a/net/can/gw.c
+++ b/net/can/gw.c
@@ -577,6 +577,13 @@ static inline void cgw_unregister_filter(struct net *net, struct cgw_job *gwj)
 			  gwj->ccgw.filter.can_mask, can_can_gw_rcv, gwj);
 }
 
+static void cgw_job_free_rcu(struct rcu_head *rcu_head)
+{
+	struct cgw_job *gwj = container_of(rcu_head, struct cgw_job, rcu);
+
+	kmem_cache_free(cgw_cache, gwj);
+}
+
 static int cgw_notifier(struct notifier_block *nb,
 			unsigned long msg, void *ptr)
 {
@@ -596,8 +603,7 @@ static int cgw_notifier(struct notifier_block *nb,
 			if (gwj->src.dev == dev || gwj->dst.dev == dev) {
 				hlist_del(&gwj->list);
 				cgw_unregister_filter(net, gwj);
-				synchronize_rcu();
-				kmem_cache_free(cgw_cache, gwj);
+				call_rcu(&gwj->rcu, cgw_job_free_rcu);
 			}
 		}
 	}
@@ -1155,8 +1161,7 @@ static void cgw_remove_all_jobs(struct net *net)
 	hlist_for_each_entry_safe(gwj, nx, &net->can.cgw_list, list) {
 		hlist_del(&gwj->list);
 		cgw_unregister_filter(net, gwj);
-		synchronize_rcu();
-		kmem_cache_free(cgw_cache, gwj);
+		call_rcu(&gwj->rcu, cgw_job_free_rcu);
 	}
 }
 
@@ -1224,8 +1229,7 @@ static int cgw_remove_job(struct sk_buff *skb, struct nlmsghdr *nlh,
 
 		hlist_del(&gwj->list);
 		cgw_unregister_filter(net, gwj);
-		synchronize_rcu();
-		kmem_cache_free(cgw_cache, gwj);
+		call_rcu(&gwj->rcu, cgw_job_free_rcu);
 		err = 0;
 		break;
 	}
-- 
2.35.0.263.gb82422642f-goog

Re: [PATCH net-next] can: gw: use call_rcu() instead of costly synchronize_rcu()

[Oliver Hartkopp]

On 07.02.22 20:07, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@google.com>
> 
> Commit fb8696ab14ad ("can: gw: synchronize rcu operations
> before removing gw job entry") added three synchronize_rcu() calls
> to make sure one rcu grace period was observed before freeing
> a "struct cgw_job" (which are tiny objects).
> 
> This should be converted to call_rcu() to avoid adding delays
> in device / network dismantles.
> 
> Use the rcu_head that was already in struct cgw_job,
> not yet used.
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Oliver Hartkopp <socketcan@hartkopp.net>

Tested-by: Oliver Hartkopp <socketcan@hartkopp.net>

Thanks Eric!

> ---
>   net/can/gw.c | 16 ++++++++++------
>   1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/net/can/gw.c b/net/can/gw.c
> index d8861e862f157aec36c417b71eb7e8f59bd064b9..20e74fe7d0906dccc65732b8f9e7e14e2d1192c3 100644
> --- a/net/can/gw.c
> +++ b/net/can/gw.c
> @@ -577,6 +577,13 @@ static inline void cgw_unregister_filter(struct net *net, struct cgw_job *gwj)
>   			  gwj->ccgw.filter.can_mask, can_can_gw_rcv, gwj);
>   }
>   
> +static void cgw_job_free_rcu(struct rcu_head *rcu_head)
> +{
> +	struct cgw_job *gwj = container_of(rcu_head, struct cgw_job, rcu);
> +
> +	kmem_cache_free(cgw_cache, gwj);
> +}
> +
>   static int cgw_notifier(struct notifier_block *nb,
>   			unsigned long msg, void *ptr)
>   {
> @@ -596,8 +603,7 @@ static int cgw_notifier(struct notifier_block *nb,
>   			if (gwj->src.dev == dev || gwj->dst.dev == dev) {
>   				hlist_del(&gwj->list);
>   				cgw_unregister_filter(net, gwj);
> -				synchronize_rcu();
> -				kmem_cache_free(cgw_cache, gwj);
> +				call_rcu(&gwj->rcu, cgw_job_free_rcu);
>   			}
>   		}
>   	}
> @@ -1155,8 +1161,7 @@ static void cgw_remove_all_jobs(struct net *net)
>   	hlist_for_each_entry_safe(gwj, nx, &net->can.cgw_list, list) {
>   		hlist_del(&gwj->list);
>   		cgw_unregister_filter(net, gwj);
> -		synchronize_rcu();
> -		kmem_cache_free(cgw_cache, gwj);
> +		call_rcu(&gwj->rcu, cgw_job_free_rcu);
>   	}
>   }
>   
> @@ -1224,8 +1229,7 @@ static int cgw_remove_job(struct sk_buff *skb, struct nlmsghdr *nlh,
>   
>   		hlist_del(&gwj->list);
>   		cgw_unregister_filter(net, gwj);
> -		synchronize_rcu();
> -		kmem_cache_free(cgw_cache, gwj);
> +		call_rcu(&gwj->rcu, cgw_job_free_rcu);
>   		err = 0;
>   		break;
>   	}

Re: [PATCH net-next] can: gw: use call_rcu() instead of costly synchronize_rcu()

[Jakub Kicinski]

On Mon,  7 Feb 2022 11:07:06 -0800 Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@google.com>
> 
> Commit fb8696ab14ad ("can: gw: synchronize rcu operations
> before removing gw job entry") added three synchronize_rcu() calls
> to make sure one rcu grace period was observed before freeing
> a "struct cgw_job" (which are tiny objects).
> 
> This should be converted to call_rcu() to avoid adding delays
> in device / network dismantles.
> 
> Use the rcu_head that was already in struct cgw_job,
> not yet used.
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Oliver Hartkopp <socketcan@hartkopp.net>

Adding Marc and linux-can to CC.

Re: [PATCH net-next] can: gw: use call_rcu() instead of costly synchronize_rcu()

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1199 bytes --]

On 08.02.2022 20:27:31, Oliver Hartkopp wrote:
> 
> 
> On 07.02.22 20:07, Eric Dumazet wrote:
> > From: Eric Dumazet <edumazet@google.com>
> > 
> > Commit fb8696ab14ad ("can: gw: synchronize rcu operations
> > before removing gw job entry") added three synchronize_rcu() calls
> > to make sure one rcu grace period was observed before freeing
> > a "struct cgw_job" (which are tiny objects).
> > 
> > This should be converted to call_rcu() to avoid adding delays
> > in device / network dismantles.
> > 
> > Use the rcu_head that was already in struct cgw_job,
> > not yet used.
> > 
> > Signed-off-by: Eric Dumazet <edumazet@google.com>
> > Cc: Oliver Hartkopp <socketcan@hartkopp.net>
> 
> Tested-by: Oliver Hartkopp <socketcan@hartkopp.net>

Applied to linux-can-next/testing.

There are several more synchronize_rcu() in net/can. Oliver, can you
create similar patches for those?

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH net-next] can: gw: use call_rcu() instead of costly synchronize_rcu()

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 947 bytes --]

On 07.02.2022 11:07:06, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@google.com>
> 
> Commit fb8696ab14ad ("can: gw: synchronize rcu operations
> before removing gw job entry") added three synchronize_rcu() calls
> to make sure one rcu grace period was observed before freeing
> a "struct cgw_job" (which are tiny objects).
> 
> This should be converted to call_rcu() to avoid adding delays
> in device / network dismantles.
> 
> Use the rcu_head that was already in struct cgw_job,
> not yet used.
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Oliver Hartkopp <socketcan@hartkopp.net>

Added to linux-can-next/testing.

Thanks,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]