| Message ID | 20250307155725.219009-4-sdf@fomichev.me (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: remove rtnl_lock from the callers of queue APIs | expand |
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.
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.
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,
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(+)