This series reduces the (small ?) contention over uevent_sock_mutex,
noticed when creating/deleting many network namespaces/devices.
1) uevent_seqnum becomes an atomic64_t
2) Only acquire uevent_sock_mutex whenever using uevent_sock_list
Eric Dumazet (2):
kobject: make uevent_seqnum atomic
kobject: reduce uevent_sock_mutex scope
include/linux/kobject.h | 2 +-
kernel/ksysfs.c | 2 +-
lib/kobject_uevent.c | 24 +++++++++++-------------
3 files changed, 13 insertions(+), 15 deletions(-)
--
2.43.0.687.g38aa6559b0-goog
We will soon no longer acquire uevent_sock_mutex
for most kobject_uevent_net_broadcast() calls,
and also while calling uevent_net_broadcast().
Make uevent_seqnum an atomic64_t to get its own protection.
This fixes a race while reading /sys/kernel/uevent_seqnum.
Signed-off-by: Eric Dumazet <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Christian Brauner <[email protected]>
---
include/linux/kobject.h | 2 +-
kernel/ksysfs.c | 2 +-
lib/kobject_uevent.c | 17 +++++++++--------
3 files changed, 11 insertions(+), 10 deletions(-)
diff --git a/include/linux/kobject.h b/include/linux/kobject.h
index c30affcc43b444cc17cb894b83b17b52e41f8ebc..c8219505a79f98bc370e52997efc8af51833cfda 100644
--- a/include/linux/kobject.h
+++ b/include/linux/kobject.h
@@ -38,7 +38,7 @@ extern char uevent_helper[];
#endif
/* counter to tag the uevent, read only except for the kobject core */
-extern u64 uevent_seqnum;
+extern atomic64_t uevent_seqnum;
/*
* The actions here must match the index to the string array
diff --git a/kernel/ksysfs.c b/kernel/ksysfs.c
index 1d4bc493b2f4b2e94133cec75e569bef3f3ead25..32ae7fa74a9c072a44f7280b950b97d25cb07baf 100644
--- a/kernel/ksysfs.c
+++ b/kernel/ksysfs.c
@@ -39,7 +39,7 @@ static struct kobj_attribute _name##_attr = __ATTR_RW(_name)
static ssize_t uevent_seqnum_show(struct kobject *kobj,
struct kobj_attribute *attr, char *buf)
{
- return sysfs_emit(buf, "%llu\n", (unsigned long long)uevent_seqnum);
+ return sysfs_emit(buf, "%llu\n", (u64)atomic64_read(&uevent_seqnum));
}
KERNEL_ATTR_RO(uevent_seqnum);
diff --git a/lib/kobject_uevent.c b/lib/kobject_uevent.c
index fb9a2f06dd1e79db0e5db17362c88152790e2b36..9cb1a7fdaeba4fc5c698fbe84f359fb305345be1 100644
--- a/lib/kobject_uevent.c
+++ b/lib/kobject_uevent.c
@@ -30,7 +30,7 @@
#include <net/net_namespace.h>
-u64 uevent_seqnum;
+atomic64_t uevent_seqnum;
#ifdef CONFIG_UEVENT_HELPER
char uevent_helper[UEVENT_HELPER_PATH_LEN] = CONFIG_UEVENT_HELPER_PATH;
#endif
@@ -44,7 +44,7 @@ struct uevent_sock {
static LIST_HEAD(uevent_sock_list);
#endif
-/* This lock protects uevent_seqnum and uevent_sock_list */
+/* This lock protects uevent_sock_list */
static DEFINE_MUTEX(uevent_sock_mutex);
/* the strings here must match the enum in include/linux/kobject.h */
@@ -583,13 +583,13 @@ int kobject_uevent_env(struct kobject *kobj, enum kobject_action action,
break;
}
- mutex_lock(&uevent_sock_mutex);
/* we will send an event, so request a new sequence number */
- retval = add_uevent_var(env, "SEQNUM=%llu", ++uevent_seqnum);
- if (retval) {
- mutex_unlock(&uevent_sock_mutex);
+ retval = add_uevent_var(env, "SEQNUM=%llu",
+ atomic64_inc_return(&uevent_seqnum));
+ if (retval)
goto exit;
- }
+
+ mutex_lock(&uevent_sock_mutex);
retval = kobject_uevent_net_broadcast(kobj, env, action_string,
devpath);
mutex_unlock(&uevent_sock_mutex);
@@ -688,7 +688,8 @@ static int uevent_net_broadcast(struct sock *usk, struct sk_buff *skb,
int ret;
/* bump and prepare sequence number */
- ret = snprintf(buf, sizeof(buf), "SEQNUM=%llu", ++uevent_seqnum);
+ ret = snprintf(buf, sizeof(buf), "SEQNUM=%llu",
+ atomic64_inc_return(&uevent_seqnum));
if (ret < 0 || (size_t)ret >= sizeof(buf))
return -ENOMEM;
ret++;
--
2.43.0.687.g38aa6559b0-goog
This is a followup of commit a3498436b3a0 ("netns: restrict uevents")
- uevent_sock_mutex no longer protects uevent_seqnum thanks
to prior patch in the series.
- uevent_net_broadcast() can run without holding uevent_sock_mutex.
- Instead of grabbing uevent_sock_mutex before calling
kobject_uevent_net_broadcast(), we can move the
mutex_lock(&uevent_sock_mutex) to the place we iterate over
uevent_sock_list : uevent_net_broadcast_untagged().
After this patch, typical netdevice creations and destructions
calling uevent_net_broadcast_tagged() no longer need to acquire
uevent_sock_mutex.
Signed-off-by: Eric Dumazet <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Christian Brauner <[email protected]>
---
lib/kobject_uevent.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/lib/kobject_uevent.c b/lib/kobject_uevent.c
index 9cb1a7fdaeba4fc5c698fbe84f359fb305345be1..03b427e2707e357ab12abeb9da234432c4bc0fb3 100644
--- a/lib/kobject_uevent.c
+++ b/lib/kobject_uevent.c
@@ -42,10 +42,9 @@ struct uevent_sock {
#ifdef CONFIG_NET
static LIST_HEAD(uevent_sock_list);
-#endif
-
/* This lock protects uevent_sock_list */
static DEFINE_MUTEX(uevent_sock_mutex);
+#endif
/* the strings here must match the enum in include/linux/kobject.h */
static const char *kobject_actions[] = {
@@ -315,6 +314,7 @@ static int uevent_net_broadcast_untagged(struct kobj_uevent_env *env,
int retval = 0;
/* send netlink message */
+ mutex_lock(&uevent_sock_mutex);
list_for_each_entry(ue_sk, &uevent_sock_list, list) {
struct sock *uevent_sock = ue_sk->sk;
@@ -334,6 +334,7 @@ static int uevent_net_broadcast_untagged(struct kobj_uevent_env *env,
if (retval == -ENOBUFS || retval == -ESRCH)
retval = 0;
}
+ mutex_unlock(&uevent_sock_mutex);
consume_skb(skb);
return retval;
@@ -589,10 +590,8 @@ int kobject_uevent_env(struct kobject *kobj, enum kobject_action action,
if (retval)
goto exit;
- mutex_lock(&uevent_sock_mutex);
retval = kobject_uevent_net_broadcast(kobj, env, action_string,
devpath);
- mutex_unlock(&uevent_sock_mutex);
#ifdef CONFIG_UEVENT_HELPER
/* call uevent_helper, usually only enabled during early boot */
@@ -743,9 +742,7 @@ static int uevent_net_rcv_skb(struct sk_buff *skb, struct nlmsghdr *nlh,
return -EPERM;
}
- mutex_lock(&uevent_sock_mutex);
ret = uevent_net_broadcast(net->uevent_sock->sk, skb, extack);
- mutex_unlock(&uevent_sock_mutex);
return ret;
}
--
2.43.0.687.g38aa6559b0-goog
On Wed, Feb 14, 2024 at 08:48:27AM +0000, Eric Dumazet wrote:
> This series reduces the (small ?) contention over uevent_sock_mutex,
> noticed when creating/deleting many network namespaces/devices.
>
> 1) uevent_seqnum becomes an atomic64_t
>
> 2) Only acquire uevent_sock_mutex whenever using uevent_sock_list
Cool, any boot-time measured speedups from this? Or is this just tiny
optimizations that you noticed doing reviews?
thanks,
greg k-h
On Wed, Feb 14, 2024 at 11:34 AM Greg Kroah-Hartman
<[email protected]> wrote:
>
> On Wed, Feb 14, 2024 at 08:48:27AM +0000, Eric Dumazet wrote:
> > This series reduces the (small ?) contention over uevent_sock_mutex,
> > noticed when creating/deleting many network namespaces/devices.
> >
> > 1) uevent_seqnum becomes an atomic64_t
> >
> > 2) Only acquire uevent_sock_mutex whenever using uevent_sock_list
>
> Cool, any boot-time measured speedups from this? Or is this just tiny
> optimizations that you noticed doing reviews?
No impressive nice numbers yet, the main bottleneck is still rtnl,
which I am working on net-next tree.
Other candidates are : rdma_nets_rwsem, proc_subdir_lock, pcpu_alloc_mutex, ...
Christian made the much needed changes [1], since the last time I took
a look at kobject (this was in 2017 !)
[1]
commit a3498436b3a0f8ec289e6847e1de40b4123e1639
Author: Christian Brauner <[email protected]>
Date: Sun Apr 29 12:44:12 2018 +0200
netns: restrict uevents
On Wed, Feb 14, 2024 at 08:48:28AM +0000, Eric Dumazet wrote:
> We will soon no longer acquire uevent_sock_mutex
> for most kobject_uevent_net_broadcast() calls,
> and also while calling uevent_net_broadcast().
>
> Make uevent_seqnum an atomic64_t to get its own protection.
>
> This fixes a race while reading /sys/kernel/uevent_seqnum.
>
> Signed-off-by: Eric Dumazet <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: Christian Brauner <[email protected]>
> ---
Nice,
Reviewed-by: Christian Brauner <[email protected]>
On Wed, Feb 14, 2024 at 08:48:29AM +0000, Eric Dumazet wrote:
> This is a followup of commit a3498436b3a0 ("netns: restrict uevents")
>
> - uevent_sock_mutex no longer protects uevent_seqnum thanks
> to prior patch in the series.
>
> - uevent_net_broadcast() can run without holding uevent_sock_mutex.
>
> - Instead of grabbing uevent_sock_mutex before calling
> kobject_uevent_net_broadcast(), we can move the
> mutex_lock(&uevent_sock_mutex) to the place we iterate over
> uevent_sock_list : uevent_net_broadcast_untagged().
>
> After this patch, typical netdevice creations and destructions
> calling uevent_net_broadcast_tagged() no longer need to acquire
> uevent_sock_mutex.
>
> Signed-off-by: Eric Dumazet <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: Christian Brauner <[email protected]>
> ---
Very nice,
Reviewed-by: Christian Brauner <[email protected]>