Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.
Low priority / nit picks for your consideration:
- [Low] The patch is presented purely as a performance optimisation
("amortize RX socket locking") but gives no throughput, latency or CPU…
- [Low] Removing the local skb_overhead declaration from
virtio_transport_inc_rx_pkt()…
--
vsock/virtio: amortize RX socket locking for stream packets
This commit keeps the socket lock held in the virtio-vsock RX worker
while it processes a bounded run of RW packets for the same established
STREAM socket. A batch is limited to 64 packets or 64KB of payload. The
commit adds the exported helpers virtio_transport_recv_pkt_batch() and
virtio_transport_rx_batch_finish(), and virtio_transport_inc_rx_pkt()
now shares the skb metadata headroom check.
> diff --git a/net/vmw_vsock/virtio_transport.c
> b/net/vmw_vsock/virtio_transport.c
> index 4f9aa9c4c3aa5..ef076732158d4 100644
> --- a/net/vmw_vsock/virtio_transport.c
> +++ b/net/vmw_vsock/virtio_transport.c
> @@ -629,8 +629,16 @@ virtio_transport_seqpacket_allow(struct vsock_sock *vsk,
> u32 remote_cid)
> return seqpacket_allow;
> }
>
> +/*
> + * Keep a bounded run of packets for one socket under a single socket lock.
> + * Limit packet count and payload size to bound the work done while locked.
> + */
> +#define VIRTIO_TRANSPORT_RX_BATCH_MAX_PKTS 64
> +#define VIRTIO_TRANSPORT_RX_BATCH_MAX_BYTES (64 * 1024)
[Severity: Low]
This isn't a bug, but are there throughput, latency or CPU numbers for
this change? Which workload does it help?
The commit is presented only as a performance optimization. To get that
benefit it adds a new struct in include/linux/virtio_vsock.h, two
exported symbols, and new rules for when a packet can join a batch.
It also keeps lock_sock() held across iterations of the loop in
virtio_transport_rx_work(). As a result, recvmsg, sendmsg, setsockopt or
close on the same socket can now wait for up to 64 packets / 64KB
instead of one packet.
In this patch, the batched path in virtio_transport_recv_pkt_batch()
still does the lookup and refcount work for every packet:
sk = virtio_transport_recv_pkt_find_socket(skb, &src, &dst, net);
...
if (!skb_set_owner_sk_safe(skb, sk)) {
...
if (batch->sk == sk) {
/* Keep the batch reference; drop this packet's lookup reference. */
sock_put(sk);
virtio_transport_recv_pkt_find_socket() takes the global
vsock_table_lock and does a sock_hold(). That means only the
lock_sock()/release_sock() pair is saved.
sk_data_ready() also still runs while the lock is held. A reader it
wakes can therefore go back to sleep on sk_lock.wq until the batch
ends.
Two later commits in the series remove those costs: "vsock/virtio:
reuse same-flow socket lookup in RX batches" and "vsock/virtio: defer
RX readable notifications until batch unlock". Even so, no commit
message in the series has numbers showing that the change is a net win.
Could benchmark data be added to the commit message?
[ ... ]
> diff --git a/net/vmw_vsock/virtio_transport_common.c
> b/net/vmw_vsock/virtio_transport_common.c
> index e3e75e59896fe..a73e37b3fa62f 100644
> --- a/net/vmw_vsock/virtio_transport_common.c
> +++ b/net/vmw_vsock/virtio_transport_common.c
> @@ -576,10 +576,16 @@ virtio_transport_collapse_rx_queue(struct
> virtio_vsock_sock *vvs,
[ ... ]
> static bool virtio_transport_inc_rx_pkt(struct virtio_vsock_sock *vvs,
> u32 len)
> {
> - u64 skb_overhead = (skb_queue_len(&vvs->rx_queue) + 1) *
> SKB_TRUESIZE(0);
>
> /* Allow at most buf_alloc * 2 total budget (payload + overhead),
[Severity: Low]
This isn't a bug, but removing skb_overhead leaves a blank line right
after the opening brace of virtio_transport_inc_rx_pkt().
checkpatch --strict warns about this ("Blank lines aren't necessary
after an open brace"). The blank line is still there at the end of the
series.
[ ... ]
--
Sashiko AI review ·
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261010142247.99223-1-physicalmtea%40gmail.com
