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] Commit message concern: the message for "vsock/virtio: split
socket lookup from locked RX processing" lists what the refactor does…
--
vsock/virtio: split socket lookup from locked RX processing
This commit splits virtio_transport_recv_pkt() into two helpers. One
does the socket lookup. The other runs the receive state machine under
the socket lock. The decoded source and destination addresses and the
network namespace reach the locked helper through a new struct
virtio_transport_rx_pkt_ctx.
> Keep virtio_transport_recv_pkt() on its existing per-packet locking path.
> Pass the source and destination addresses used for lookup to the locked
> receive path so they are decoded only once.
[Severity: Low]
This isn't a bug, but does "decoded only once" describe this patch on
its own? The old virtio_transport_recv_pkt() also decoded src and dst
once with vsock_addr_init(). It then reused them for the lookup and for
vsock_check_source().
> Preserve source validation and failure handling. Keep the lookup reference
> separate from the skb owner reference so the locked receive path can manage
> the two lifetimes independently.
In this patch, virtio_transport_recv_pkt_locked() doesn't touch either
reference. The caller still calls skb_set_owner_sk_safe(),
release_sock(), sock_put() and kfree_skb() in the same order as before:
free_pkt = virtio_transport_recv_pkt_locked(t, skb, sk, &ctx);
release_sock(sk);
...
sock_put(sk);
if (free_pkt)
kfree_skb(skb);
Also, struct virtio_transport_rx_pkt_ctx has only one user here.
These statements seem to describe the next patch in the series,
"vsock/virtio: amortize RX socket locking for stream packets". That
patch adds virtio_transport_recv_pkt_batch() as a second caller of
virtio_transport_recv_pkt_find_socket() and
virtio_transport_recv_pkt_locked().
It also adds batchable, batch and defer_data_ready to the ctx struct.
It keeps the lookup reference for the life of the batch and takes the
skb owner reference per packet.
The diff reorders one thing: on a type mismatch, sock_put() now runs
before virtio_transport_reset_no_sock(). This has no effect, because
virtio_transport_reset_no_sock() uses skb->sk and not the looked-up sk.
Could the commit message say that this is preparation for the batched
RX work, and add a "No functional change intended" line?
--
Sashiko AI review ·
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261010142247.99223-1-physicalmtea%40gmail.com
