[Patch net 0/2] vsock suspend/resume fix

[Patch net 0/2] vsock suspend/resume fix

[Junnan Wu]

CC all maintainers and reviews.
Modify commits accroding to reviewers' comments.
Re-organize the patches, cover letter, tag, Signed-Off, Co-worker etc.

Junnan Wu (2):
  vsock/virtio: initialize rx_buf_nr and rx_buf_max_nr when resuming
  vsock/virtio: Don't reset the created SOCKET during suspend to ram

 net/vmw_vsock/virtio_transport.c | 28 +++++++++++++++++++---------
 1 file changed, 19 insertions(+), 9 deletions(-)


base-commit: a64dcfb451e254085a7daee5fe51bf22959d52d3
-- 
2.34.1

[Patch net 1/2] vsock/virtio: initialize rx_buf_nr and rx_buf_max_nr when resuming

[Junnan Wu]

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`.

Fixes: bd50c5dc182b ("vsock/virtio: add support for device suspend/resume")
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

[Patch net 2/2] vsock/virtio: Don't reset the created SOCKET during suspend to ram

[Junnan Wu]

Function virtio_vsock_vqs_del will be invoked in 2 cases
virtio_vsock_remove() and virtio_vsock_freeze().

And when driver freeze, the connected socket will be set to TCP_CLOSE
and it will cause that socket can not be unusable after resume.

Refactor function virtio_vsock_vqs_del to differentiate the 2 use cases.

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 | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/net/vmw_vsock/virtio_transport.c b/net/vmw_vsock/virtio_transport.c
index f0e48e6911fc..909048c9069b 100644
--- a/net/vmw_vsock/virtio_transport.c
+++ b/net/vmw_vsock/virtio_transport.c
@@ -716,14 +716,20 @@ static void virtio_vsock_vqs_start(struct virtio_vsock *vsock)
 	queue_work(virtio_vsock_workqueue, &vsock->send_pkt_work);
 }
 
-static void virtio_vsock_vqs_del(struct virtio_vsock *vsock)
+static void virtio_vsock_vqs_del(struct virtio_vsock *vsock, bool vsock_reset)
 {
 	struct virtio_device *vdev = vsock->vdev;
 	struct sk_buff *skb;
 
-	/* Reset all connected sockets when the VQs disappear */
-	vsock_for_each_connected_socket(&virtio_transport.transport,
-					virtio_vsock_reset_sock);
+	/* When driver is removed, reset all connected
+	 * sockets because the VQs disappear.
+	 * When driver is just freezed, don't do that because the connected
+	 * socket still need to use after restore.
+	 */
+	if (vsock_reset) {
+		vsock_for_each_connected_socket(&virtio_transport.transport,
+						virtio_vsock_reset_sock);
+	}
 
 	/* Stop all work handlers to make sure no one is accessing the device,
 	 * so we can safely call virtio_reset_device().
@@ -831,7 +837,7 @@ static void virtio_vsock_remove(struct virtio_device *vdev)
 	rcu_assign_pointer(the_virtio_vsock, NULL);
 	synchronize_rcu();
 
-	virtio_vsock_vqs_del(vsock);
+	virtio_vsock_vqs_del(vsock, true);
 
 	/* Other works can be queued before 'config->del_vqs()', so we flush
 	 * all works before to free the vsock object to avoid use after free.
@@ -856,7 +862,7 @@ static int virtio_vsock_freeze(struct virtio_device *vdev)
 	rcu_assign_pointer(the_virtio_vsock, NULL);
 	synchronize_rcu();
 
-	virtio_vsock_vqs_del(vsock);
+	virtio_vsock_vqs_del(vsock, false);
 
 	mutex_unlock(&the_virtio_vsock_mutex);
 
-- 
2.34.1

Re: [Patch net 2/2] vsock/virtio: Don't reset the created SOCKET during suspend to ram

[Stefano Garzarella]

On Tue, Feb 11, 2025 at 03:19:22PM +0800, Junnan Wu wrote:
>Function virtio_vsock_vqs_del will be invoked in 2 cases
>virtio_vsock_remove() and virtio_vsock_freeze().
>
>And when driver freeze, the connected socket will be set to TCP_CLOSE
>and it will cause that socket can not be unusable after resume.
>
>Refactor function virtio_vsock_vqs_del to differentiate the 2 use cases.
>
>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 | 18 ++++++++++++------
> 1 file changed, 12 insertions(+), 6 deletions(-)

We are still discussing this in v1:

https://lore.kernel.org/virtualization/iv6oalr6yuwsfkoxnorp4t77fdjheteyojauwf2phshucdxatf@ominy3hfcpxb/T/#u

Please stop sending new versions before reaching an agreement!

BTW I still think this is wrong, so:

Nacked-by: Stefano Garzarella <sgarzare@redhat.com>


>
>diff --git a/net/vmw_vsock/virtio_transport.c b/net/vmw_vsock/virtio_transport.c
>index f0e48e6911fc..909048c9069b 100644
>--- a/net/vmw_vsock/virtio_transport.c
>+++ b/net/vmw_vsock/virtio_transport.c
>@@ -716,14 +716,20 @@ static void virtio_vsock_vqs_start(struct virtio_vsock *vsock)
> 	queue_work(virtio_vsock_workqueue, &vsock->send_pkt_work);
> }
>
>-static void virtio_vsock_vqs_del(struct virtio_vsock *vsock)
>+static void virtio_vsock_vqs_del(struct virtio_vsock *vsock, bool vsock_reset)
> {
> 	struct virtio_device *vdev = vsock->vdev;
> 	struct sk_buff *skb;
>
>-	/* Reset all connected sockets when the VQs disappear */
>-	vsock_for_each_connected_socket(&virtio_transport.transport,
>-					virtio_vsock_reset_sock);
>+	/* When driver is removed, reset all connected
>+	 * sockets because the VQs disappear.

You wrote it here too, you have to reset them because the VQs are going 
to disappear, isn't it the same after the freeze?

>+	 * When driver is just freezed, don't do that because the connected
>+	 * socket still need to use after restore.
>+	 */
>+	if (vsock_reset) {
>+		vsock_for_each_connected_socket(&virtio_transport.transport,
>+						virtio_vsock_reset_sock);
>+	}
>
> 	/* Stop all work handlers to make sure no one is accessing the device,
> 	 * so we can safely call virtio_reset_device().
>@@ -831,7 +837,7 @@ static void virtio_vsock_remove(struct virtio_device *vdev)
> 	rcu_assign_pointer(the_virtio_vsock, NULL);
> 	synchronize_rcu();
>
>-	virtio_vsock_vqs_del(vsock);
>+	virtio_vsock_vqs_del(vsock, true);
>
> 	/* Other works can be queued before 'config->del_vqs()', so we flush
> 	 * all works before to free the vsock object to avoid use after free.
>@@ -856,7 +862,7 @@ static int virtio_vsock_freeze(struct virtio_device *vdev)
> 	rcu_assign_pointer(the_virtio_vsock, NULL);
> 	synchronize_rcu();
>
>-	virtio_vsock_vqs_del(vsock);
>+	virtio_vsock_vqs_del(vsock, false);
>
> 	mutex_unlock(&the_virtio_vsock_mutex);
>
>-- 
>2.34.1
>

Re: [Patch net 0/2] vsock suspend/resume fix

[Stefano Garzarella]

For the third time, please READ this link:
https://www.kernel.org/doc/html/latest/process/submitting-patches.html

This is a v2, so you should put it in the tags [PATCH net v2 ...]

And also include a changelog and a link to the v1:

v1: https://lore.kernel.org/virtualization/20250207052033.2222629-1-junnan01.wu@samsung.com/

On Tue, Feb 11, 2025 at 03:19:20PM +0800, Junnan Wu wrote:
>CC all maintainers and reviews.
>Modify commits accroding to reviewers' comments.
>Re-organize the patches, cover letter, tag, Signed-Off, Co-worker etc.
>
>Junnan Wu (2):
>  vsock/virtio: initialize rx_buf_nr and rx_buf_max_nr when resuming
>  vsock/virtio: Don't reset the created SOCKET during suspend to ram
>
> net/vmw_vsock/virtio_transport.c | 28 +++++++++++++++++++---------
> 1 file changed, 19 insertions(+), 9 deletions(-)
>
>
>base-commit: a64dcfb451e254085a7daee5fe51bf22959d52d3
>-- 
>2.34.1
>

Re: [Patch net 1/2] vsock/virtio: initialize rx_buf_nr and rx_buf_max_nr when resuming

[Stefano Garzarella]

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

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.

The rest LGTM.

Stefano

>
>Fixes: bd50c5dc182b ("vsock/virtio: add support for device suspend/resume")
>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
>

Re: Re: [Patch net 1/2] vsock/virtio: initialize rx_buf_nr and rx_buf_max_nr when resuming

[Junnan Wu]

>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.

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

BRs
Junnan Wu

>The rest LGTM.
>
>Stefano
>
>>
>>Fixes: bd50c5dc182b ("vsock/virtio: add support for device suspend/resume")
>>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
>>

Re: [Patch net 1/2] vsock/virtio: initialize rx_buf_nr and rx_buf_max_nr when resuming

[Stefano Garzarella]

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

Thanks,
Stefano

>
>BRs
>Junnan Wu
>
>>The rest LGTM.
>>
>>Stefano
>>
>>>
>>>Fixes: bd50c5dc182b ("vsock/virtio: add support for device suspend/resume")
>>>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
>>>
>

Re: [Patch net 1/2] vsock/virtio: initialize rx_buf_nr and rx_buf_max_nr when resuming

[Stefano Garzarella]

On Thu, 13 Feb 2025 at 10:25, Stefano Garzarella <sgarzare@redhat.com> 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

Re: Re: [Patch net 1/2] vsock/virtio: initialize rx_buf_nr and rx_buf_max_nr when resuming

[Junnan Wu]

On Thu, 13 Feb 2025 at 15:47, Stefano Garzarella <sgarzare@redhat.com> wrote:
>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

Well, I agree with you that these two patches should be splited.
And I will send v3 version of the first patch individually.

And according to our discussion, the second one can be ignored, until
we find a suitable way to deal the scenario I metionded.

Thanks,
Junnan

[Patch net v3] vsock/virtio: fix variables initialization during resuming

[Junnan Wu]

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

Re: [Patch net v3] vsock/virtio: fix variables initialization during resuming

[Luigi Leonardi]

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>

Re: [Patch net v3] vsock/virtio: fix variables initialization during resuming

[Michael S. Tsirkin]

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

Re: [Patch net v3] vsock/virtio: fix variables initialization during resuming

[Stefano Garzarella]

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>

Re: [Patch net v3] vsock/virtio: fix variables initialization during resuming

[patchwork-bot+netdevbpf]

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!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [Patch net 2/2] vsock/virtio: Don't reset the created SOCKET during suspend to ram

[Markus Elfring]

…
> and it will cause that socket can not be unusable after resume.
…

I find such a wording confusing.
Can the change description become clearer?

Regards,
Markus