As we move away from rtnl_lock for queue ops, introduce
per-netdev_nl_sock lock.

Cc: Mina Almasry <almasrymina@google.com>
Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
---
 include/net/netdev_netlink.h | 1 +
 net/core/netdev-genl.c       | 5 +++++
 2 files changed, 6 insertions(+)

diff --git a/include/net/netdev_netlink.h b/include/net/netdev_netlink.h
index 1599573d35c9..075962dbe743 100644
--- a/include/net/netdev_netlink.h
+++ b/include/net/netdev_netlink.h
@@ -5,6 +5,7 @@
 #include <linux/list.h>
 
 struct netdev_nl_sock {
+	struct mutex lock;
 	struct list_head bindings;
 };
 
diff --git a/net/core/netdev-genl.c b/net/core/netdev-genl.c
index a219be90c739..8acdeeae24e7 100644
--- a/net/core/netdev-genl.c
+++ b/net/core/netdev-genl.c
@@ -859,6 +859,7 @@ int netdev_nl_bind_rx_doit(struct sk_buff *skb, struct genl_info *info)
 		goto err_genlmsg_free;
 	}
 
+	mutex_lock(&priv->lock);
 	rtnl_lock();
 
 	netdev = __dev_get_by_index(genl_info_net(info), ifindex);
@@ -925,6 +926,7 @@ int netdev_nl_bind_rx_doit(struct sk_buff *skb, struct genl_info *info)
 	net_devmem_unbind_dmabuf(binding);
 err_unlock:
 	rtnl_unlock();
+	mutex_unlock(&priv->lock);
 err_genlmsg_free:
 	nlmsg_free(rsp);
 	return err;
@@ -933,6 +935,7 @@ int netdev_nl_bind_rx_doit(struct sk_buff *skb, struct genl_info *info)
 void netdev_nl_sock_priv_init(struct netdev_nl_sock *priv)
 {
 	INIT_LIST_HEAD(&priv->bindings);
+	mutex_init(&priv->lock);
 }
 
 void netdev_nl_sock_priv_destroy(struct netdev_nl_sock *priv)
@@ -940,11 +943,13 @@ void netdev_nl_sock_priv_destroy(struct netdev_nl_sock *priv)
 	struct net_devmem_dmabuf_binding *binding;
 	struct net_devmem_dmabuf_binding *temp;
 
+	mutex_lock(&priv->lock);
 	list_for_each_entry_safe(binding, temp, &priv->bindings, list) {
 		rtnl_lock();
 		net_devmem_unbind_dmabuf(binding);
 		rtnl_unlock();
 	}
+	mutex_unlock(&priv->lock);
 }
 
 static int netdev_genl_netdevice_event(struct notifier_block *nb,
-- 
2.48.1

On Fri,  7 Mar 2025 07:57:24 -0800 Stanislav Fomichev wrote:
> diff --git a/net/core/netdev-genl.c b/net/core/netdev-genl.c
> index a219be90c739..8acdeeae24e7 100644
> --- a/net/core/netdev-genl.c
> +++ b/net/core/netdev-genl.c
> @@ -859,6 +859,7 @@ int netdev_nl_bind_rx_doit(struct sk_buff *skb, struct genl_info *info)
>  		goto err_genlmsg_free;
>  	}
>  
> +	mutex_lock(&priv->lock);
>  	rtnl_lock();
>  
>  	netdev = __dev_get_by_index(genl_info_net(info), ifindex);
> @@ -925,6 +926,7 @@ int netdev_nl_bind_rx_doit(struct sk_buff *skb, struct genl_info *info)
>  	net_devmem_unbind_dmabuf(binding);
>  err_unlock:
>  	rtnl_unlock();
> +	mutex_unlock(&priv->lock);
>  err_genlmsg_free:
>  	nlmsg_free(rsp);
>  	return err;

I think you're missing an unlock before successful return here no?

On 03/07, Jakub Kicinski wrote:
> On Fri,  7 Mar 2025 07:57:24 -0800 Stanislav Fomichev wrote:
> > diff --git a/net/core/netdev-genl.c b/net/core/netdev-genl.c
> > index a219be90c739..8acdeeae24e7 100644
> > --- a/net/core/netdev-genl.c
> > +++ b/net/core/netdev-genl.c
> > @@ -859,6 +859,7 @@ int netdev_nl_bind_rx_doit(struct sk_buff *skb, struct genl_info *info)
> >  		goto err_genlmsg_free;
> >  	}
> >  
> > +	mutex_lock(&priv->lock);
> >  	rtnl_lock();
> >  
> >  	netdev = __dev_get_by_index(genl_info_net(info), ifindex);
> > @@ -925,6 +926,7 @@ int netdev_nl_bind_rx_doit(struct sk_buff *skb, struct genl_info *info)
> >  	net_devmem_unbind_dmabuf(binding);
> >  err_unlock:
> >  	rtnl_unlock();
> > +	mutex_unlock(&priv->lock);
> >  err_genlmsg_free:
> >  	nlmsg_free(rsp);
> >  	return err;
> 
> I think you're missing an unlock before successful return here no?

