> > 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? >
No. Patch 2 preserves the existing per-packet sk_write_space() behavior, so Patch 3 is not a correctness fix for it. Patch 3 only coalesces the native/ default notifications. Moving batch->sk earlier is required only after the deferred state is introduced here: the socket must already be recorded before the first packet can set write_space_pending, so the notification can still be delivered if that packet immediately ends the batch. Sorry, the commit message did not make this distinction clear. > > > >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); > > This is kind of virtio-vsock specific, why need to be here? > I stored this state here because __vsock_create() can capture the native callback before the socket is exposed or any callback replacement occurs, and the transport cannot directly reference the unexported default callback. However, I agree that exposing this virtio-specific state directly in struct vsock_sock is not ideal. I'll look at hiding it behind an AF_VSOCK helper in v5 while preserving the capture point and the lifetime required by deferred callbacks. > > > > /* 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? > I believe this check is necessary because notification coalescing is only safe for the original native sk_write_space callback. Sockmap can replace this callback under sk_callback_lock, which is independent of the socket lock held across the batch, and the replacement may require per-packet handling. Comparing it with the callback captured at socket creation lets us fall back to the existing per-packet behavior when it has been replaced. I'll add a comment explaining this. > > !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? > This is related to the new callback eligibility check, but I agree that the original rewrite was unnecessary. I’ll keep the original serialization wording and only extend the comment to mention both the native protocol and callbacks. > BTW if you are assisted by AI, please read > https://docs.kernel.org/process/coding-assistants.html > Thanks for the reminder. I read the document when you mentioned it previously. take responsibility for every change in it. As this review has shown, I missed some netdev conventions, and the patch organization can also be improved; those are my mistakes, and I’ll address them in v5. I used an LLM to help set up the test environment, translate and polish the English text, and review the series. I ran and verified the tests myself. I’ll disclose that assistance in v5 as described in the documentation. > > 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 > > >
