On Thu, 13 Feb 2025 at 10:25, Stefano Garzarella [off-list ref] wrote:

On Thu, Feb 13, 2025 at 09:28:05AM +0800, Junnan Wu wrote:

quoted

quoted

You need to update the title now that you're moving also queued_replies.

Well, I will update the title in V3 version.

quoted

On Tue, Feb 11, 2025 at 03:19:21PM +0800, Junnan Wu wrote:

quoted

When executing suspend to ram twice in a row,
the `rx_buf_nr` and `rx_buf_max_nr` increase to three times vq->num_free.
Then after virtqueue_get_buf and `rx_buf_nr` decreased
in function virtio_transport_rx_work,
the condition to fill rx buffer
(rx_buf_nr < rx_buf_max_nr / 2) will never be met.

It is because that `rx_buf_nr` and `rx_buf_max_nr`
are initialized only in virtio_vsock_probe(),
but they should be reset whenever virtqueues are recreated,
like after a suspend/resume.

Move the `rx_buf_nr` and `rx_buf_max_nr` initialization in
virtio_vsock_vqs_init(), so we are sure that they are properly
initialized, every time we initialize the virtqueues, either when we
load the driver or after a suspend/resume.
At the same time, also move `queued_replies`.

Why?

As I mentioned the commit description should explain why the changes are
being made for both reviewers and future references to this patch.

After your kindly remind, I have double checked all locations where `queued_replies`
used, and we think for order to prevent erroneous atomic load operations
on the `queued_replies` in the virtio_transport_send_pkt_work() function
which may disrupt the scheduling of vsock->rx_work
when transmitting reply-required socket packets,
this atomic variable must undergo synchronized initialization
alongside the preceding two variables after a suspend/resume.

Yes, that was my concern!

quoted

If we reach agreement on it, I will add this description in V3 version.

Yes, please, I just wanted to point out that we need to add an
explanation in the commit description.

And in the title, in this case though listing all the variables would
get too long, so you can do something like that:

     vsock/virtio: fix variables initialization during resuming

I forgot to mention that IMHO it's better to split this series.
This first patch (this one) seems ready, without controversy, and it's
a real fix, so for me v3 should be a version ready to be merged.

While the other patch is more controversial and especially not a fix
but more of a new feature, so I don't think it makes sense to continue
to have these two patches in a single series.

Thanks,
Stefano