Messages in this thread |  | | Date | Wed, 21 May 2025 09:54:03 -0700 | | From | Stanislav Fomichev <> | | Subject | Re: [PATCH net-next 2/3] udp_tunnel: remove rtnl_lock dependency |
| |
On 05/21, Jakub Kicinski wrote: > On Tue, 20 May 2025 13:36:13 -0700 Stanislav Fomichev wrote: > > Drivers that are using ops lock and don't depend on RTNL lock > > still need to manage it because udp_tunnel's RTNL dependency. > > Introduce new udp_tunnel_nic_lock and use it instead of > > rtnl_lock. Drop non-UDP_TUNNEL_NIC_INFO_MAY_SLEEP mode from > > udp_tunnel infra (udp_tunnel_nic_device_sync_work needs to > > grab udp_tunnel_nic_lock mutex and might sleep). > > There is a netdevsim-based test for this that needs to be fixed up.
Oh, I did not see that one, let me try to find and run it.
> > diff --git a/include/net/udp_tunnel.h b/include/net/udp_tunnel.h > > index 2df3b8344eb5..7f5537fdf2c9 100644 > > --- a/include/net/udp_tunnel.h > > +++ b/include/net/udp_tunnel.h > > @@ -221,19 +221,17 @@ static inline void udp_tunnel_encap_enable(struct sock *sk) > > #define UDP_TUNNEL_NIC_MAX_TABLES 4 > > > > enum udp_tunnel_nic_info_flags { > > - /* Device callbacks may sleep */ > > - UDP_TUNNEL_NIC_INFO_MAY_SLEEP = BIT(0), > > Could we use a different lock for sleeping and non-sleeping drivers?
We can probably do it if we reorder the locks (as you ask/suggest below). Overall, I'm not sure I understand why we want to have two paths here. If we can do everything via work queue, why have a separate path for the non-sleepable callback? (more code -> more bugs)
> > @@ -554,11 +543,11 @@ static void __udp_tunnel_nic_reset_ntf(struct net_device *dev) > > struct udp_tunnel_nic *utn; > > unsigned int i, j; > > > > - ASSERT_RTNL(); > > + mutex_lock(&udp_tunnel_nic_lock); > > > > utn = dev->udp_tunnel_nic; > > utn and info's lifetimes are tied to the lifetime of the device > I think their existence can remain protected by the external locks
SG, will move the lock down a bit.
> > if (!utn) > > - return; > > + goto unlock; > > > > utn->need_sync = false; > > for (i = 0; i < utn->n_tables; i++) > > > - rtnl_lock(); > > + mutex_lock(&udp_tunnel_nic_lock); > > utn->work_pending = 0; > > __udp_tunnel_nic_device_sync(utn->dev, utn); > > > > - if (utn->need_replay) > > + if (utn->need_replay) { > > + rtnl_lock(); > > udp_tunnel_nic_replay(utn->dev, utn); > > - rtnl_unlock(); > > + rtnl_unlock(); > > + } > > + mutex_unlock(&udp_tunnel_nic_lock); > > } > > What's the lock ordering between the new lock and rtnl lock?
From ops-locked, we'll get: ops->tunnel_lock (__udp_tunnel_nic_reset_ntf) From non-ops locked, we'll get: rtnl->tunnel_lock
I see your point, we need to do rtnl->tunnel_lock here as well.
> BTW the lock could live in utn, right? We can't use the instance > lock because of sharing, but we could put the lock in utn?
I was thinking that there is some global state besides udp_tunnel_nic, but I don't see any, will move the lock, thanks!
|  |