Re: [PATCH net-next v4 4/4] vsock/virtio: defer RX notifications until batch unlock

[Stefano Garzarella <sgarzare@redhat.com>]

On Sun, Oct 18, 2026 at 12:55:35PM +0800, Jia Jia wrote:
>>
>> On Wed, Oct 14, 2026 at 07:56:40PM +0800, Jia Jia wrote:
>> >An RX batch can make a socket readable while the worker still holds its
>> >socket lock. Waking a blocking reader then only moves it to waiting for
>> >the same lock.
>> >
>> >Record the existing low-watermark condition and notify once after
>> >release_sock(). Move the coalesced write-space notification after the
>> >unlock as well, since both callbacks wake sk_sleep(sk). Deliver
>> >write-space before data-ready and keep the lookup reference until both
>> >callbacks complete.
>> >
>> >As with write-space, save the initial sk_data_ready callback and batch only
>> >while both saved callbacks remain installed. Sockets with replaced
>> >callbacks retain their per-packet behavior.
>> >
>> >The existing psock-marker check also covers a concurrent sockmap attach:
>> >at most the already admitted packet completes on the native path, and the
>> >next packet ends the batch.
>> >
>> >Signed-off-by: Jia Jia <physicalmtea@gmail.com>
>> >---
>> > 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 | 35 ++++++++++++++++++-------
>> > 4 files changed, 31 insertions(+), 11 deletions(-)
>> >
>> >diff --git a/include/linux/virtio_vsock.h b/include/linux/virtio_vsock.h
>> >index bf077ee0ded88..f1396ca3ad9b5 100644
>> >--- a/include/linux/virtio_vsock.h
>> >+++ b/include/linux/virtio_vsock.h
>> >@@ -288,6 +288,7 @@ struct virtio_transport_rx_batch {
>> >       unsigned int pkts;
>> >       size_t bytes;
>> >       bool write_space_pending;
>> >+      bool data_ready_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 9a40960d10cbe..78bab5820f528 100644
>> >--- a/include/net/af_vsock.h
>> >+++ b/include/net/af_vsock.h
>> >@@ -64,9 +64,10 @@ struct vsock_sock {
>> >       bool sent_request;
>> >       bool ignore_connecting_rst;
>> >       /*
>> >-       * Initial callback, used to identify replacements and deliver deferred
>> >-       * write-space notifications.
>> >+       * Initial callbacks, used to identify replacements and deliver deferred
>> >+       * notifications.
>> >        */
>> >+      void (*default_data_ready)(struct sock *sk);
>> >       void (*default_write_space)(struct sock *sk);
>> >
>> >       /* Protected by lock_sock(sk) */
>> >diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
>> >index fe7f49da6c4bc..f40659ec29df0 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_data_ready = sk->sk_data_ready;
>> >       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 f4b5f0230493e..8b62b789b71c5 100644
>> >--- a/net/vmw_vsock/virtio_transport_common.c
>> >+++ b/net/vmw_vsock/virtio_transport_common.c
>> >@@ -1588,7 +1588,8 @@ virtio_transport_recv_enqueue(struct vsock_sock *vsk,
>> >
>> > static int
>> > virtio_transport_recv_connected(struct sock *sk,
>> >-                              struct sk_buff *skb)
>> >+                              struct sk_buff *skb,
>> >+                              bool *data_ready_pending)
>> > {
>> >       struct virtio_vsock_hdr *hdr = virtio_vsock_hdr(skb);
>> >       struct vsock_sock *vsk = vsock_sk(sk);
>> >@@ -1607,7 +1608,13 @@ virtio_transport_recv_connected(struct sock *sk,
>> >                       vsock_remove_sock(vsk);
>> >                       break;
>> >               }
>> >-              vsock_data_ready(sk);
>> >+              if (!data_ready_pending) {
>> >+                      vsock_data_ready(sk);
>> >+              } else if (!*data_ready_pending &&
>> >+                         (vsock_stream_has_data(vsk) >= sk->sk_rcvlowat ||
>> >+                          sock_flag(sk, SOCK_DONE))) {
>> >+                      *data_ready_pending = true;
>> >+              }
>> >               return err;
>> >       case VIRTIO_VSOCK_OP_CREDIT_REQUEST:
>> >               virtio_transport_send_credit_update(vsk);
>> >@@ -1820,6 +1827,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_data_ready) == vsk->default_data_ready &&
>> >              READ_ONCE(sk->sk_write_space) == vsk->default_write_space &&
>> >              !sock_flag(sk, SOCK_DONE) &&
>> >              vsk->transport == &t->transport &&
>> >@@ -1830,8 +1838,9 @@ 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.  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.
>> >+ * record default write-space and data-ready notifications for batch finish
>> >+ * instead of delivering them immediately.  Return true if the caller must
>> >+ * free @skb.
>> >  */
>> > static bool
>> > virtio_transport_recv_pkt_locked(struct virtio_transport *t,
>> >@@ -1883,7 +1892,9 @@ virtio_transport_recv_pkt_locked(struct virtio_transport *t,
>> >               kfree_skb(skb);
>> >               break;
>> >       case TCP_ESTABLISHED:
>> >-              virtio_transport_recv_connected(sk, skb);
>> >+              virtio_transport_recv_connected(sk, skb,
>> >+                                              batch ?
>> >+                                              &batch->data_ready_pending : NULL);
>> >               break;
>> >       case TCP_CLOSING:
>> >               virtio_transport_recv_disconnecting(sk, skb);
>> >@@ -1957,27 +1968,33 @@ EXPORT_SYMBOL_GPL(virtio_transport_recv_pkt);
>> > /*
>> >  * Finish the RX batch.
>> >  * 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.
>> >+ * lock_sock(). Release the lock, deliver any deferred write-space and
>> >+ * data-ready notifications in that order, then drop 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;
>> >+      bool data_ready_pending = batch->data_ready_pending;
>> >       struct sock *sk = batch->sk;
>> >
>> >       batch->sk = NULL;
>> >       batch->pkts = 0;
>> >       batch->bytes = 0;
>> >       batch->write_space_pending = false;
>> >+      batch->data_ready_pending = false;
>> >
>> >       if (!sk)
>> >               return;
>> >
>> >+      release_sock(sk);
>> >+
>>
>> Why in the previous patch we release the socket after calling
>> default_write_space() and now we are moving before?
>>
>
>My intention was to keep the two behavioral changes separately
>testable. Patch 3 is a complete intermediate state: it coalesces
>repeated sk_write_space()
>notifications while preserving the existing under-lock callback
>context. Patch 4 then adds data-ready coalescing and moves both
>deferred notifications
>after release_sock(). Patch 4 therefore builds on Patch 3 rather than fixing it.

Okay, I see, makes sense, please make it clear in the commit 
description.

Thanks,
Stefano

>
>> >       if (write_space_pending)
>> >               vsock_sk(sk)->default_write_space(sk);
>> >
>> >-      release_sock(sk);
>> >+      if (data_ready_pending)
>> >+              vsock_sk(sk)->default_data_ready(sk);
>> >+
>> >       sock_put(sk);
>> > }
>> > EXPORT_SYMBOL_GPL(virtio_transport_rx_batch_finish);
>> >--
>> >2.53.0
>> >
>>
>