Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.
Critical: 0 · High: 0 · Medium: 1 · Low: 0
If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).
- [Medium] The patch defers sock_def_readable() until after
release_sock() so that a reader is not woken while the RX worker still…
--
vsock/virtio: defer RX readable notifications until batch unlock
This commit records when an RX batch makes a socket readable. It then
calls the saved sock_def_readable() callback once, after the batch
releases the socket lock, instead of calling it per packet. The
original sk_data_ready callback is saved in __vsock_create(), and
batching now also requires sk_data_ready to still match it.
> diff --git a/net/vmw_vsock/virtio_transport_common.c
> b/net/vmw_vsock/virtio_transport_common.c
> index d6ebfde880db6..84822d6bfb958 100644
> --- a/net/vmw_vsock/virtio_transport_common.c
> +++ b/net/vmw_vsock/virtio_transport_common.c
[ ... ]
> @@ -1975,6 +1987,10 @@ void virtio_transport_rx_batch_finish(struct
> virtio_transport_rx_batch *batch)
> vsock_sk(sk)->default_write_space(sk);
>
> release_sock(sk);
> +
> + if (data_ready_pending)
> + vsock_sk(sk)->default_data_ready(sk);
> +
[Severity: Medium]
Do blocking recv() readers still get woken while the worker holds the
socket lock?
virtio_transport_rx_batch_finish() still calls default_write_space()
(sock_def_write_space()) before release_sock(). That function wakes
the same wait queue that sk_sleep(sk) returns:
net/core/sock.c:sock_def_write_space() {
...
if (skwq_has_sleeper(wq))
wake_up_interruptible_sync_poll(&wq->wait, EPOLLOUT |
EPOLLWRNORM | EPOLLWRBAND);
...
}
A blocking stream reader uses DEFINE_WAIT(wait) in
__vsock_stream_recvmsg(). It sleeps on that queue through
vsock_connectible_wait_data():
prepare_to_wait(sk_sleep(sk), wait, TASK_INTERRUPTIBLE);
DEFINE_WAIT() uses autoremove_wake_function(), which ignores the
EPOLLOUT key. So the EPOLLOUT wakeup also wakes the reader and removes
it from the queue.
On a socket that mostly receives, tx_cnt - peer_fwd_cnt is about zero.
That means virtio_transport_space_update() reports free credit for
nearly every batch, and virtio_transport_recv_pkt_locked() sets
batch->write_space_pending:
if (space_available) {
if (batch)
batch->write_space_pending = true;
else
sk->sk_write_space(sk);
}
In that case the reader wakes under the lock, calls lock_sock(), and
waits on sk_lock.wq until release_sock(). When the deferred
default_data_ready() runs, no sleeper is left. This looks like the
situation the commit message says the patch avoids:
"Waking a reader at that point can make it immediately wait
for the same lock."
It seems only poll/epoll waiters that filter on EPOLLIN alone would
benefit from the deferral.
The previous patch in the series ("vsock/virtio: coalesce RX
write-space notifications in lock batches") argues that a writer
cannot use new credit until the lock is released. Could the
default_write_space() call move after release_sock() as well? If not,
could the commit message say why write-space stays under the lock and
that blocking readers are not covered?
> sock_put(sk);
> }
> EXPORT_SYMBOL_GPL(virtio_transport_rx_batch_finish);
--
Sashiko AI review ·
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261013063112.168321-1-physicalmtea%40gmail.com
