[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240606233512.37483-1-kuniyu@amazon.com>
Date: Thu, 6 Jun 2024 16:35:12 -0700
From: Kuniyuki Iwashima <kuniyu@...zon.com>
To: <kuba@...nel.org>
CC: <Liam.Howlett@...cle.com>, <anjali.k.kulkarni@...cle.com>,
<davem@...emloft.net>, <dsahern@...il.com>, <edumazet@...gle.com>,
<jiri@...nulli.us>, <netdev@...r.kernel.org>, <pabeni@...hat.com>,
<kuniyu@...zon.com>
Subject: Re: [PATCH net-next 2/2] net: netlink: remove the cb_mutex "injection" from netlink core
From: Jakub Kicinski <kuba@...nel.org>
Date: Thu, 6 Jun 2024 12:29:06 -0700
> Back in 2007, in commit af65bdfce98d ("[NETLINK]: Switch cb_lock spinlock
> to mutex and allow to override it") netlink core was extended to allow
> subsystems to replace the dump mutex lock with its own lock.
>
> The mechanism was used by rtnetlink to take rtnl_lock but it isn't
> sufficiently flexible for other users. Over the 17 years since
> it was added no other user appeared. Since rtnetlink needs conditional
> locking now, and doesn't use it either, axe this feature complete.
>
> Signed-off-by: Jakub Kicinski <kuba@...nel.org>
Reviewed-by: Kuniyuki Iwashima <kuniyu@...zon.com>
Thanks!
> ---
> CC: anjali.k.kulkarni@...cle.com
> CC: Liam.Howlett@...cle.com
> CC: jiri@...nulli.us
> ---
> include/linux/netlink.h | 1 -
> net/netlink/af_netlink.c | 18 +++---------------
> 2 files changed, 3 insertions(+), 16 deletions(-)
>
> diff --git a/include/linux/netlink.h b/include/linux/netlink.h
> index 5df7340d4dab..b332c2048c75 100644
> --- a/include/linux/netlink.h
> +++ b/include/linux/netlink.h
> @@ -47,7 +47,6 @@ struct netlink_kernel_cfg {
> unsigned int groups;
> unsigned int flags;
> void (*input)(struct sk_buff *skb);
> - struct mutex *cb_mutex;
> int (*bind)(struct net *net, int group);
> void (*unbind)(struct net *net, int group);
> void (*release) (struct sock *sk, unsigned long *groups);
> diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
> index 8bbbe75e75db..0b7a89db3ab7 100644
> --- a/net/netlink/af_netlink.c
> +++ b/net/netlink/af_netlink.c
> @@ -636,8 +636,7 @@ static struct proto netlink_proto = {
> };
>
> static int __netlink_create(struct net *net, struct socket *sock,
> - struct mutex *dump_cb_mutex, int protocol,
> - int kern)
> + int protocol, int kern)
> {
> struct sock *sk;
> struct netlink_sock *nlk;
> @@ -655,7 +654,6 @@ static int __netlink_create(struct net *net, struct socket *sock,
> lockdep_set_class_and_name(&nlk->nl_cb_mutex,
> nlk_cb_mutex_keys + protocol,
> nlk_cb_mutex_key_strings[protocol]);
> - nlk->dump_cb_mutex = dump_cb_mutex;
> init_waitqueue_head(&nlk->wait);
>
> sk->sk_destruct = netlink_sock_destruct;
> @@ -667,7 +665,6 @@ static int netlink_create(struct net *net, struct socket *sock, int protocol,
> int kern)
> {
> struct module *module = NULL;
> - struct mutex *cb_mutex;
> struct netlink_sock *nlk;
> int (*bind)(struct net *net, int group);
> void (*unbind)(struct net *net, int group);
> @@ -696,7 +693,6 @@ static int netlink_create(struct net *net, struct socket *sock, int protocol,
> module = nl_table[protocol].module;
> else
> err = -EPROTONOSUPPORT;
> - cb_mutex = nl_table[protocol].cb_mutex;
> bind = nl_table[protocol].bind;
> unbind = nl_table[protocol].unbind;
> release = nl_table[protocol].release;
> @@ -705,7 +701,7 @@ static int netlink_create(struct net *net, struct socket *sock, int protocol,
> if (err < 0)
> goto out;
>
> - err = __netlink_create(net, sock, cb_mutex, protocol, kern);
> + err = __netlink_create(net, sock, protocol, kern);
> if (err < 0)
> goto out_module;
>
> @@ -2016,7 +2012,6 @@ __netlink_kernel_create(struct net *net, int unit, struct module *module,
> struct sock *sk;
> struct netlink_sock *nlk;
> struct listeners *listeners = NULL;
> - struct mutex *cb_mutex = cfg ? cfg->cb_mutex : NULL;
> unsigned int groups;
>
> BUG_ON(!nl_table);
> @@ -2027,7 +2022,7 @@ __netlink_kernel_create(struct net *net, int unit, struct module *module,
> if (sock_create_lite(PF_NETLINK, SOCK_DGRAM, unit, &sock))
> return NULL;
>
> - if (__netlink_create(net, sock, cb_mutex, unit, 1) < 0)
> + if (__netlink_create(net, sock, unit, 1) < 0)
> goto out_sock_release_nosk;
>
> sk = sock->sk;
> @@ -2055,7 +2050,6 @@ __netlink_kernel_create(struct net *net, int unit, struct module *module,
> if (!nl_table[unit].registered) {
> nl_table[unit].groups = groups;
> rcu_assign_pointer(nl_table[unit].listeners, listeners);
> - nl_table[unit].cb_mutex = cb_mutex;
> nl_table[unit].module = module;
> if (cfg) {
> nl_table[unit].bind = cfg->bind;
> @@ -2326,15 +2320,9 @@ static int netlink_dump(struct sock *sk, bool lock_taken)
> netlink_skb_set_owner_r(skb, sk);
>
> if (nlk->dump_done_errno > 0) {
> - struct mutex *extra_mutex = nlk->dump_cb_mutex;
> -
> cb->extack = &extack;
>
> - if (extra_mutex)
> - mutex_lock(extra_mutex);
> nlk->dump_done_errno = cb->dump(skb, cb);
> - if (extra_mutex)
> - mutex_unlock(extra_mutex);
>
> /* EMSGSIZE plus something already in the skb means
> * that there's more to dump but current skb has filled up.
> --
> 2.45.2
Powered by blists - more mailing lists