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

[netdev-bot+sashiko@kernel.org]

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] Changelog overstatement at this commit only: the commit message
  of "net: add sk_set_nospace() and sk_clear_nospace()" claims it…

--

net: add sk_set_nospace() and sk_clear_nospace()

This adds two helpers in net/core/sock.c that fold the sk->sk_socket
dereference and its NULL check into a single place, and converts the
SOCK_NOSPACE setters and clearers in core, tcp, kcm, mptcp, smc and tls
over to them.  It is described as a preparation patch for a later one
that gives TCP a private mirror of the bit.

> Add sk_set_nospace() and sk_clear_nospace() helpers and convert all
> the setters and clearers to them, so that "git grep _bit(SOCK_NOSPACE"
> only reports the two helpers and the remaining test_bit() sites.

[Severity: Low]
Is this description accurate for this patch on its own?  At this commit
the grep still reports twelve open-coded sites outside the two helpers:

  drivers/block/drbd/drbd_worker.c  set_bit(SOCK_NOSPACE, &sk->sk_socket->flags)
  drivers/nvme/host/tcp.c           clear_bit(SOCK_NOSPACE, &sk->sk_socket->flags)
  drivers/nvme/target/tcp.c         clear_bit(SOCK_NOSPACE, &sk->sk_socket->flags)
  fs/dlm/lowcomms.c                 clear_bit(SOCK_NOSPACE, &con->sock->flags)
  fs/dlm/lowcomms.c                 set_bit(SOCK_NOSPACE, &con->sock->sk->sk_socket->flags)
  net/ceph/messenger.c              clear_bit(SOCK_NOSPACE, &sk->sk_socket->flags)
  net/rds/tcp_send.c                set_bit(SOCK_NOSPACE, &tc->t_sock->sk->sk_socket->flags)
  net/rds/tcp_send.c                set_bit(SOCK_NOSPACE, &sk->sk_socket->flags)
  net/sunrpc/svcsock.c              set_bit(SOCK_NOSPACE, &svsk->sk_sock->flags)
  net/sunrpc/svcsock.c              clear_bit(SOCK_NOSPACE, &svsk->sk_sock->flags)
  net/sunrpc/xprtsock.c             set_bit(SOCK_NOSPACE, &sk->sk_socket->flags)
  net/sunrpc/xprtsock.c             clear_bit(SOCK_NOSPACE, &sk->sk_socket->flags)

The remaining sites are all converted by later patches in the series
("sunrpc: use sk_set_nospace() and sk_clear_nospace()", "rds: use
sk_set_nospace()", "dlm: use sk_set_nospace() and sk_clear_nospace()",
"drbd: use sk_set_nospace()", "nvme-tcp: use sk_clear_nospace()" and
"libceph: use sk_clear_nospace()"), and all six land before "tcp: add
tp->tcp_nospace", so the single choke point the mirror depends on is in
place by the time the mirror exists and no intermediate tree state is
broken.

Could the changelog say that the conversion is completed by the following
patches rather than by this one?

> Callers that had no NULL check are all called from user context with a
> socket attached, so folding the check into the helpers only makes them
> more robust.

The same "some with a NULL check, some without" wording in the first
paragraph reads as a description of the state after this patch, when
drbd_worker.c and net/ceph/messenger.c still carry their own checks.

<br>

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