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!
