Re: [PATCH v2 net-next 2/9] net: add sk_set_nospace() and sk_clear_nospace()

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The kernel-doc added for sk_clear_nospace() in net/core/sock.c
  says the helper is "Called from ->sk_write_space() handlers, once send…

--

net: add sk_set_nospace() and sk_clear_nospace()

This patch adds sk_set_nospace() and sk_clear_nospace() helpers that wrap
the SOCK_NOSPACE bit operations on sk->sk_socket->flags with a NULL check.
It also converts the core networking, tcp, kcm, mptcp, smc and tls setters
and clearers to use them. It prepares for a later patch that gives TCP a
private copy of the bit and needs a single choke point.

[ ... ]

+/**
+ *	sk_clear_nospace - tell the transport no writer is waiting for space
+ *	@sk: socket
+ *
+ *	Called from ->sk_write_space() handlers, once send space has been
+ *	made available to writers.
+ */

[Severity: Low]
This isn't a bug, but does this kernel-doc match how the helper is used?
One caller converted in this same patch doesn't follow the documented
calling context.

kcm_tx_work() in net/kcm/kcmsock.c is a work item handler
(INIT_WORK(&kcm->tx_work, kcm_tx_work)), not a ->sk_write_space() handler.
It only tests SOCK_NOSPACE and clears it without checking whether send
space is available. After that it calls ->sk_write_space():

net/kcm/kcmsock.c:kcm_tx_work() {
    ...
	if (likely(sk->sk_socket) &&
	    test_bit(SOCK_NOSPACE, &sk->sk_socket->flags)) {
		sk_clear_nospace(sk);
		sk->sk_write_space(sk);
	}
    ...
}

Other callers converted later in the series are also outside the
documented context. For example, the sunrpc svcsock code clears the bit
from a helper that checks wspace. The doc text is still the same at the
end of the series.

The commit message calls these helpers the single choke point that the
follow-up tcp_nospace patch depends on. Could the kernel-doc describe the
actual calling contexts, so nobody relies on a stricter contract when
adding ordering or locking to the helper later?

+void sk_clear_nospace(struct sock *sk)
+{
+	struct socket *sock = sk->sk_socket;
+
+	if (sock)
+		clear_bit(SOCK_NOSPACE, &sock->flags);
+}
+EXPORT_SYMBOL(sk_clear_nospace);

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260924134729.2047213-1-edumazet%40google.com