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

[Stefano Garzarella <sgarzare@redhat.com>]

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?

>
>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 | 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?

>
> 	/* 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?

> 	       !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?

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

> 	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
>