>
> 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?
>
Thanks for the review, I tried loading the current callback in an
earlier version, but READ_ONCE() does not synchronize the load and
invocation with sockmap attach or teardown.
For example, it could load sk_psock_write_space, then invoke it after
the psock has been removed, in which case the wrapper would no longer
forward the
native wakeup. The sk_data_ready case is more serious: the stale
sockmap wrapper can dequeue a native skb and then drop it when its
actor finds no psock.
There is also an attach-side semantic issue: one deferred data-ready
notification may represent several native skbs, while one invocation
of the sockmap
callback consumes only one skb. The saved callbacks are therefore
intentional and represent the native path at packet admission, rather
than the callback
installed at batch finish. A packet boundary that observes a callback,
protocol, or psock change terminates the batch. I’ll document this
admission-time
rule more clearly and add sockmap attach/detach stress coverage.
> > +
> > 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?
>
>
My intention was to structure the series in two stages. Patches 1 and
2 form a complete batching change while retaining the existing
per-packet
notification behavior. Patches 3 and 4 then add notification
coalescing and after-unlock delivery. Therefore, Patch 3 is not a
correctness fix for Patch
2; the series through Patch 2 remains functional on its own.
The batch->sk assignment moves because Patch 3 introduces pending
notification state. Once the first packet can record a deferred
notification, the batch
must record its socket before processing that packet, even if the
packet subsequently ends the batch.
I agree, however, that this split causes avoidable churn.
> > 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
