| Message ID | 20250214012200.1883896-1-junnan01.wu@samsung.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 55eff109e76a14e5ed10c8c3c3978d20a35e2a4d |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v3] vsock/virtio: fix variables initialization during resuming | expand |
On Fri, Feb 14, 2025 at 09:22:00AM +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. > >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. > >Fixes: bd50c5dc182b ("vsock/virtio: add support for device suspend/resume") >Link: https://lore.kernel.org/virtualization/20250207052033.2222629-1-junnan01.wu@samsung.com/ >Co-developed-by: Ying Gao <ying01.gao@samsung.com> >Signed-off-by: Ying Gao <ying01.gao@samsung.com> >Signed-off-by: Junnan Wu <junnan01.wu@samsung.com> >--- > net/vmw_vsock/virtio_transport.c | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > >diff --git a/net/vmw_vsock/virtio_transport.c b/net/vmw_vsock/virtio_transport.c >index b58c3818f284..f0e48e6911fc 100644 >--- a/net/vmw_vsock/virtio_transport.c >+++ b/net/vmw_vsock/virtio_transport.c >@@ -670,6 +670,13 @@ static int virtio_vsock_vqs_init(struct virtio_vsock *vsock) > }; > int ret; > >+ mutex_lock(&vsock->rx_lock); >+ vsock->rx_buf_nr = 0; >+ vsock->rx_buf_max_nr = 0; >+ mutex_unlock(&vsock->rx_lock); >+ >+ atomic_set(&vsock->queued_replies, 0); >+ > ret = virtio_find_vqs(vdev, VSOCK_VQ_MAX, vsock->vqs, vqs_info, NULL); > if (ret < 0) > return ret; >@@ -779,9 +786,6 @@ static int virtio_vsock_probe(struct virtio_device *vdev) > > vsock->vdev = vdev; > >- vsock->rx_buf_nr = 0; >- vsock->rx_buf_max_nr = 0; >- atomic_set(&vsock->queued_replies, 0); > > mutex_init(&vsock->tx_lock); > mutex_init(&vsock->rx_lock); >-- >2.34.1 > Reviewed-by: Luigi Leonardi <leonardi@redhat.com>
On Fri, Feb 14, 2025 at 09:22:00AM +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. > > 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. > > Fixes: bd50c5dc182b ("vsock/virtio: add support for device suspend/resume") > Link: https://lore.kernel.org/virtualization/20250207052033.2222629-1-junnan01.wu@samsung.com/ > Co-developed-by: Ying Gao <ying01.gao@samsung.com> > Signed-off-by: Ying Gao <ying01.gao@samsung.com> > Signed-off-by: Junnan Wu <junnan01.wu@samsung.com> Acked-by: Michael S. Tsirkin <mst@redhat.com> > --- > net/vmw_vsock/virtio_transport.c | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/net/vmw_vsock/virtio_transport.c b/net/vmw_vsock/virtio_transport.c > index b58c3818f284..f0e48e6911fc 100644 > --- a/net/vmw_vsock/virtio_transport.c > +++ b/net/vmw_vsock/virtio_transport.c > @@ -670,6 +670,13 @@ static int virtio_vsock_vqs_init(struct virtio_vsock *vsock) > }; > int ret; > > + mutex_lock(&vsock->rx_lock); > + vsock->rx_buf_nr = 0; > + vsock->rx_buf_max_nr = 0; > + mutex_unlock(&vsock->rx_lock); > + > + atomic_set(&vsock->queued_replies, 0); > + > ret = virtio_find_vqs(vdev, VSOCK_VQ_MAX, vsock->vqs, vqs_info, NULL); > if (ret < 0) > return ret; > @@ -779,9 +786,6 @@ static int virtio_vsock_probe(struct virtio_device *vdev) > > vsock->vdev = vdev; > > - vsock->rx_buf_nr = 0; > - vsock->rx_buf_max_nr = 0; > - atomic_set(&vsock->queued_replies, 0); > > mutex_init(&vsock->tx_lock); > mutex_init(&vsock->rx_lock); > -- > 2.34.1
On Fri, Feb 14, 2025 at 09:22:00AM +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. > >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. > >Fixes: bd50c5dc182b ("vsock/virtio: add support for device suspend/resume") >Link: https://lore.kernel.org/virtualization/20250207052033.2222629-1-junnan01.wu@samsung.com/ >Co-developed-by: Ying Gao <ying01.gao@samsung.com> >Signed-off-by: Ying Gao <ying01.gao@samsung.com> >Signed-off-by: Junnan Wu <junnan01.wu@samsung.com> >--- > net/vmw_vsock/virtio_transport.c | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
Hello: This patch was applied to netdev/net.git (main) by Jakub Kicinski <kuba@kernel.org>: On Fri, 14 Feb 2025 09:22:00 +0800 you 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. > > [...] Here is the summary with links: - [net,v3] vsock/virtio: fix variables initialization during resuming https://git.kernel.org/netdev/net/c/55eff109e76a You are awesome, thank you!
diff --git a/net/vmw_vsock/virtio_transport.c b/net/vmw_vsock/virtio_transport.c index b58c3818f284..f0e48e6911fc 100644 --- a/net/vmw_vsock/virtio_transport.c +++ b/net/vmw_vsock/virtio_transport.c @@ -670,6 +670,13 @@ static int virtio_vsock_vqs_init(struct virtio_vsock *vsock) }; int ret; + mutex_lock(&vsock->rx_lock); + vsock->rx_buf_nr = 0; + vsock->rx_buf_max_nr = 0; + mutex_unlock(&vsock->rx_lock); + + atomic_set(&vsock->queued_replies, 0); + ret = virtio_find_vqs(vdev, VSOCK_VQ_MAX, vsock->vqs, vqs_info, NULL); if (ret < 0) return ret; @@ -779,9 +786,6 @@ static int virtio_vsock_probe(struct virtio_device *vdev) vsock->vdev = vdev; - vsock->rx_buf_nr = 0; - vsock->rx_buf_max_nr = 0; - atomic_set(&vsock->queued_replies, 0); mutex_init(&vsock->tx_lock); mutex_init(&vsock->rx_lock);
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. 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. Fixes: bd50c5dc182b ("vsock/virtio: add support for device suspend/resume") Link: https://lore.kernel.org/virtualization/20250207052033.2222629-1-junnan01.wu@samsung.com/ Co-developed-by: Ying Gao <ying01.gao@samsung.com> Signed-off-by: Ying Gao <ying01.gao@samsung.com> Signed-off-by: Junnan Wu <junnan01.wu@samsung.com> --- net/vmw_vsock/virtio_transport.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)