On Thu, 13 Feb 2025 at 10:25, Stefano Garzarella <sgarzare@xxxxxxxxxx> wrote: > > On Thu, Feb 13, 2025 at 09:28:05AM +0800, Junnan Wu wrote: > >>You need to update the title now that you're moving also queued_replies. > >> > > > >Well, I will update the title in V3 version. > > > >>On Tue, Feb 11, 2025 at 03:19:21PM +0800, Junnan Wu wrote: > >>>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! > > > > >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
