Re: [PATCH v2 net-next 12/13] af_unix: Replace the big lock with small locks.
From: Kuniyuki Iwashima <hidden>
Date: 2021-11-18 05:44:12
Subsystem:
networking [general], networking [unix sockets], the rest · Maintainers:
"David S. Miller", Eric Dumazet, Jakub Kicinski, Paolo Abeni, Kuniyuki Iwashima, Linus Torvalds
I'm resending this not to be missed because I sent this to myself by mistake.. I'm sorry for bothering you. From: Kuniyuki Iwashima <redacted> Date: Sun, 14 Nov 2021 10:24:27 +0900
The hash table of AF_UNIX sockets is protected by the single lock. This patch replaces it with per-hash locks.
[...]
+static void unix_table_double_lock(unsigned int hash1, unsigned int hash2)
+{
+ /* hash1 and hash2 is never the same because
+ * one is between 0 and UNIX_HASH_SIZE - 1, and
+ * another is between UNIX_HASH_SIZE and UNIX_HASH_SIZE * 2.
+ */
+ if (hash1 > hash2)
+ swap(hash1, hash2);
+
+ spin_lock(&unix_table_locks[hash1]);
+ spin_lock_nested(&unix_table_locks[hash2], SINGLE_DEPTH_NESTING);
+}
+
+static void unix_table_double_unlock(unsigned int hash1, unsigned int hash2)
+{
+ spin_unlock(&unix_table_locks[hash1]);
+ spin_unlock(&unix_table_locks[hash2]);
+}The status is "Changes Requested" on patchwork because of some newly added sparse warnings. There are two kinds of warnings. One is about unix_table_double_lock/unlock() and the other is about unix_seq_start/stop(). https://patchwork.hopto.org/static/nipa/579645/12617773/build_32bit/summary ---8<--- +../net/unix/af_unix.c:159:13: warning: context imbalance in 'unix_table_double_lock' - wrong count at exit +../net/unix/af_unix.c:172:13: warning: context imbalance in 'unix_table_double_unlock' - unexpected unlock +../net/unix/af_unix.c:1258:13: warning: context imbalance in 'unix_state_double_lock' - wrong count at exit +../net/unix/af_unix.c:1276:17: warning: context imbalance in 'unix_state_double_unlock' - unexpected unlock +../net/unix/af_unix.c:1579:9: warning: context imbalance in 'unix_stream_connect' - different lock contexts for basic block +../net/unix/af_unix.c:1944:25: warning: context imbalance in 'unix_dgram_sendmsg' - unexpected unlock +../net/unix/af_unix.c:3255:28: warning: context imbalance in 'unix_next_socket' - unexpected unlock +../net/unix/af_unix.c:3284:28: warning: context imbalance in 'unix_seq_stop' - unexpected unlock ---8<--- We can avoid the former by adding these annotations, but it seems a little bit redundant. Also, there has already been the same kind of warnings for unix_state_double_lock() without sparse annotations. ---8<---
diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index 89a844e7141b..b26a2ea26029 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c@@ -157,6 +157,8 @@ static unsigned int unix_abstract_hash(struct sockaddr_un *sunaddr, } static void unix_table_double_lock(unsigned int hash1, unsigned int hash2) + __acquires(unix_table_locks) + __acquires(unix_table_locks) { /* hash1 and hash2 is never the same because * one is between 0 and UNIX_HASH_SIZE - 1, and
@@ -170,6 +172,8 @@ static void unix_table_double_lock(unsigned int hash1, unsigned int hash2) } static void unix_table_double_unlock(unsigned int hash1, unsigned int hash2) + __releases(unix_table_locks) + __releases(unix_table_locks) { spin_unlock(&unix_table_locks[hash1]); spin_unlock(&unix_table_locks[hash2]); ---8<---
[...]
quoted hunk ↗ jump to hunk
@@ -3216,7 +3235,7 @@ static struct sock *unix_next_socket(struct seq_file *seq, struct sock *sk, loff_t *pos) { - unsigned long bucket; + unsigned long bucket = get_bucket(*pos); while (sk > (struct sock *)SEQ_START_TOKEN) { sk = sk_next(sk);@@ -3227,12 +3246,13 @@ static struct sock *unix_next_socket(struct seq_file *seq, } do { + spin_lock(&unix_table_locks[bucket]); sk = unix_from_bucket(seq, pos); if (sk) return sk; next_bucket: - bucket = get_bucket(*pos) + 1; + spin_unlock(&unix_table_locks[bucket++]); *pos = set_bucket_offset(bucket, 1); } while (bucket < ARRAY_SIZE(unix_socket_table));@@ -3240,10 +3260,7 @@ static struct sock *unix_next_socket(struct seq_file *seq, } static void *unix_seq_start(struct seq_file *seq, loff_t *pos) - __acquires(unix_table_lock) { - spin_lock(&unix_table_lock); - if (!*pos) return SEQ_START_TOKEN;@@ -3260,9 +3277,11 @@ static void *unix_seq_next(struct seq_file *seq, void *v, loff_t *pos) } static void unix_seq_stop(struct seq_file *seq, void *v) - __releases(unix_table_lock) { - spin_unlock(&unix_table_lock); + struct sock *sk = v; + + if (sk) + spin_unlock(&unix_table_locks[sk->sk_hash]); } static int unix_seq_show(struct seq_file *seq, void *v)
[...] The latter happens by replacing the big lock with per-hash locks. It moves spin_lock() from unix_seq_start() to unix_next_socket(). unix_next_socket() keeps holding a lock until it returns NULL, so Sparse cannot understand the logic. At least, we can add __releases annotation in unix_seq_stop(), but it rather increases warnings. And tcp_seq_stop() does not have annotations. Are these warnings acceptable, or is there any better way?