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

[Bobby Eshleman <bobbyeshleman@gmail.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.
> 
> 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);
>  
>  	/* 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 &&
>  	       !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);

Any reason not to just do READ_ONCE(sk->sk_write_space)(sk) instead of
storing this for later? If we setup the socket in a sockmap program
mid-batch, doesn't this and sk_data_ready (from the next patch) go
stale?

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

Why reword here?

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

Seems like `batch->sk = sk` is moving around between patches?

I don't know, feels like if the batching patch introduces
sk_write_space/sk_data_ready bugs, then that patch should probably
contain these patches too. Should minimize churn too?


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

Best,
Bobby