[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <24f640eb-6f28-4704-855b-afb1dfe06613@davidwei.uk>
Date: Wed, 3 Jan 2024 13:57:40 -0800
From: David Wei <dw@...idwei.uk>
To: Eric Dumazet <edumazet@...gle.com>
Cc: Jakub Kicinski <kuba@...nel.org>, Jiri Pirko <jiri@...nulli.us>,
Sabrina Dubroca <sd@...asysnail.net>, netdev@...r.kernel.org,
"David S. Miller" <davem@...emloft.net>, Paolo Abeni <pabeni@...hat.com>
Subject: Re: [PATCH net-next v5 3/5] netdevsim: forward skbs from one
connected port to another
On 2024-01-02 03:20, Eric Dumazet wrote:
> On Thu, Dec 28, 2023 at 2:46 AM David Wei <dw@...idwei.uk> wrote:
>>
>> Forward skbs sent from one netdevsim port to its connected netdevsim
>> port using dev_forward_skb, in a spirit similar to veth.
>>
>> Add a tx_dropped variable to struct netdevsim, tracking the number of
>> skbs that could not be forwarded using dev_forward_skb().
>>
>> The xmit() function accessing the peer ptr is protected by an RCU read
>> critical section. The rcu_read_lock() is functionally redundant as since
>> v5.0 all softirqs are implicitly RCU read critical sections; but it is
>> useful for human readers.
>>
>> If another CPU is concurrently in nsim_destroy(), then it will first set
>> the peer ptr to NULL. This does not affect any existing readers that
>> dereferenced a non-NULL peer. Then, in unregister_netdevice(), there is
>> a synchronize_rcu() before the netdev is actually unregistered and
>> freed. This ensures that any readers i.e. xmit() that got a non-NULL
>> peer will complete before the netdev is freed.
>>
>> Any readers after the RCU_INIT_POINTER() but before synchronize_rcu()
>> will dereference NULL, making it safe.
>>
>> The codepath to nsim_destroy() and nsim_create() takes both the newly
>> added nsim_dev_list_lock and rtnl_lock. This makes it safe with
>> concurrent calls to linking two netdevsims together.
>>
>> Signed-off-by: David Wei <dw@...idwei.uk>
>> ---
>> drivers/net/netdevsim/netdev.c | 21 ++++++++++++++++++---
>> drivers/net/netdevsim/netdevsim.h | 1 +
>> 2 files changed, 19 insertions(+), 3 deletions(-)
>>
>
>
>> @@ -302,7 +318,6 @@ static void nsim_setup(struct net_device *dev)
>> eth_hw_addr_random(dev);
>>
>> dev->tx_queue_len = 0;
>> - dev->flags |= IFF_NOARP;
>
> This part seems to be unrelated to this patch ?
Hi Eric, I found that this change is needed for skb forwarding to work.
Would you prefer me splitting this change into its own patch?
>
>> dev->flags &= ~IFF_MULTICAST;
>> dev->priv_flags |= IFF_LIVE_ADDR_CHANGE |
>> IFF_NO_QUEUE;
Powered by blists - more mailing lists