Re: [Intel-wired-lan] [PATCH net-next 2/3] udp_tunnel: remove rtnl_lock dependency

[Stanislav Fomichev <stfomichev@gmail.com>]

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!