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 <[email protected]>
> ---
> 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
