Re: [PATCH net-next v4 3/4] vsock/virtio: coalesce RX write-space notifications in lock batches

[Jia Jia]

>
> On Wed, Oct 14, 2026 at 07:56:39PM +0800, Jia Jia wrote:
> >Each received packet updates peer credit and may call sk_write_space().
> >A writer cannot use that credit until the batch releases the socket lock,
> >so one notification at batch finish is sufficient.
> >
> >Save the initial sk_write_space callback when the AF_VSOCK socket is
> >created and require it to remain installed for batching. This lets the
> >transport module deliver the deferred notification without referring to
> >the unexported default callback. Sockets with replaced callbacks retain
> >their per-packet behavior.
> >
> >Record the batch socket before processing its first packet so a batch that
> >ends on that packet cannot lose the pending notification.
>
> mmm, is this a fix of the previous patch?
>

No. Patch 2 preserves the existing per-packet sk_write_space()
behavior, so Patch 3 is not a correctness fix for it. Patch 3 only
coalesces the native/ default notifications.

Moving batch->sk earlier is required only after the deferred state is
introduced here: the socket must already be recorded before the first
packet can set write_space_pending, so the notification can still be
delivered if that packet immediately ends the batch. Sorry, the commit
message did not make this distinction clear.

> >
> >Signed-off-by: Jia Jia <physicalmtea@xxxxxxxxx>
> >---
> > include/linux/virtio_vsock.h            |  1 +
> > include/net/af_vsock.h                  |  5 +++
> > net/vmw_vsock/af_vsock.c                |  1 +
> > net/vmw_vsock/virtio_transport_common.c | 53 +++++++++++++++++--------
> > 4 files changed, 44 insertions(+), 16 deletions(-)
> >
> >diff --git a/include/linux/virtio_vsock.h b/include/linux/virtio_vsock.h
> >index 26dde6909c4ed..bf077ee0ded88 100644
> >--- a/include/linux/virtio_vsock.h
> >+++ b/include/linux/virtio_vsock.h
> >@@ -287,6 +287,7 @@ struct virtio_transport_rx_batch {
> >       struct sock *sk;
> >       unsigned int pkts;
> >       size_t bytes;
> >+      bool write_space_pending;
> > };
> >
> > void virtio_transport_recv_pkt_batch(struct virtio_transport *t,
> >diff --git a/include/net/af_vsock.h b/include/net/af_vsock.h
> >index 5549298c1ec6b..9a40960d10cbe 100644
> >--- a/include/net/af_vsock.h
> >+++ b/include/net/af_vsock.h
> >@@ -63,6 +63,11 @@ struct vsock_sock {
> >       u32 peer_shutdown;
> >       bool sent_request;
> >       bool ignore_connecting_rst;
> >+      /*
> >+       * Initial callback, used to identify replacements and deliver deferred
> >+       * write-space notifications.
> >+       */
> >+      void (*default_write_space)(struct sock *sk);
>
> This is kind of virtio-vsock specific, why need to be here?
>

I stored this state here because __vsock_create() can capture the
native callback before the socket is exposed or any callback
replacement occurs, and the transport cannot directly reference the
unexported default callback. However, I agree that exposing this
virtio-specific state directly in struct vsock_sock is not ideal. I'll
look at hiding it behind an AF_VSOCK helper in v5 while preserving the
capture point and the lifetime required by deferred callbacks.


> >
> >       /* Protected by lock_sock(sk) */
> >       u64 buffer_size;
> >diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
> >index 44cee74519555..fe7f49da6c4bc 100644
> >--- a/net/vmw_vsock/af_vsock.c
> >+++ b/net/vmw_vsock/af_vsock.c
> >@@ -958,6 +958,7 @@ static struct sock *__vsock_create(struct net *net,
> >               sk->sk_type = type;
> >
> >       vsk = vsock_sk(sk);
> >+      vsk->default_write_space = sk->sk_write_space;
> >       vsock_addr_init(&vsk->local_addr, VMADDR_CID_ANY, VMADDR_PORT_ANY);
> >       vsock_addr_init(&vsk->remote_addr, VMADDR_CID_ANY, VMADDR_PORT_ANY);
> >
> >diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
> >index ff74e805527a5..f4b5f0230493e 100644
> >--- a/net/vmw_vsock/virtio_transport_common.c
> >+++ b/net/vmw_vsock/virtio_transport_common.c
> >@@ -1820,6 +1820,7 @@ virtio_transport_recv_pkt_batchable(struct virtio_transport *t,
> >       return sk->sk_state == TCP_ESTABLISHED &&
> >              sk->sk_type == SOCK_STREAM &&
> >              READ_ONCE(sk->sk_prot) == sk->sk_prot_creator &&
> >+             READ_ONCE(sk->sk_write_space) == vsk->default_write_space &&
>
> Why?
>

I believe this check is necessary because notification coalescing is
only safe for the original native sk_write_space callback. Sockmap can
replace this callback under sk_callback_lock, which is independent of
the socket lock held across the batch, and the replacement may require
per-packet handling. Comparing it with the callback captured at socket
creation lets us fall back to the existing per-packet behavior when it
has been replaced. I'll add a comment explaining this.


> >              !sock_flag(sk, SOCK_DONE) &&
> >              vsk->transport == &t->transport &&
> >              vvs && virtio_transport_rx_skb_has_headroom(vvs) &&
> >@@ -1828,13 +1829,15 @@ virtio_transport_recv_pkt_batchable(struct virtio_transport *t,
> >
> > /*
> >  * The caller holds sk's socket lock.  Set @batchable if the socket can remain
> >- * locked for another ordinary STREAM/RW packet.  Return true if the caller
> >- * must free @skb.
> >+ * locked for another ordinary STREAM/RW packet.  When @batch is non-NULL,
> >+ * record a default write-space notification for batch finish instead of
> >+ * delivering it immediately.  Return true if the caller must free @skb.
> >  */
> > static bool
> > virtio_transport_recv_pkt_locked(struct virtio_transport *t,
> >                                struct sk_buff *skb, struct sock *sk,
> >-                               bool *batchable)
> >+                               bool *batchable,
> >+                               struct virtio_transport_rx_batch *batch)
> > {
> >       struct virtio_vsock_hdr *hdr = virtio_vsock_hdr(skb);
> >       struct sockaddr_vm src;
> >@@ -1863,8 +1866,12 @@ virtio_transport_recv_pkt_locked(struct virtio_transport *t,
> >       if (vsk->local_addr.svm_cid != VMADDR_CID_ANY)
> >               vsk->local_addr.svm_cid = le64_to_cpu(hdr->dst_cid);
> >
> >-      if (space_available)
> >-              sk->sk_write_space(sk);
> >+      if (space_available) {
> >+              if (batch)
> >+                      batch->write_space_pending = true;
> >+              else
> >+                      sk->sk_write_space(sk);
> >+      }
> >
> >       switch (sk->sk_state) {
> >       case TCP_LISTEN:
> >@@ -1931,7 +1938,7 @@ void virtio_transport_recv_pkt(struct virtio_transport *t,
> >       }
> >
> >       lock_sock(sk);
> >-      free_pkt = virtio_transport_recv_pkt_locked(t, skb, sk, NULL);
> >+      free_pkt = virtio_transport_recv_pkt_locked(t, skb, sk, NULL, NULL);
> >       release_sock(sk);
> >
> >       /* Release refcnt obtained when we fetched this socket out of the
> >@@ -1949,22 +1956,27 @@ EXPORT_SYMBOL_GPL(virtio_transport_recv_pkt);
> >
> > /*
> >  * Finish the RX batch.
> >- * For a non-empty batch, the caller must hold the socket lock acquired with
> >- * lock_sock(). This function releases the lock and the batch's lookup
> >- * reference.
> >+ * For a non-empty batch, the caller holds the socket lock acquired with
> >+ * lock_sock(). Deliver any deferred write-space notification while the lock
> >+ * is held, then release the lock and the batch's lookup reference.
> >  * An empty batch is a no-op.
> >  */
> > void virtio_transport_rx_batch_finish(struct virtio_transport_rx_batch *batch)
> > {
> >+      bool write_space_pending = batch->write_space_pending;
> >       struct sock *sk = batch->sk;
> >
> >       batch->sk = NULL;
> >       batch->pkts = 0;
> >       batch->bytes = 0;
> >+      batch->write_space_pending = false;
> >
> >       if (!sk)
> >               return;
> >
> >+      if (write_space_pending)
> >+              vsock_sk(sk)->default_write_space(sk);
> >+
> >       release_sock(sk);
> >       sock_put(sk);
> > }
> >@@ -2019,7 +2031,8 @@ void virtio_transport_recv_pkt_batch(struct virtio_transport *t,
> >                       }
> >
> >                       free_pkt = virtio_transport_recv_pkt_locked(t, skb, sk,
> >-                                                                  &batchable);
> >+                                                                  &batchable,
> >+                                                                  batch);
> >                       if (!batchable)
> >                               virtio_transport_rx_batch_finish(batch);
> >                       if (free_pkt)
> >@@ -2059,23 +2072,31 @@ void virtio_transport_recv_pkt_batch(struct virtio_transport *t,
> >
> >       lock_sock(sk);
> >       /*
> >-       * Sockmap removal restores the native protocol under sk_callback_lock.
> >-       * Serialize this initial eligibility check with that update.
> >+       * Sockmap teardown restores callbacks under sk_callback_lock. Keep the
> >+       * initial eligibility check consistent with that update.
> >        */
>
> Unrelated change?
>

This is related to the new callback eligibility check, but I agree
that the original rewrite was unnecessary. I’ll keep the original
serialization wording and only extend the comment to mention both the
native protocol and callbacks.

> BTW if you are assisted by AI, please read https://docs.kernel.org/process/coding-assistants.html
>

Thanks for the reminder. I read the document when you mentioned it
previously.  take responsibility for every change in it. As this
review has shown, I missed some netdev conventions, and the patch
organization can also be improved; those are my mistakes, and I’ll
address them in v5.

I used an LLM to help set up the test environment, translate and
polish the English text, and review the series. I ran and verified the
tests myself. I’ll
disclose that assistance in v5 as described in the documentation.


> >       read_lock_bh(&sk->sk_callback_lock);
> >       start_batch = virtio_transport_recv_pkt_batchable(t, sk);
> >       read_unlock_bh(&sk->sk_callback_lock);
> >
> >+      if (start_batch)
> >+              batch->sk = sk;
> >+
> >       free_pkt = virtio_transport_recv_pkt_locked(t, skb, sk,
> >-                                                  start_batch ? &batchable : NULL);
> >+                                                  start_batch ? &batchable : NULL,
> >+                                                  start_batch ? batch : NULL);
> >       if (start_batch && batchable) {
> >               /* Keep the lookup reference until the batch is released. */
> >-              batch->sk = sk;
> >               return;
> >       }
> >
> >-      release_sock(sk);
> >-      sock_put(sk);
> >+      if (start_batch) {
> >+              virtio_transport_rx_batch_finish(batch);
> >+      } else {
> >+              release_sock(sk);
> >+              sock_put(sk);
> >+      }
> >+
> >       if (free_pkt)
> >               kfree_skb(skb);
> > }
> >--
> >2.53.0
> >
>