Currently vsock's lingering effectively boils down to waiting (or timing
out) until packets are consumed or dropped by the peer; be it by receiving
the data, closing or shutting down the connection.

To align with the semantics described in the SO_LINGER section of man
socket(7) and to mimic AF_INET's behaviour more closely, change the logic
of a lingering close(): instead of waiting for all data to be handled,
block until data is considered sent from the vsock's transport point of
view. That is until worker picks the packets for processing and decrements
virtio_vsock_sock::bytes_unsent down to 0.

Note that such lingering is limited to transports that actually implement
vsock_transport::unsent_bytes() callback. This excludes Hyper-V and VMCI,
under which no lingering would be observed.

The implementation does not adhere strictly to man page's interpretation of
SO_LINGER: shutdown() will not trigger the lingering. This follows AF_INET.

Signed-off-by: Michal Luczaj <mhal@rbox.co>
---
 net/vmw_vsock/virtio_transport_common.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
index 7f7de6d8809655fe522749fbbc9025df71f071bd..aeb7f3794f7cfc251dde878cb44fdcc54814c89c 100644
--- a/net/vmw_vsock/virtio_transport_common.c
+++ b/net/vmw_vsock/virtio_transport_common.c
@@ -1196,12 +1196,21 @@ static void virtio_transport_wait_close(struct sock *sk, long timeout)
 {
 	if (timeout) {
 		DEFINE_WAIT_FUNC(wait, woken_wake_function);
+		ssize_t (*unsent)(struct vsock_sock *vsk);
+		struct vsock_sock *vsk = vsock_sk(sk);
+
+		/* Some transports (Hyper-V, VMCI) do not implement
+		 * unsent_bytes. For those, no lingering on close().
+		 */
+		unsent = vsk->transport->unsent_bytes;
+		if (!unsent)
+			return;
 
 		add_wait_queue(sk_sleep(sk), &wait);
 
 		do {
-			if (sk_wait_event(sk, &timeout,
-					  sock_flag(sk, SOCK_DONE), &wait))
+			if (sk_wait_event(sk, &timeout, unsent(vsk) == 0,
+					  &wait))
 				break;
 		} while (!signal_pending(current) && timeout);
 

-- 
2.49.0

Hi Michal,

On Mon, Apr 21, 2025 at 11:50:41PM +0200, Michal Luczaj wrote:
>Currently vsock's lingering effectively boils down to waiting (or timing
>out) until packets are consumed or dropped by the peer; be it by receiving
>the data, closing or shutting down the connection.
>
>To align with the semantics described in the SO_LINGER section of man
>socket(7) and to mimic AF_INET's behaviour more closely, change the logic
>of a lingering close(): instead of waiting for all data to be handled,
>block until data is considered sent from the vsock's transport point of
>view. That is until worker picks the packets for processing and decrements
>virtio_vsock_sock::bytes_unsent down to 0.
>
>Note that such lingering is limited to transports that actually implement
>vsock_transport::unsent_bytes() callback. This excludes Hyper-V and VMCI,
>under which no lingering would be observed.
>
>The implementation does not adhere strictly to man page's interpretation of
>SO_LINGER: shutdown() will not trigger the lingering. This follows AF_INET.
>
>Signed-off-by: Michal Luczaj <mhal@rbox.co>
>---
> net/vmw_vsock/virtio_transport_common.c | 13 +++++++++++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
>
>diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
>index 7f7de6d8809655fe522749fbbc9025df71f071bd..aeb7f3794f7cfc251dde878cb44fdcc54814c89c 100644
>--- a/net/vmw_vsock/virtio_transport_common.c
>+++ b/net/vmw_vsock/virtio_transport_common.c
>@@ -1196,12 +1196,21 @@ static void virtio_transport_wait_close(struct sock *sk, long timeout)
> {
> 	if (timeout) {
> 		DEFINE_WAIT_FUNC(wait, woken_wake_function);
>+		ssize_t (*unsent)(struct vsock_sock *vsk);
>+		struct vsock_sock *vsk = vsock_sk(sk);
>+
>+		/* Some transports (Hyper-V, VMCI) do not implement
>+		 * unsent_bytes. For those, no lingering on close().
>+		 */
>+		unsent = vsk->transport->unsent_bytes;
>+		if (!unsent)
>+			return;

IIUC if `unsent_bytes` is not implemented, virtio_transport_wait_close 
basically does nothing. My concern is that we are breaking the userspace 
due to a change in the behavior: Before this patch, with a vmci/hyper-v 
transport, this function would wait for SOCK_DONE to be set, but not 
anymore.

>
> 		add_wait_queue(sk_sleep(sk), &wait);
>
> 		do {
>-			if (sk_wait_event(sk, &timeout,
>-					  sock_flag(sk, SOCK_DONE), 
>&wait))
>+			if (sk_wait_event(sk, &timeout, unsent(vsk) == 
>0,
>+					  &wait))
> 				break;
> 		} while (!signal_pending(current) && timeout);
>
>
>-- 2.49.0
>

Thanks,
Luigi

On Wed, Apr 23, 2025 at 05:53:12PM +0200, Luigi Leonardi wrote:
>Hi Michal,
>
>On Mon, Apr 21, 2025 at 11:50:41PM +0200, Michal Luczaj wrote:
>>Currently vsock's lingering effectively boils down to waiting (or timing
>>out) until packets are consumed or dropped by the peer; be it by receiving
>>the data, closing or shutting down the connection.
>>
>>To align with the semantics described in the SO_LINGER section of man
>>socket(7) and to mimic AF_INET's behaviour more closely, change the logic
>>of a lingering close(): instead of waiting for all data to be handled,
>>block until data is considered sent from the vsock's transport point of
>>view. That is until worker picks the packets for processing and decrements
>>virtio_vsock_sock::bytes_unsent down to 0.
>>
>>Note that such lingering is limited to transports that actually implement
>>vsock_transport::unsent_bytes() callback. This excludes Hyper-V and VMCI,
>>under which no lingering would be observed.
>>
>>The implementation does not adhere strictly to man page's interpretation of
>>SO_LINGER: shutdown() will not trigger the lingering. This follows AF_INET.
>>
>>Signed-off-by: Michal Luczaj <mhal@rbox.co>
>>---
>>net/vmw_vsock/virtio_transport_common.c | 13 +++++++++++--
>>1 file changed, 11 insertions(+), 2 deletions(-)
>>
>>diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
>>index 7f7de6d8809655fe522749fbbc9025df71f071bd..aeb7f3794f7cfc251dde878cb44fdcc54814c89c 100644
>>--- a/net/vmw_vsock/virtio_transport_common.c
>>+++ b/net/vmw_vsock/virtio_transport_common.c
>>@@ -1196,12 +1196,21 @@ static void virtio_transport_wait_close(struct sock *sk, long timeout)
>>{
>>	if (timeout) {
>>		DEFINE_WAIT_FUNC(wait, woken_wake_function);
>>+		ssize_t (*unsent)(struct vsock_sock *vsk);
>>+		struct vsock_sock *vsk = vsock_sk(sk);
>>+
>>+		/* Some transports (Hyper-V, VMCI) do not implement
>>+		 * unsent_bytes. For those, no lingering on close().
>>+		 */
>>+		unsent = vsk->transport->unsent_bytes;
>>+		if (!unsent)
>>+			return;
>
>IIUC if `unsent_bytes` is not implemented, virtio_transport_wait_close 
>basically does nothing. My concern is that we are breaking the 
>userspace due to a change in the behavior: Before this patch, with a 
>vmci/hyper-v transport, this function would wait for SOCK_DONE to be 
>set, but not anymore.

Wait, we are in virtio_transport_common.c, why we are talking about 
Hyper-V and VMCI?

I asked to check `vsk->transport->unsent_bytes` in the v1, because this 
code was part of af_vsock.c, but now we are back to virtio code, so I'm 
confused...

Stefano

>
>>
>>		add_wait_queue(sk_sleep(sk), &wait);
>>
>>		do {
>>-			if (sk_wait_event(sk, &timeout,
>>-					  sock_flag(sk, SOCK_DONE), &wait))
>>+			if (sk_wait_event(sk, &timeout, unsent(vsk) == 0,
>>+					  &wait))
>>				break;
>>		} while (!signal_pending(current) && timeout);
>>
>>
>>-- 2.49.0
>>
>
>Thanks,
>Luigi
>

On 4/23/25 18:34, Stefano Garzarella wrote:
> On Wed, Apr 23, 2025 at 05:53:12PM +0200, Luigi Leonardi wrote:
>> Hi Michal,
>>
>> On Mon, Apr 21, 2025 at 11:50:41PM +0200, Michal Luczaj wrote:
>>> Currently vsock's lingering effectively boils down to waiting (or timing
>>> out) until packets are consumed or dropped by the peer; be it by receiving
>>> the data, closing or shutting down the connection.
>>>
>>> To align with the semantics described in the SO_LINGER section of man
>>> socket(7) and to mimic AF_INET's behaviour more closely, change the logic
>>> of a lingering close(): instead of waiting for all data to be handled,
>>> block until data is considered sent from the vsock's transport point of
>>> view. That is until worker picks the packets for processing and decrements
>>> virtio_vsock_sock::bytes_unsent down to 0.
>>>
>>> Note that such lingering is limited to transports that actually implement
>>> vsock_transport::unsent_bytes() callback. This excludes Hyper-V and VMCI,
>>> under which no lingering would be observed.
>>>
>>> The implementation does not adhere strictly to man page's interpretation of
>>> SO_LINGER: shutdown() will not trigger the lingering. This follows AF_INET.
>>>
>>> Signed-off-by: Michal Luczaj <mhal@rbox.co>
>>> ---
>>> net/vmw_vsock/virtio_transport_common.c | 13 +++++++++++--
>>> 1 file changed, 11 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
>>> index 7f7de6d8809655fe522749fbbc9025df71f071bd..aeb7f3794f7cfc251dde878cb44fdcc54814c89c 100644
>>> --- a/net/vmw_vsock/virtio_transport_common.c
>>> +++ b/net/vmw_vsock/virtio_transport_common.c
>>> @@ -1196,12 +1196,21 @@ static void virtio_transport_wait_close(struct sock *sk, long timeout)
>>> {
>>> 	if (timeout) {
>>> 		DEFINE_WAIT_FUNC(wait, woken_wake_function);
>>> +		ssize_t (*unsent)(struct vsock_sock *vsk);
>>> +		struct vsock_sock *vsk = vsock_sk(sk);
>>> +
>>> +		/* Some transports (Hyper-V, VMCI) do not implement
>>> +		 * unsent_bytes. For those, no lingering on close().
>>> +		 */
>>> +		unsent = vsk->transport->unsent_bytes;
>>> +		if (!unsent)
>>> +			return;
>>
>> IIUC if `unsent_bytes` is not implemented, virtio_transport_wait_close 
>> basically does nothing. My concern is that we are breaking the 
>> userspace due to a change in the behavior: Before this patch, with a 
>> vmci/hyper-v transport, this function would wait for SOCK_DONE to be 
>> set, but not anymore.
> 
> Wait, we are in virtio_transport_common.c, why we are talking about 
> Hyper-V and VMCI?
> 
> I asked to check `vsk->transport->unsent_bytes` in the v1, because this 
> code was part of af_vsock.c, but now we are back to virtio code, so I'm 
> confused...

Might your confusion be because of similar names?
vsock_transport::unsent_bytes != virtio_vsock_sock::bytes_unsent

I agree with Luigi, it is a breaking change for userspace depending on a
non-standard behaviour. What's the protocol here; do it anyway, then see if
anyone complains?

As for Hyper-V and VMCI losing the "lingering", do we care? And if we do,
take Hyper-V, is it possible to test any changes without access to
proprietary host/hypervisor?

On Wed, Apr 23, 2025 at 11:06:33PM +0200, Michal Luczaj wrote:
>On 4/23/25 18:34, Stefano Garzarella wrote:
>> On Wed, Apr 23, 2025 at 05:53:12PM +0200, Luigi Leonardi wrote:
>>> Hi Michal,
>>>
>>> On Mon, Apr 21, 2025 at 11:50:41PM +0200, Michal Luczaj wrote:
>>>> Currently vsock's lingering effectively boils down to waiting (or timing
>>>> out) until packets are consumed or dropped by the peer; be it by receiving
>>>> the data, closing or shutting down the connection.
>>>>
>>>> To align with the semantics described in the SO_LINGER section of man
>>>> socket(7) and to mimic AF_INET's behaviour more closely, change the logic
>>>> of a lingering close(): instead of waiting for all data to be handled,
>>>> block until data is considered sent from the vsock's transport point of
>>>> view. That is until worker picks the packets for processing and decrements
>>>> virtio_vsock_sock::bytes_unsent down to 0.
>>>>
>>>> Note that such lingering is limited to transports that actually implement
>>>> vsock_transport::unsent_bytes() callback. This excludes Hyper-V and VMCI,
>>>> under which no lingering would be observed.
>>>>
>>>> The implementation does not adhere strictly to man page's interpretation of
>>>> SO_LINGER: shutdown() will not trigger the lingering. This follows AF_INET.
>>>>
>>>> Signed-off-by: Michal Luczaj <mhal@rbox.co>
>>>> ---
>>>> net/vmw_vsock/virtio_transport_common.c | 13 +++++++++++--
>>>> 1 file changed, 11 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
>>>> index 7f7de6d8809655fe522749fbbc9025df71f071bd..aeb7f3794f7cfc251dde878cb44fdcc54814c89c 100644
>>>> --- a/net/vmw_vsock/virtio_transport_common.c
>>>> +++ b/net/vmw_vsock/virtio_transport_common.c
>>>> @@ -1196,12 +1196,21 @@ static void virtio_transport_wait_close(struct sock *sk, long timeout)
>>>> {
>>>> 	if (timeout) {
>>>> 		DEFINE_WAIT_FUNC(wait, woken_wake_function);
>>>> +		ssize_t (*unsent)(struct vsock_sock *vsk);
>>>> +		struct vsock_sock *vsk = vsock_sk(sk);
>>>> +
>>>> +		/* Some transports (Hyper-V, VMCI) do not implement
>>>> +		 * unsent_bytes. For those, no lingering on close().
>>>> +		 */
>>>> +		unsent = vsk->transport->unsent_bytes;
>>>> +		if (!unsent)
>>>> +			return;
>>>
>>> IIUC if `unsent_bytes` is not implemented, virtio_transport_wait_close
>>> basically does nothing. My concern is that we are breaking the
>>> userspace due to a change in the behavior: Before this patch, with a
>>> vmci/hyper-v transport, this function would wait for SOCK_DONE to be
>>> set, but not anymore.
>>
>> Wait, we are in virtio_transport_common.c, why we are talking about
>> Hyper-V and VMCI?
>>
>> I asked to check `vsk->transport->unsent_bytes` in the v1, because this
>> code was part of af_vsock.c, but now we are back to virtio code, so I'm
>> confused...
>
>Might your confusion be because of similar names?

In v1 this code IIRC was in af_vsock.c, now you pushed back on virtio 
common code, so I still don't understand how 
virtio_transport_wait_close() can be called with vmci or hyper-v 
transports.

Can you provide an example?

>vsock_transport::unsent_bytes != virtio_vsock_sock::bytes_unsent
>
>I agree with Luigi, it is a breaking change for userspace depending on a
>non-standard behaviour. What's the protocol here; do it anyway, then see if
>anyone complains?
>
>As for Hyper-V and VMCI losing the "lingering", do we care? And if we do,
>take Hyper-V, is it possible to test any changes without access to
>proprietary host/hypervisor?
>

Again, how this code can be called when using vmci or hyper-v 
transports?

If we go back on v1 implementation, I can understand it, but with this 
version I really don't understand the scenario.

Stefano

On 4/24/25 09:28, Stefano Garzarella wrote:
> On Wed, Apr 23, 2025 at 11:06:33PM +0200, Michal Luczaj wrote:
>> On 4/23/25 18:34, Stefano Garzarella wrote:
>>> On Wed, Apr 23, 2025 at 05:53:12PM +0200, Luigi Leonardi wrote:
>>>> Hi Michal,
>>>>
>>>> On Mon, Apr 21, 2025 at 11:50:41PM +0200, Michal Luczaj wrote:
>>>>> Currently vsock's lingering effectively boils down to waiting (or timing
>>>>> out) until packets are consumed or dropped by the peer; be it by receiving
>>>>> the data, closing or shutting down the connection.
>>>>>
>>>>> To align with the semantics described in the SO_LINGER section of man
>>>>> socket(7) and to mimic AF_INET's behaviour more closely, change the logic
>>>>> of a lingering close(): instead of waiting for all data to be handled,
>>>>> block until data is considered sent from the vsock's transport point of
>>>>> view. That is until worker picks the packets for processing and decrements
>>>>> virtio_vsock_sock::bytes_unsent down to 0.
>>>>>
>>>>> Note that such lingering is limited to transports that actually implement
>>>>> vsock_transport::unsent_bytes() callback. This excludes Hyper-V and VMCI,
>>>>> under which no lingering would be observed.
>>>>>
>>>>> The implementation does not adhere strictly to man page's interpretation of
>>>>> SO_LINGER: shutdown() will not trigger the lingering. This follows AF_INET.
>>>>>
>>>>> Signed-off-by: Michal Luczaj <mhal@rbox.co>
>>>>> ---
>>>>> net/vmw_vsock/virtio_transport_common.c | 13 +++++++++++--
>>>>> 1 file changed, 11 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
>>>>> index 7f7de6d8809655fe522749fbbc9025df71f071bd..aeb7f3794f7cfc251dde878cb44fdcc54814c89c 100644
>>>>> --- a/net/vmw_vsock/virtio_transport_common.c
>>>>> +++ b/net/vmw_vsock/virtio_transport_common.c
>>>>> @@ -1196,12 +1196,21 @@ static void virtio_transport_wait_close(struct sock *sk, long timeout)
>>>>> {
>>>>> 	if (timeout) {
>>>>> 		DEFINE_WAIT_FUNC(wait, woken_wake_function);
>>>>> +		ssize_t (*unsent)(struct vsock_sock *vsk);
>>>>> +		struct vsock_sock *vsk = vsock_sk(sk);
>>>>> +
>>>>> +		/* Some transports (Hyper-V, VMCI) do not implement
>>>>> +		 * unsent_bytes. For those, no lingering on close().
>>>>> +		 */
>>>>> +		unsent = vsk->transport->unsent_bytes;
>>>>> +		if (!unsent)
>>>>> +			return;
>>>>
>>>> IIUC if `unsent_bytes` is not implemented, virtio_transport_wait_close
>>>> basically does nothing. My concern is that we are breaking the
>>>> userspace due to a change in the behavior: Before this patch, with a
>>>> vmci/hyper-v transport, this function would wait for SOCK_DONE to be
>>>> set, but not anymore.
>>>
>>> Wait, we are in virtio_transport_common.c, why we are talking about
>>> Hyper-V and VMCI?
>>>
>>> I asked to check `vsk->transport->unsent_bytes` in the v1, because this
>>> code was part of af_vsock.c, but now we are back to virtio code, so I'm
>>> confused...
>>
>> Might your confusion be because of similar names?
> 
> In v1 this code IIRC was in af_vsock.c, now you pushed back on virtio 
> common code, so I still don't understand how 
> virtio_transport_wait_close() can be called with vmci or hyper-v 
> transports.
> 
> Can you provide an example?

You're right, it was me who was confused. VMCI and Hyper-V have their own
vsock_transport::release callbacks that do not call
virtio_transport_wait_close().

So VMCI and Hyper-V never lingered anyway?

>> vsock_transport::unsent_bytes != virtio_vsock_sock::bytes_unsent
>>
>> I agree with Luigi, it is a breaking change for userspace depending on a
>> non-standard behaviour. What's the protocol here; do it anyway, then see if
>> anyone complains?
>>
>> As for Hyper-V and VMCI losing the "lingering", do we care? And if we do,
>> take Hyper-V, is it possible to test any changes without access to
>> proprietary host/hypervisor?
>>
> 
> Again, how this code can be called when using vmci or hyper-v 
> transports?

It cannot, you're right.

> If we go back on v1 implementation, I can understand it, but with this 
> version I really don't understand the scenario.
> 
> Stefano
>

On Thu, 24 Apr 2025 at 09:53, Michal Luczaj <mhal@rbox.co> wrote:
>
> On 4/24/25 09:28, Stefano Garzarella wrote:
> > On Wed, Apr 23, 2025 at 11:06:33PM +0200, Michal Luczaj wrote:
> >> On 4/23/25 18:34, Stefano Garzarella wrote:
> >>> On Wed, Apr 23, 2025 at 05:53:12PM +0200, Luigi Leonardi wrote:
> >>>> Hi Michal,
> >>>>
> >>>> On Mon, Apr 21, 2025 at 11:50:41PM +0200, Michal Luczaj wrote:
> >>>>> Currently vsock's lingering effectively boils down to waiting (or timing
> >>>>> out) until packets are consumed or dropped by the peer; be it by receiving
> >>>>> the data, closing or shutting down the connection.
> >>>>>
> >>>>> To align with the semantics described in the SO_LINGER section of man
> >>>>> socket(7) and to mimic AF_INET's behaviour more closely, change the logic
> >>>>> of a lingering close(): instead of waiting for all data to be handled,
> >>>>> block until data is considered sent from the vsock's transport point of
> >>>>> view. That is until worker picks the packets for processing and decrements
> >>>>> virtio_vsock_sock::bytes_unsent down to 0.
> >>>>>
> >>>>> Note that such lingering is limited to transports that actually implement
> >>>>> vsock_transport::unsent_bytes() callback. This excludes Hyper-V and VMCI,
> >>>>> under which no lingering would be observed.
> >>>>>
> >>>>> The implementation does not adhere strictly to man page's interpretation of
> >>>>> SO_LINGER: shutdown() will not trigger the lingering. This follows AF_INET.
> >>>>>
> >>>>> Signed-off-by: Michal Luczaj <mhal@rbox.co>
> >>>>> ---
> >>>>> net/vmw_vsock/virtio_transport_common.c | 13 +++++++++++--
> >>>>> 1 file changed, 11 insertions(+), 2 deletions(-)
> >>>>>
> >>>>> diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
> >>>>> index 7f7de6d8809655fe522749fbbc9025df71f071bd..aeb7f3794f7cfc251dde878cb44fdcc54814c89c 100644
> >>>>> --- a/net/vmw_vsock/virtio_transport_common.c
> >>>>> +++ b/net/vmw_vsock/virtio_transport_common.c
> >>>>> @@ -1196,12 +1196,21 @@ static void virtio_transport_wait_close(struct sock *sk, long timeout)
> >>>>> {
> >>>>>   if (timeout) {
> >>>>>           DEFINE_WAIT_FUNC(wait, woken_wake_function);
> >>>>> +         ssize_t (*unsent)(struct vsock_sock *vsk);
> >>>>> +         struct vsock_sock *vsk = vsock_sk(sk);
> >>>>> +
> >>>>> +         /* Some transports (Hyper-V, VMCI) do not implement
> >>>>> +          * unsent_bytes. For those, no lingering on close().
> >>>>> +          */
> >>>>> +         unsent = vsk->transport->unsent_bytes;
> >>>>> +         if (!unsent)
> >>>>> +                 return;
> >>>>
> >>>> IIUC if `unsent_bytes` is not implemented, virtio_transport_wait_close
> >>>> basically does nothing. My concern is that we are breaking the
> >>>> userspace due to a change in the behavior: Before this patch, with a
> >>>> vmci/hyper-v transport, this function would wait for SOCK_DONE to be
> >>>> set, but not anymore.
> >>>
> >>> Wait, we are in virtio_transport_common.c, why we are talking about
> >>> Hyper-V and VMCI?
> >>>
> >>> I asked to check `vsk->transport->unsent_bytes` in the v1, because this
> >>> code was part of af_vsock.c, but now we are back to virtio code, so I'm
> >>> confused...
> >>
> >> Might your confusion be because of similar names?
> >
> > In v1 this code IIRC was in af_vsock.c, now you pushed back on virtio
> > common code, so I still don't understand how
> > virtio_transport_wait_close() can be called with vmci or hyper-v
> > transports.
> >
> > Can you provide an example?
>
> You're right, it was me who was confused. VMCI and Hyper-V have their own
> vsock_transport::release callbacks that do not call
> virtio_transport_wait_close().
>
> So VMCI and Hyper-V never lingered anyway?

I think so.

Indeed I was happy with v1, since I think this should be supported by
the vsock core and should not depend on the transport.
But we can do also later.

Stefano

>
> >> vsock_transport::unsent_bytes != virtio_vsock_sock::bytes_unsent
> >>
> >> I agree with Luigi, it is a breaking change for userspace depending on a
> >> non-standard behaviour. What's the protocol here; do it anyway, then see if
> >> anyone complains?
> >>
> >> As for Hyper-V and VMCI losing the "lingering", do we care? And if we do,
> >> take Hyper-V, is it possible to test any changes without access to
> >> proprietary host/hypervisor?
> >>
> >
> > Again, how this code can be called when using vmci or hyper-v
> > transports?
>
> It cannot, you're right.
>
> > If we go back on v1 implementation, I can understand it, but with this
> > version I really don't understand the scenario.
> >
> > Stefano
> >
>

On 4/24/25 10:36, Stefano Garzarella wrote:
> On Thu, 24 Apr 2025 at 09:53, Michal Luczaj <mhal@rbox.co> wrote:
>>
>> On 4/24/25 09:28, Stefano Garzarella wrote:
>>> On Wed, Apr 23, 2025 at 11:06:33PM +0200, Michal Luczaj wrote:
>>>> On 4/23/25 18:34, Stefano Garzarella wrote:
>>>>> On Wed, Apr 23, 2025 at 05:53:12PM +0200, Luigi Leonardi wrote:
>>>>>> Hi Michal,
>>>>>>
>>>>>> On Mon, Apr 21, 2025 at 11:50:41PM +0200, Michal Luczaj wrote:
>>>>>>> Currently vsock's lingering effectively boils down to waiting (or timing
>>>>>>> out) until packets are consumed or dropped by the peer; be it by receiving
>>>>>>> the data, closing or shutting down the connection.
>>>>>>>
>>>>>>> To align with the semantics described in the SO_LINGER section of man
>>>>>>> socket(7) and to mimic AF_INET's behaviour more closely, change the logic
>>>>>>> of a lingering close(): instead of waiting for all data to be handled,
>>>>>>> block until data is considered sent from the vsock's transport point of
>>>>>>> view. That is until worker picks the packets for processing and decrements
>>>>>>> virtio_vsock_sock::bytes_unsent down to 0.
>>>>>>>
>>>>>>> Note that such lingering is limited to transports that actually implement
>>>>>>> vsock_transport::unsent_bytes() callback. This excludes Hyper-V and VMCI,
>>>>>>> under which no lingering would be observed.
>>>>>>>
>>>>>>> The implementation does not adhere strictly to man page's interpretation of
>>>>>>> SO_LINGER: shutdown() will not trigger the lingering. This follows AF_INET.
>>>>>>>
>>>>>>> Signed-off-by: Michal Luczaj <mhal@rbox.co>
>>>>>>> ---
>>>>>>> net/vmw_vsock/virtio_transport_common.c | 13 +++++++++++--
>>>>>>> 1 file changed, 11 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
>>>>>>> index 7f7de6d8809655fe522749fbbc9025df71f071bd..aeb7f3794f7cfc251dde878cb44fdcc54814c89c 100644
>>>>>>> --- a/net/vmw_vsock/virtio_transport_common.c
>>>>>>> +++ b/net/vmw_vsock/virtio_transport_common.c
>>>>>>> @@ -1196,12 +1196,21 @@ static void virtio_transport_wait_close(struct sock *sk, long timeout)
>>>>>>> {
>>>>>>>   if (timeout) {
>>>>>>>           DEFINE_WAIT_FUNC(wait, woken_wake_function);
>>>>>>> +         ssize_t (*unsent)(struct vsock_sock *vsk);
>>>>>>> +         struct vsock_sock *vsk = vsock_sk(sk);
>>>>>>> +
>>>>>>> +         /* Some transports (Hyper-V, VMCI) do not implement
>>>>>>> +          * unsent_bytes. For those, no lingering on close().
>>>>>>> +          */
>>>>>>> +         unsent = vsk->transport->unsent_bytes;
>>>>>>> +         if (!unsent)
>>>>>>> +                 return;
>>>>>>
>>>>>> IIUC if `unsent_bytes` is not implemented, virtio_transport_wait_close
>>>>>> basically does nothing. My concern is that we are breaking the
>>>>>> userspace due to a change in the behavior: Before this patch, with a
>>>>>> vmci/hyper-v transport, this function would wait for SOCK_DONE to be
>>>>>> set, but not anymore.
>>>>>
>>>>> Wait, we are in virtio_transport_common.c, why we are talking about
>>>>> Hyper-V and VMCI?
>>>>>
>>>>> I asked to check `vsk->transport->unsent_bytes` in the v1, because this
>>>>> code was part of af_vsock.c, but now we are back to virtio code, so I'm
>>>>> confused...
>>>>
>>>> Might your confusion be because of similar names?
>>>
>>> In v1 this code IIRC was in af_vsock.c, now you pushed back on virtio
>>> common code, so I still don't understand how
>>> virtio_transport_wait_close() can be called with vmci or hyper-v
>>> transports.
>>>
>>> Can you provide an example?
>>
>> You're right, it was me who was confused. VMCI and Hyper-V have their own
>> vsock_transport::release callbacks that do not call
>> virtio_transport_wait_close().
>>
>> So VMCI and Hyper-V never lingered anyway?
> 
> I think so.
> 
> Indeed I was happy with v1, since I think this should be supported by
> the vsock core and should not depend on the transport.
> But we can do also later.

OK, for now let me fix this nonsense in comment and commit message.

But I'll wait for your opinion on [1] (drop, squash, change order of
patches?) before posting v3.

[1]:
https://lore.kernel.org/netdev/20250421-vsock-linger-v2-2-fe9febd64668@rbox.co/

Thanks,
Michal

On Thu, Apr 24, 2025 at 01:24:59PM +0200, Michal Luczaj wrote:
>On 4/24/25 10:36, Stefano Garzarella wrote:
>> On Thu, 24 Apr 2025 at 09:53, Michal Luczaj <mhal@rbox.co> wrote:
>>> On 4/24/25 09:28, Stefano Garzarella wrote:

[...]

>>> You're right, it was me who was confused. VMCI and Hyper-V have their own
>>> vsock_transport::release callbacks that do not call
>>> virtio_transport_wait_close().
>>>
>>> So VMCI and Hyper-V never lingered anyway?
>>
>> I think so.
>>
>> Indeed I was happy with v1, since I think this should be supported by
>> the vsock core and should not depend on the transport.
>> But we can do also later.
>
>OK, for now let me fix this nonsense in comment and commit message.

Thanks!

>
>But I'll wait for your opinion on [1] (drop, squash, change order of
>patches?) before posting v3.

I'm fine with a second patch to fix the indentation and the order looks 
fine.

BTW I'm thinking if it makes sense to go back on moving the lingering in 
the core. I mean, if `unsent_bytes` is implemented, support linger, if 
not, don't support it, like now.

That said, this should be implemented in another patch (or eventually 
another series if you prefer), so my idea is the following split:
- use unsent_bytes() just in virtio
- move linger support in af_vsock.c (depending on transports 
   implementing unsent_bytes())
- implement unsent_bytes() in other transports (in the future)

WDYT?

Thanks,
Stefano

On 4/28/25 15:56, Stefano Garzarella wrote:
> On Thu, Apr 24, 2025 at 01:24:59PM +0200, Michal Luczaj wrote:
>> On 4/24/25 10:36, Stefano Garzarella wrote:
>>> On Thu, 24 Apr 2025 at 09:53, Michal Luczaj <mhal@rbox.co> wrote:
>>>> On 4/24/25 09:28, Stefano Garzarella wrote:
> 
> [...]
> 
>>>> You're right, it was me who was confused. VMCI and Hyper-V have their own
>>>> vsock_transport::release callbacks that do not call
>>>> virtio_transport_wait_close().
>>>>
>>>> So VMCI and Hyper-V never lingered anyway?
>>>
>>> I think so.
>>>
>>> Indeed I was happy with v1, since I think this should be supported by
>>> the vsock core and should not depend on the transport.
>>> But we can do also later.
>>
>> OK, for now let me fix this nonsense in comment and commit message.
> 
> Thanks!
> 
>>
>> But I'll wait for your opinion on [1] (drop, squash, change order of
>> patches?) before posting v3.
> 
> I'm fine with a second patch to fix the indentation and the order looks 
> fine.
> 
> BTW I'm thinking if it makes sense to go back on moving the lingering in 
> the core. I mean, if `unsent_bytes` is implemented, support linger, if 
> not, don't support it, like now.
> 
> That said, this should be implemented in another patch (or eventually 
> another series if you prefer), so my idea is the following split:
> - use unsent_bytes() just in virtio
> - move linger support in af_vsock.c (depending on transports 
>    implementing unsent_bytes())
> - implement unsent_bytes() in other transports (in the future)
> 
> WDYT?

Sure, makes sense. Even though I'm not certain I understand "use
unsent_bytes() just in virtio" part. Anyway, we can carry the discussion to
v3:
https://lore.kernel.org/netdev/20250430-vsock-linger-v3-0-ddbe73b53457@rbox.co/

Note that I took the liberty to assume unsent_bytes() is always there for
loopback/virtio transports. Check for NULL is introduced when the code is
moved to core. By the end of the series it changes nothing, but I hope it's
a tiny bit more sensible.

Thanks,
Michal