> > On Tue, Nov 03, 2026 at 12:11:34PM +0800, Jia Jia wrote: > >An RX batch can make a socket readable while the RX worker retains its > >socket lock. Waking a blocked reader before release_sock() merely makes it > >wait for the same lock. > > > >P4 coalesces native sk_write_space notifications while retaining their > >under-lock callback context. This patch also coalesces native sk_data_ready > >notifications and moves both callbacks after release_sock(), so awakened > >tasks do not immediately block on the RX worker's socket lock. > > > >Save the native data-ready callback before the AF_VSOCK socket is > >published. Batch only while the native protocol and both native readiness > >callbacks are present and no psock is observed under sk_callback_lock. A > >socket with a replacement callback continues down the existing per-packet > >path. > > > >At batch finish, deliver write-space before data-ready and hold the > >connected lookup reference until both callbacks return. Packets admitted on > >the native path continue to use their saved native callbacks. Loading a > >current sockmap callback at finish would not serialize its invocation > >against sockmap teardown and could lose the native wakeup or invoke a stale > >data-ready wrapper. > > > >Extract the data-ready predicate from vsock_data_ready() so the deferred > >and regular paths share the low-watermark test. > > > >Performance: > > > >P4-P5 contribution (P1-P3 vs P1-P5): > > I'm still confused on the reason why P4-P5 are not squashed. > This patch is essentially changing 90% of the previous patch... >
I think I may have misunderstood how far to carry our v4 discussion into v5. I understood that keeping write-space coalescing under the socket lock as a complete intermediate step, before adding data-ready and moving both callbacks after release_sock(), would keep the two changes separately testable. That is why I kept the same split in v5. In the resulting v5 series, however, P5 changes too much of P4 for that split to remain useful. I will squash P4 and P5. Sorry for the confusion. > > > >End-to-end RX throughput (64K Guest recv() size, SO_RCVLOWAT=1; Gbit/s): > > > > payload P1-P3 RX P1-P5 RX throughput change (95% CI) > > (Gbit/s) (Gbit/s) > > 64B 0.2449 0.2463 +0.55% [-3.86%, +5.17%] > > 256B 0.9649 0.9931 +2.93% [+0.83%, +5.07%] > > 512B 1.8205 1.8260 +0.30% [-4.27%, +5.10%] > > 1K 3.3431 3.3420 -0.03% [-2.15%, +2.12%] > > 4K 5.6541 5.8428 +3.34% [+1.10%, +5.63%] > > 8K 5.4280 5.3364 -1.69% [-15.55%, +14.46%] > > 16K 5.5935 5.6701 +1.37% [-1.41%, +4.23%] > > 64K 5.6917 5.8351 +2.52% [-0.20%, +5.31%] > > > >Fixed-syscall stress test (receiver recv() size = sender write size; > >Gbit/s): > > > > payload P1-P3 RX P1-P5 RX throughput change (95% CI) > > recv() calls are listed as P1-P3/P1-P5. > > 64B 0.2553 0.2575 +0.87% [-3.68%, +5.63%] > > 1048577/1048577 > > 256B 0.9069 0.9183 +1.26% [-3.96%, +6.76%] > > 1048577/1048577 > > 512B 1.6048 1.6459 +2.56% [+0.98%, +4.18%] > > 1048577/1048577 > > 1K 2.6939 2.7357 +1.55% [-1.35%, +4.54%] > > 1048577/1048577 > > 4K 4.9168 4.9678 +1.04% [-1.84%, +4.00%] > > 524965/525283 > > 8K 5.1476 5.2005 +1.03% [-4.02%, +6.34%] > > 262926/263156 > > 16K 5.5964 5.6609 +1.15% [-2.86%, +5.33%] > > 131311/131475 > > 64K 5.9439 5.9073 -0.62% [-3.00%, +1.83%] > > 32875/32937 > > > >Guest CPU1 aggregate cycles per received byte > >(fixed 64K receiver, with perf): > > > > payload P1-P3 P1-P5 change (95% CI) > > 64B 60.281 58.258 -3.36% [-8.74%, +2.35%] > > 256B 14.242 13.648 -4.17% [-9.81%, +1.82%] > > 512B 7.547 7.211 -4.45% [-9.73%, +1.13%] > > 1K 3.987 3.853 -3.35% [-8.52%, +2.12%] > > 4K 1.983 1.880 -5.17% [-7.97%, -2.27%] > > 8K 1.665 1.620 -2.75% [-9.03%, +3.96%] > > 16K 1.409 1.346 -4.52% [-10.94%, +2.37%] > > 64K 1.322 1.281 -3.11% [-6.49%, +0.38%] > > > >Full-series cycles/B (baseline vs P1-P5): > > > >Guest CPU1 aggregate cycles per received byte > >(fixed 64K receiver, with perf): > > > > payload baseline P1-P5 change (95% CI) > > 64B 65.282 57.843 -11.40% [-13.77%, -8.95%] > > 256B 14.710 13.813 -6.10% [ -7.89%, -4.28%] > > 512B 7.709 7.188 -6.75% [ -8.79%, -4.66%] > > 1K 4.075 3.862 -5.23% [ -6.80%, -3.63%] > > 4K 2.034 1.887 -7.21% [ -8.67%, -5.72%] > > 8K 1.664 1.568 -5.78% [ -7.83%, -3.69%] > > 16K 1.450 1.385 -4.48% [ -6.08%, -2.86%] > > 64K 1.338 1.317 -1.51% [ -3.21%, +0.22%] > > > >Signed-off-by: Jia Jia <[email protected]> > >--- > > include/linux/virtio_vsock.h | 5 ++- > > include/net/af_vsock.h | 9 +++-- > > net/vmw_vsock/af_vsock.c | 27 ++++++++++--- > > net/vmw_vsock/virtio_transport_common.c | 52 +++++++++++++++---------- > > 4 files changed, 62 insertions(+), 31 deletions(-) > > > >Tested: > > Concurrent map-entry attach/detach during sequenced RX and duplex traffic > > passed on baseline, P1-P3, P1-P4, and P1-P5. The active SK_PASS case also > > passed on each state, with 200,000 verdict invocations. The concurrent > > SK_PASS attach/detach case completed 1,000 insert/remove pairs per state; > > its 10,000-record suffix after the final detach was verified. No kernel > > diagnostic markers were found in successful runs. > > > > Sockmap may replace sk_data_ready and sk_write_space. With non-native > > callbacks, native RX batching must be declined and the ordinary per-packet > > receive path used. The active SK_PASS case covers the condition expected to > > trigger this fallback. The test does not trace each packet's internal > > branch, so it verifies the fallback scenario without asserting that every > > packet took that branch. > > > > The duplex cases exercised write-space backpressure: the guest's > > nonblocking > > send reached EAGAIN while the host was not reading. After the host began > > draining data, the guest observed POLLOUT and resumed sending. Counts below > > are observed EAGAIN/POLLOUT recoveries. > > > > Test / result baseline P1-P3 P1-P4 > > P1-P5 > > No-verdict RX churn: attach/detach pairs 433493 455507 430985 > > 380351 > > No-verdict duplex: attach/detach pairs 229793 212410 214198 > > 148755 > > No-verdict duplex: EAGAIN/POLLOUT 365/365 266/266 264/264 > > 443/443 > > Active SK_PASS: EAGAIN/POLLOUT 2548/2548 3013/3013 > > 3093/3093 4208/4208 > > SK_PASS attach/detach: intervals with verdict progress 705 616 631 692 > > SK_PASS attach/detach: verdict invocations 99747 99894 97475 82409 > > Post-detach SK_PASS suffix 10,000/10,000 records per run > > > >diff --git a/include/linux/virtio_vsock.h b/include/linux/virtio_vsock.h > >index bf28e8e32e280..2aa0e81e655d5 100644 > >--- a/include/linux/virtio_vsock.h > >+++ b/include/linux/virtio_vsock.h > >@@ -287,14 +287,15 @@ void virtio_transport_recv_pkt(struct virtio_transport > >*t, > > * Initialize a batch to zero before use. A non-empty batch owns @sk's > > socket > > * lock and the reference returned by vsock_find_connected_socket_net(). The > > * caller retains both until it calls virtio_transport_rx_batch_finish(). An > >- * empty batch owns neither. A pending native write-space notification > >belongs > >- * to the lock holder and is delivered by > >virtio_transport_rx_batch_finish(). > >+ * empty batch owns neither. Pending native readiness notifications belong > >to > >+ * the lock holder and are delivered by virtio_transport_rx_batch_finish(). > > */ > > struct virtio_transport_rx_batch { > > struct sock *sk; > > unsigned int pkts; > > size_t bytes; > > bool write_space_pending; > >+ bool data_ready_pending; > > }; > > > > static inline bool > >diff --git a/include/net/af_vsock.h b/include/net/af_vsock.h > >index b76ab565ea58b..57ee545fdb496 100644 > >--- a/include/net/af_vsock.h > >+++ b/include/net/af_vsock.h > >@@ -64,9 +64,10 @@ struct vsock_sock { > > bool sent_request; > > bool ignore_connecting_rst; > > /* > >- * Captured before the socket is published. This identifies AF_VSOCK's > >- * native write-space callback after a consumer has replaced it. > >+ * Captured before the socket is published. These identify AF_VSOCK's > >+ * native readiness callbacks after a consumer has replaced them. > > */ > >+ void (*native_data_ready)(struct sock *sk); > > void (*native_write_space)(struct sock *sk); > > > > /* Protected by lock_sock(sk) */ > >@@ -83,7 +84,9 @@ s64 vsock_stream_has_data(struct vsock_sock *vsk); > > s64 vsock_stream_has_space(struct vsock_sock *vsk); > > struct sock *vsock_create_connected(struct sock *parent); > > void vsock_data_ready(struct sock *sk); > >-bool vsock_write_space_is_native(const struct sock *sk); > >+bool vsock_data_ready_pending(struct sock *sk); > >+bool vsock_callbacks_are_native(const struct sock *sk); > >+void vsock_call_native_data_ready(struct sock *sk); > > void vsock_call_native_write_space(struct sock *sk); > > > > /**** TRANSPORT ****/ > >diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c > >index e39567990539f..0617073e99ed8 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->native_data_ready = sk->sk_data_ready; > > vsk->native_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); > >@@ -1113,24 +1114,38 @@ s64 vsock_stream_has_space(struct vsock_sock *vsk) > > } > > EXPORT_SYMBOL_GPL(vsock_stream_has_space); > > > >-void vsock_data_ready(struct sock *sk) > >+bool vsock_data_ready_pending(struct sock *sk) > > { > > struct vsock_sock *vsk = vsock_sk(sk); > > > >- if (vsock_stream_has_data(vsk) >= sk->sk_rcvlowat || > >- sock_flag(sk, SOCK_DONE)) > >+ return vsock_stream_has_data(vsk) >= sk->sk_rcvlowat || > >+ sock_flag(sk, SOCK_DONE); > >+} > >+EXPORT_SYMBOL_GPL(vsock_data_ready_pending); > >+ > >+void vsock_data_ready(struct sock *sk) > >+{ > >+ if (vsock_data_ready_pending(sk)) > > sk->sk_data_ready(sk); > > } > > EXPORT_SYMBOL_GPL(vsock_data_ready); > > > >-bool vsock_write_space_is_native(const struct sock *sk) > >+bool vsock_callbacks_are_native(const struct sock *sk) > > { > > const struct vsock_sock *vsk = vsock_sk(sk); > > > >- return READ_ONCE(sk->sk_write_space) == > >+ return READ_ONCE(sk->sk_data_ready) == > >+ READ_ONCE(vsk->native_data_ready) && > >+ READ_ONCE(sk->sk_write_space) == > > READ_ONCE(vsk->native_write_space); > > } > >-EXPORT_SYMBOL_GPL(vsock_write_space_is_native); > >+EXPORT_SYMBOL_GPL(vsock_callbacks_are_native); > >+ > >+void vsock_call_native_data_ready(struct sock *sk) > >+{ > >+ vsock_sk(sk)->native_data_ready(sk); > >+} > >+EXPORT_SYMBOL_GPL(vsock_call_native_data_ready); > > > > void vsock_call_native_write_space(struct sock *sk) > > { > >diff --git a/net/vmw_vsock/virtio_transport_common.c > >b/net/vmw_vsock/virtio_transport_common.c > >index af26344678e41..9d02e81042c21 100644 > >--- a/net/vmw_vsock/virtio_transport_common.c > >+++ b/net/vmw_vsock/virtio_transport_common.c > >@@ -1598,7 +1598,8 @@ virtio_transport_recv_enqueue(struct vsock_sock *vsk, > > > > static int > > virtio_transport_recv_connected(struct sock *sk, > >- struct sk_buff *skb) > >+ struct sk_buff *skb, > >+ bool *data_ready_pending) > > { > > struct virtio_vsock_hdr *hdr = virtio_vsock_hdr(skb); > > struct vsock_sock *vsk = vsock_sk(sk); > >@@ -1617,7 +1618,12 @@ virtio_transport_recv_connected(struct sock *sk, > > vsock_remove_sock(vsk); > > break; > > } > >- vsock_data_ready(sk); > >+ if (data_ready_pending) { > >+ if (!*data_ready_pending && > >vsock_data_ready_pending(sk)) > >+ *data_ready_pending = true; > >+ } else { > >+ vsock_data_ready(sk); > >+ } > > return err; > > case VIRTIO_VSOCK_OP_CREDIT_REQUEST: > > virtio_transport_send_credit_update(vsk); > >@@ -1874,20 +1880,19 @@ __virtio_transport_rx_batch_can_receive(struct > >virtio_transport *t, > > return false; > > > > /* > >- * Sockmap can replace sk_prot or sk_write_space, and publish a psock, > >- * without the socket lock. Coalescing is safe only for the native > >- * write-space callback, so a boundary that sees a replacement > >returns to > >- * the existing per-packet path. > >+ * Sockmap can replace sk_prot or either readiness callback, and > >publish a > >+ * psock, without the socket lock. Coalescing is safe only for native > >+ * callbacks, so a boundary that sees a replacement returns to the > >existing > >+ * per-packet path. > > */ > > return READ_ONCE(sk->sk_prot) == sk->sk_prot_creator && > >- vsock_write_space_is_native(sk) && > >+ vsock_callbacks_are_native(sk) && > > !virtio_transport_sock_has_psock(sk); > > } > > > > /* > >- * Sockmap teardown restores the native protocol and write-space callback > >- * under sk_callback_lock. Serialize each packet admission check with those > >- * updates. > >+ * Sockmap teardown restores the native protocol and callbacks under > >+ * sk_callback_lock. Serialize each packet admission check with those > >updates. > > */ > > static bool > > virtio_transport_rx_batch_can_receive(struct virtio_transport *t, > >@@ -1905,9 +1910,8 @@ virtio_transport_rx_batch_can_receive(struct > >virtio_transport *t, > > /* > > * The caller holds sk's socket lock. If @batchable is non-NULL, update it > > * after processing @skb to say whether a native STREAM/RW batch can > > continue. > >- * When @batch is non-NULL, record a native write-space notification for > >batch > >- * finish. A writer cannot consume the updated credit until this lock is > >- * released. > >+ * When @batch is non-NULL, record native readiness notifications for batch > >+ * finish. A reader or writer cannot make progress until this lock is > >released. > > */ > > static void > > virtio_transport_recv_pkt_locked(struct virtio_transport *t, > >@@ -1917,6 +1921,7 @@ virtio_transport_recv_pkt_locked(struct > >virtio_transport *t, > > { > > struct virtio_vsock_hdr *hdr = virtio_vsock_hdr(skb); > > struct vsock_sock *vsk = vsock_sk(sk); > >+ bool *data_ready_pending; > > struct sockaddr_vm src; > > bool space_available; > > > >@@ -1926,6 +1931,8 @@ virtio_transport_recv_pkt_locked(struct > >virtio_transport *t, > > if (batchable) > > *batchable = false; > > > >+ data_ready_pending = batch ? &batch->data_ready_pending : NULL; > > Can we just pass `batch` to virtio_transport_recv_connected and avoid > all of this complexity? > Sure, understood. > >+ > > /* > > * The socket may have been closed or reassigned after lookup but > > before > > * its socket lock was acquired. Listener sockets have no assigned > >@@ -1962,7 +1969,7 @@ virtio_transport_recv_pkt_locked(struct > >virtio_transport *t, > > kfree_skb(skb); > > break; > > case TCP_ESTABLISHED: > >- virtio_transport_recv_connected(sk, skb); > >+ virtio_transport_recv_connected(sk, skb, data_ready_pending); > > break; > > case TCP_CLOSING: > > virtio_transport_recv_disconnecting(sk, skb); > >@@ -2036,32 +2043,37 @@ EXPORT_SYMBOL_GPL(virtio_transport_recv_pkt); > > > > /* > > * Release an active RX batch. A non-empty batch holds the socket lock and > > the > >- * reference returned by vsock_find_connected_socket_net(). Deliver any > >- * deferred native write-space notification while holding the lock, > >preserving > >- * the callback context of the regular receive path. An empty batch holds > >- * neither. > >+ * reference returned by vsock_find_connected_socket_net(). Release the > >lock, > >+ * deliver deferred native write-space and data-ready notifications in that > >+ * order, then drop the reference. An empty batch holds neither. > > */ > > Really, avoid comments that just describe what a function is doing. They > really don't add much value and we need to update them every time, like > this... > > Comments should explain reasons, hidden behaviours, etc. > Something unclear just looking at the code. > Understood, thanks. I have not been doing this well. I will be more careful. > > void virtio_transport_rx_batch_finish(struct virtio_transport_rx_batch > > *batch) > > { > > bool write_space_pending = batch->write_space_pending; > >+ bool data_ready_pending = batch->data_ready_pending; > > struct sock *sk = batch->sk; > > > > batch->sk = NULL; > > batch->pkts = 0; > > batch->bytes = 0; > > batch->write_space_pending = false; > >+ batch->data_ready_pending = false; > > > > if (!sk) > > return; > > > > Here should be nice to have a comment to explain WHY it's safe to > release in this new position... > Sure. I will add a comment there. > >+ release_sock(sk); > >+ > > /* > >- * The packet was admitted with the native callback. Do not reload a > >current > >+ * Packets were admitted with native callbacks. Do not reload a > >current > > Why this change? > I only changed the wording because a batch can hold more than one packet and both callbacks are now deferred. > Sorry, this series is really hard to review :-( > Sorry again. This is related to my misunderstanding. > > * callback here: sockmap can replace or remove its wrapper > > independently. > > */ > > if (write_space_pending) > > vsock_call_native_write_space(sk); > > > >- release_sock(sk); > >+ if (data_ready_pending) > >+ vsock_call_native_data_ready(sk); > >+ > > sock_put(sk); > > } > > EXPORT_SYMBOL_GPL(virtio_transport_rx_batch_finish); > >-- > >2.53.0 > > >