Yes, thanks! :-( I have tested some of this code with Mina's latest TX + my
loopback mode, but it doesn't have any RX tests.. Will try to hack
something together to run RX bind before I repost.

On Fri, Mar 7, 2025 at 3:43 PM Stanislav Fomichev <stfomichev@gmail.com> wrote:
>
> On 03/07, Jakub Kicinski wrote:
> > On Fri,  7 Mar 2025 07:57:24 -0800 Stanislav Fomichev wrote:
> > > diff --git a/net/core/netdev-genl.c b/net/core/netdev-genl.c
> > > index a219be90c739..8acdeeae24e7 100644
> > > --- a/net/core/netdev-genl.c
> > > +++ b/net/core/netdev-genl.c
> > > @@ -859,6 +859,7 @@ int netdev_nl_bind_rx_doit(struct sk_buff *skb, struct genl_info *info)
> > >             goto err_genlmsg_free;
> > >     }
> > >
> > > +   mutex_lock(&priv->lock);
> > >     rtnl_lock();
> > >
> > >     netdev = __dev_get_by_index(genl_info_net(info), ifindex);
> > > @@ -925,6 +926,7 @@ int netdev_nl_bind_rx_doit(struct sk_buff *skb, struct genl_info *info)
> > >     net_devmem_unbind_dmabuf(binding);
> > >  err_unlock:
> > >     rtnl_unlock();
> > > +   mutex_unlock(&priv->lock);
> > >  err_genlmsg_free:
> > >     nlmsg_free(rsp);
> > >     return err;
> >
> > I think you're missing an unlock before successful return here no?
>
> Yes, thanks! :-( I have tested some of this code with Mina's latest TX + my
> loopback mode, but it doesn't have any RX tests.. Will try to hack
> something together to run RX bind before I repost.

Is the existing RX test not working for you?

Also running `./ncdevmem` manually on a driver you have that supports
devmem will test the binding patch.

I wonder if we can change list_head to xarray, which manages its own
locking, instead of list_head plus manual locking. Just an idea, I
don't have a strong preference here. It may be annoying that xarray do
lookups by an index, so we have to store the index somewhere. But if
all we do here is add to the xarray and later loop over it to unbind
elements, we don't need to store the indexes anywhere.

--
Thanks,
Mina

On 03/09, Mina Almasry wrote:
> On Fri, Mar 7, 2025 at 3:43 PM Stanislav Fomichev <stfomichev@gmail.com> wrote:
> >
> > On 03/07, Jakub Kicinski wrote:
> > > On Fri,  7 Mar 2025 07:57:24 -0800 Stanislav Fomichev wrote:
> > > > diff --git a/net/core/netdev-genl.c b/net/core/netdev-genl.c
> > > > index a219be90c739..8acdeeae24e7 100644
> > > > --- a/net/core/netdev-genl.c
> > > > +++ b/net/core/netdev-genl.c
> > > > @@ -859,6 +859,7 @@ int netdev_nl_bind_rx_doit(struct sk_buff *skb, struct genl_info *info)
> > > >             goto err_genlmsg_free;
> > > >     }
> > > >
> > > > +   mutex_lock(&priv->lock);
> > > >     rtnl_lock();
> > > >
> > > >     netdev = __dev_get_by_index(genl_info_net(info), ifindex);
> > > > @@ -925,6 +926,7 @@ int netdev_nl_bind_rx_doit(struct sk_buff *skb, struct genl_info *info)
> > > >     net_devmem_unbind_dmabuf(binding);
> > > >  err_unlock:
> > > >     rtnl_unlock();
> > > > +   mutex_unlock(&priv->lock);
> > > >  err_genlmsg_free:
> > > >     nlmsg_free(rsp);
> > > >     return err;
> > >
> > > I think you're missing an unlock before successful return here no?
> >
> > Yes, thanks! :-( I have tested some of this code with Mina's latest TX + my
> > loopback mode, but it doesn't have any RX tests.. Will try to hack
> > something together to run RX bind before I repost.
> 
> Is the existing RX test not working for you?
> 
> Also running `./ncdevmem` manually on a driver you have that supports
> devmem will test the binding patch.

It's a bit of a pita to run everything right now since drivers are
not in the tree :-(
 
> I wonder if we can change list_head to xarray, which manages its own
> locking, instead of list_head plus manual locking. Just an idea, I
> don't have a strong preference here. It may be annoying that xarray do
> lookups by an index, so we have to store the index somewhere. But if
> all we do here is add to the xarray and later loop over it to unbind
> elements, we don't need to store the indexes anywhere.

Yeah, having to keep the index around might be a bit awkward. And
since this is not a particularly performance sensitive place, let's
keep it as is for now?

On Sun, Mar 9, 2025 at 10:02 PM Stanislav Fomichev <stfomichev@gmail.com> wrote:
>
> On 03/09, Mina Almasry wrote:
> > On Fri, Mar 7, 2025 at 3:43 PM Stanislav Fomichev <stfomichev@gmail.com> wrote:
> > >
> > > On 03/07, Jakub Kicinski wrote:
> > > > On Fri,  7 Mar 2025 07:57:24 -0800 Stanislav Fomichev wrote:
> > > > > diff --git a/net/core/netdev-genl.c b/net/core/netdev-genl.c
> > > > > index a219be90c739..8acdeeae24e7 100644
> > > > > --- a/net/core/netdev-genl.c
> > > > > +++ b/net/core/netdev-genl.c
> > > > > @@ -859,6 +859,7 @@ int netdev_nl_bind_rx_doit(struct sk_buff *skb, struct genl_info *info)
> > > > >             goto err_genlmsg_free;
> > > > >     }
> > > > >
> > > > > +   mutex_lock(&priv->lock);
> > > > >     rtnl_lock();
> > > > >
> > > > >     netdev = __dev_get_by_index(genl_info_net(info), ifindex);
> > > > > @@ -925,6 +926,7 @@ int netdev_nl_bind_rx_doit(struct sk_buff *skb, struct genl_info *info)
> > > > >     net_devmem_unbind_dmabuf(binding);
> > > > >  err_unlock:
> > > > >     rtnl_unlock();
> > > > > +   mutex_unlock(&priv->lock);
> > > > >  err_genlmsg_free:
> > > > >     nlmsg_free(rsp);
> > > > >     return err;
> > > >
> > > > I think you're missing an unlock before successful return here no?
> > >
> > > Yes, thanks! :-( I have tested some of this code with Mina's latest TX + my
> > > loopback mode, but it doesn't have any RX tests.. Will try to hack
> > > something together to run RX bind before I repost.
> >
> > Is the existing RX test not working for you?
> >
> > Also running `./ncdevmem` manually on a driver you have that supports
> > devmem will test the binding patch.
>
> It's a bit of a pita to run everything right now since drivers are
> not in the tree :-(
>
> > I wonder if we can change list_head to xarray, which manages its own
> > locking, instead of list_head plus manual locking. Just an idea, I
> > don't have a strong preference here. It may be annoying that xarray do
> > lookups by an index, so we have to store the index somewhere. But if
> > all we do here is add to the xarray and later loop over it to unbind
> > elements, we don't need to store the indexes anywhere.
>
> Yeah, having to keep the index around might be a bit awkward. And
> since this is not a particularly performance sensitive place, let's
> keep it as is for now?

No strong preference from my end.
-- 
Thanks,
Mina

On Fri,  7 Mar 2025 07:57:24 -0800 Stanislav Fomichev wrote:
> As we move away from rtnl_lock for queue ops, introduce
> per-netdev_nl_sock lock.

What is it protecting?

On 03/07, Jakub Kicinski wrote:
> On Fri,  7 Mar 2025 07:57:24 -0800 Stanislav Fomichev wrote:
> > As we move away from rtnl_lock for queue ops, introduce
> > per-netdev_nl_sock lock.
> 
> What is it protecting?

The 'bindings' field of the netlink socket:

struct netdev_nl_sock {
       struct mutex lock;
       struct list_head bindings; <<<
};

I'm assuming it's totally valid to have several bindings per socket?
(attached to different rx queues)

On Fri, 7 Mar 2025 11:35:23 -0800 Stanislav Fomichev wrote:
> On 03/07, Jakub Kicinski wrote:
> > On Fri,  7 Mar 2025 07:57:24 -0800 Stanislav Fomichev wrote:  
> > > As we move away from rtnl_lock for queue ops, introduce
> > > per-netdev_nl_sock lock.  
> > 
> > What is it protecting?  
> 
> The 'bindings' field of the netlink socket:
> 
> struct netdev_nl_sock {
>        struct mutex lock;
>        struct list_head bindings; <<<
> };
> 
> I'm assuming it's totally valid to have several bindings per socket?

Totally, sorry, I got confused by there being two xarrays.
Lock on the socket state makes sense.