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 as a pure performance optimization
("amortize RX socket locking for stream packets").
--
vsock/virtio: amortize RX socket locking for stream packets
The virtio-vsock RX worker now keeps the socket lock and the lookup
reference held across a bounded run of STREAM/RW packets for one socket.
A batch is capped at 64 packets or 64KB of payload. It ends on control
packets, a flow change, or a change in socket state or protocol. Two new
exported functions are added, virtio_transport_recv_pkt_batch() and
virtio_transport_rx_batch_finish().
> diff --git a/net/vmw_vsock/virtio_transport_common.c
> b/net/vmw_vsock/virtio_transport_common.c
> index 8a930bc42fdc3..bfe40ecf0b2f0 100644
> --- a/net/vmw_vsock/virtio_transport_common.c
> +++ b/net/vmw_vsock/virtio_transport_common.c
[ ... ]
> @@ -1909,6 +1945,128 @@ void virtio_transport_recv_pkt(struct
> virtio_transport *t,
[ ... ]
> + if (batch->sk) {
> + vsk = vsock_sk(batch->sk);
> + if (vsock_addr_equals_addr(&src, &vsk->remote_addr) &&
> + dst.svm_port == vsk->local_addr.svm_port &&
> + vsock_net_check_mode(sock_net(batch->sk), net) &&
> + virtio_transport_recv_pkt_batchable(t, batch->sk)) {
[ ... ]
> + }
> +
> + virtio_transport_rx_batch_finish(batch);
> + }
> +
> + sk = virtio_transport_find_socket(skb, &src, &dst, net);
[ ... ]
> + lock_sock(sk);
> + /*
> + * Sockmap removal restores the native protocol under sk_callback_lock.
> + * Serialize this initial eligibility check with that update.
> + */
> + read_lock_bh(&sk->sk_callback_lock);
> + start_batch = virtio_transport_recv_pkt_batchable(t, sk);
> + read_unlock_bh(&sk->sk_callback_lock);
> +
> + free_pkt = virtio_transport_recv_pkt_locked(t, skb, sk, &src, &dst,
> + net,
> + start_batch ? &batchable :
> NULL);
> + if (start_batch && batchable) {
> + /* Keep the lookup reference until the batch is released. */
> + batch->sk = sk;
> + return;
> + }
[Severity: Low]
Is there data showing this is a net win? The subject says it will
"amortize RX socket locking for stream packets". The commit message
gives no throughput, latency or CPU-cost numbers.
Take interleaved RW traffic from two or more stream flows (A, B, A, B,
...). The cached-flow check above fails on every packet, so each packet
goes through:
virtio_transport_recv_pkt_batch()
virtio_transport_rx_batch_finish() /* release_sock + sock_put */
virtio_transport_find_socket()
lock_sock()
read_lock_bh(&sk->sk_callback_lock)
virtio_transport_recv_pkt_batchable()
read_unlock_bh(&sk->sk_callback_lock)
virtio_transport_recv_pkt_locked()
virtio_transport_recv_pkt_batchable()
Every step of the old per-packet path is still done. On top of that,
each packet pays for the read_lock_bh()/read_unlock_bh() pair, two
predicate checks, and the batch bookkeeping in
virtio_transport_rx_work(). No batch ever grows past one packet.
The socket lock from the previous flow also stays held while
virtio_transport_rx_work() runs virtqueue_get_buf(), tap delivery and
header parsing for the next packet.
The added cost is small next to lock_sock() and the socket lookup. Could
the commit message include measurements, and mention this
interleaved-flow trade-off?
--
Sashiko AI review ยท
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261013063112.168321-1-physicalmtea%40gmail.com
