| Message ID | 20250207052033.2222629-1-junnan01.wu@samsung.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [1/2] vsock/virtio: Move rx_buf_nr and rx_buf_max_nr initialization position | expand |
Hi Junnan, Ying Thank you for the contribution! A few minor comments on the process: I think this series is missing a cover letter, not all the maintainers have been CCd, and you should add the tag net (because it's a fix) to the subject. (e.g. [PATCH net 1/2]). Here you can find some useful information[1]. [1]https://www.kernel.org/doc/html/latest/process/submitting-patches.html On Fri, Feb 07, 2025 at 01:20:32PM +0800, Junnan Wu wrote: >From: Ying Gao <ying01.gao@samsung.com> > >In function virtio_vsock_probe, it initializes the variables >"rx_buf_nr" and "rx_buf_max_nr", >but in function virtio_vsock_restore it doesn't. > >Move the initizalition position into function virtio_vsock_vqs_start. > >Once executing s2r twice in a row without I guess "s2r" is "suspend to resume" but is not that clear to me. >initializing rx_buf_nr and rx_buf_max_nr, >the rx_buf_max_nr increased to three times vq->num_free, >at this time, in function virtio_transport_rx_work, >the conditions to fill rx buffer >(rx_buf_nr < rx_buf_max_nr / 2) can't be met. > >Signed-off-by: Ying Gao <ying01.gao@samsung.com> >Signed-off-by: Junnan Wu <junnan01.wu@samsung.com> Maybe you need a "Co-Developed-by"? >--- > net/vmw_vsock/virtio_transport.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > >diff --git a/net/vmw_vsock/virtio_transport.c b/net/vmw_vsock/virtio_transport.c >index b58c3818f284..9eefd0fba92b 100644 >--- a/net/vmw_vsock/virtio_transport.c >+++ b/net/vmw_vsock/virtio_transport.c >@@ -688,6 +688,8 @@ static void virtio_vsock_vqs_start(struct virtio_vsock *vsock) > mutex_unlock(&vsock->tx_lock); > > mutex_lock(&vsock->rx_lock); >+ vsock->rx_buf_nr = 0; >+ vsock->rx_buf_max_nr = 0; > virtio_vsock_rx_fill(vsock); > vsock->rx_run = true; > mutex_unlock(&vsock->rx_lock); >@@ -779,8 +781,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); >-- >2.34.1 > Code LGTM. Thank you, Luigi
On Fri, Feb 07, 2025 at 01:20:32PM +0800, Junnan Wu wrote: >From: Ying Gao <ying01.gao@samsung.com> > >In function virtio_vsock_probe, it initializes the variables >"rx_buf_nr" and "rx_buf_max_nr", >but in function virtio_vsock_restore it doesn't. > >Move the initizalition position into function virtio_vsock_vqs_start. > >Once executing s2r twice in a row without s2r ? suspend 2 ram? Please define the acronym, it was hard for me to understand (the code helped me). >initializing rx_buf_nr and rx_buf_max_nr, >the rx_buf_max_nr increased to three times vq->num_free, >at this time, in function virtio_transport_rx_work, >the conditions to fill rx buffer >(rx_buf_nr < rx_buf_max_nr / 2) can't be met. > Please add a Fixes tag, in this case I think it should be: Fixes: bd50c5dc182b ("vsock/virtio: add support for device suspend/resume") but please, double check. >Signed-off-by: Ying Gao <ying01.gao@samsung.com> >Signed-off-by: Junnan Wu <junnan01.wu@samsung.com> >--- > net/vmw_vsock/virtio_transport.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) I find the commit title/description a bit hard to understand, please take a look at: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes In this case I'd write something like this: vsock/virtio: initialize rx_buf_nr and rx_buf_max_nr when resuming [Describe the symptom] When executing suspend/resume twice in a row, ... [Describe the issue] `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. ... [Desribe the fix, what this patch does] 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. ... > >diff --git a/net/vmw_vsock/virtio_transport.c b/net/vmw_vsock/virtio_transport.c >index b58c3818f284..9eefd0fba92b 100644 >--- a/net/vmw_vsock/virtio_transport.c >+++ b/net/vmw_vsock/virtio_transport.c >@@ -688,6 +688,8 @@ static void virtio_vsock_vqs_start(struct virtio_vsock *vsock) I think it is better to move the initialization of those fields in virtio_vsock_vqs_init(). > mutex_unlock(&vsock->tx_lock); > > mutex_lock(&vsock->rx_lock); >+ vsock->rx_buf_nr = 0; >+ vsock->rx_buf_max_nr = 0; > virtio_vsock_rx_fill(vsock); > vsock->rx_run = true; > mutex_unlock(&vsock->rx_lock); >@@ -779,8 +781,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); Should we also move `queued_replies` ? Thanks, Stefano > > mutex_init(&vsock->tx_lock); >-- >2.34.1 > >
diff --git a/net/vmw_vsock/virtio_transport.c b/net/vmw_vsock/virtio_transport.c index b58c3818f284..9eefd0fba92b 100644 --- a/net/vmw_vsock/virtio_transport.c +++ b/net/vmw_vsock/virtio_transport.c @@ -688,6 +688,8 @@ static void virtio_vsock_vqs_start(struct virtio_vsock *vsock) mutex_unlock(&vsock->tx_lock); mutex_lock(&vsock->rx_lock); + vsock->rx_buf_nr = 0; + vsock->rx_buf_max_nr = 0; virtio_vsock_rx_fill(vsock); vsock->rx_run = true; mutex_unlock(&vsock->rx_lock); @@ -779,8 +781,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);