[PATCH net-next v3 0/4] vsock: SOCK_LINGER rework

[PATCH net-next v3 0/4] vsock: SOCK_LINGER rework

[Michal Luczaj]

Change vsock's lingerning to wait on close() until all data is sent, i.e.
until workers picked all the packets for processing.

Changes in v3:
- Set "vsock/virtio" topic where appropriate
- Do not claim that Hyper-V and VMCI ever lingered [Stefano]
- Move lingering to af_vsock core [Stefano] 
- Link to v2: https://lore.kernel.org/r/20250421-vsock-linger-v2-0-fe9febd64668@rbox.co

Changes in v2:
- Comment that some transports do not implement unsent_bytes [Stefano]
- Reduce the indentation of virtio_transport_wait_close() [Stefano] 
- Do not linger on shutdown(), expand the commit messages [Paolo]
- Link to v1: https://lore.kernel.org/r/20250407-vsock-linger-v1-0-1458038e3492@rbox.co

Changes in v1:
- Do not assume `unsent_bytes()` is implemented by all transports [Stefano]
- Link to v0: https://lore.kernel.org/netdev/df2d51fd-03e7-477f-8aea-938446f47864@rbox.co/

Signed-off-by: Michal Luczaj <mhal@rbox.co>
---
Michal Luczaj (4):
      vsock/virtio: Linger on unsent data
      vsock/virtio: Reduce indentation in virtio_transport_wait_close()
      vsock: Move lingering logic to af_vsock core
      vsock/test: Expand linger test to ensure close() does not misbehave

 include/net/af_vsock.h                  |  1 +
 net/vmw_vsock/af_vsock.c                | 25 +++++++++++++++++++++++++
 net/vmw_vsock/virtio_transport_common.c | 19 +------------------
 tools/testing/vsock/vsock_test.c        | 30 +++++++++++++++++++++++++++---
 4 files changed, 54 insertions(+), 21 deletions(-)
---
base-commit: eed848871c96d4b5a7b06307755b75abd0cc7a06
change-id: 20250304-vsock-linger-9026e5f9986c

Best regards,
-- 
Michal Luczaj <mhal@rbox.co>

[PATCH net-next v3 1/4] vsock/virtio: Linger on unsent data

[Michal Luczaj]

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 (some interpretation of) lingering was always limited to
transports that called virtio_transport_wait_close() on transport release.
This does not change, i.e. under Hyper-V and VMCI 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 | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
index 7f7de6d8809655fe522749fbbc9025df71f071bd..49c6617b467195ba385cc3db86caa4321b422d7a 100644
--- a/net/vmw_vsock/virtio_transport_common.c
+++ b/net/vmw_vsock/virtio_transport_common.c
@@ -1196,12 +1196,16 @@ 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);
+
+		unsent = vsk->transport->unsent_bytes;
 
 		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

[PATCH net-next v3 2/4] vsock/virtio: Reduce indentation in virtio_transport_wait_close()

[Michal Luczaj]

Flatten the function. Remove the nested block by inverting the condition:
return early on !timeout.

No functional change intended.

Suggested-by: Stefano Garzarella <sgarzare@redhat.com>
Signed-off-by: Michal Luczaj <mhal@rbox.co>
---
 net/vmw_vsock/virtio_transport_common.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
index 49c6617b467195ba385cc3db86caa4321b422d7a..4425802c5d718f65aaea425ea35886ad64e2fe6e 100644
--- a/net/vmw_vsock/virtio_transport_common.c
+++ b/net/vmw_vsock/virtio_transport_common.c
@@ -1194,23 +1194,23 @@ static void virtio_transport_remove_sock(struct vsock_sock *vsk)
 
 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);
+	DEFINE_WAIT_FUNC(wait, woken_wake_function);
+	ssize_t (*unsent)(struct vsock_sock *vsk);
+	struct vsock_sock *vsk = vsock_sk(sk);
 
-		unsent = vsk->transport->unsent_bytes;
+	if (!timeout)
+		return;
 
-		add_wait_queue(sk_sleep(sk), &wait);
+	unsent = vsk->transport->unsent_bytes;
 
-		do {
-			if (sk_wait_event(sk, &timeout, unsent(vsk) == 0,
-					  &wait))
-				break;
-		} while (!signal_pending(current) && timeout);
+	add_wait_queue(sk_sleep(sk), &wait);
 
-		remove_wait_queue(sk_sleep(sk), &wait);
-	}
+	do {
+		if (sk_wait_event(sk, &timeout, unsent(vsk) == 0, &wait))
+			break;
+	} while (!signal_pending(current) && timeout);
+
+	remove_wait_queue(sk_sleep(sk), &wait);
 }
 
 static void virtio_transport_cancel_close_work(struct vsock_sock *vsk,

-- 
2.49.0

[PATCH net-next v3 3/4] vsock: Move lingering logic to af_vsock core

[Michal Luczaj]

Lingering should be transport-independent in the long run. In preparation
for supporting other transports, as well the linger on shutdown(), move
code to core.

Guard against an unimplemented vsock_transport::unsent_bytes() callback.

Suggested-by: Stefano Garzarella <sgarzare@redhat.com>
Signed-off-by: Michal Luczaj <mhal@rbox.co>
---
 include/net/af_vsock.h                  |  1 +
 net/vmw_vsock/af_vsock.c                | 25 +++++++++++++++++++++++++
 net/vmw_vsock/virtio_transport_common.c | 23 +----------------------
 3 files changed, 27 insertions(+), 22 deletions(-)

diff --git a/include/net/af_vsock.h b/include/net/af_vsock.h
index 9e85424c834353d016a527070dd62e15ff3bfce1..bd8b88d70423051dd05fc445fe37971af631ba03 100644
--- a/include/net/af_vsock.h
+++ b/include/net/af_vsock.h
@@ -221,6 +221,7 @@ void vsock_for_each_connected_socket(struct vsock_transport *transport,
 				     void (*fn)(struct sock *sk));
 int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock *psk);
 bool vsock_find_cid(unsigned int cid);
+void vsock_linger(struct sock *sk, long timeout);
 
 /**** TAP ****/
 
diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
index fc6afbc8d6806a4d98c66abc3af4bd139c583b08..946b37de679a0e68b84cd982a3af2a959c60ee57 100644
--- a/net/vmw_vsock/af_vsock.c
+++ b/net/vmw_vsock/af_vsock.c
@@ -1013,6 +1013,31 @@ static int vsock_getname(struct socket *sock,
 	return err;
 }
 
+void vsock_linger(struct sock *sk, long timeout)
+{
+	DEFINE_WAIT_FUNC(wait, woken_wake_function);
+	ssize_t (*unsent)(struct vsock_sock *vsk);
+	struct vsock_sock *vsk = vsock_sk(sk);
+
+	if (!timeout)
+		return;
+
+	/* unsent_bytes() may be unimplemented. */
+	unsent = vsk->transport->unsent_bytes;
+	if (!unsent)
+		return;
+
+	add_wait_queue(sk_sleep(sk), &wait);
+
+	do {
+		if (sk_wait_event(sk, &timeout, unsent(vsk) == 0, &wait))
+			break;
+	} while (!signal_pending(current) && timeout);
+
+	remove_wait_queue(sk_sleep(sk), &wait);
+}
+EXPORT_SYMBOL_GPL(vsock_linger);
+
 static int vsock_shutdown(struct socket *sock, int mode)
 {
 	int err;
diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
index 4425802c5d718f65aaea425ea35886ad64e2fe6e..9230b8358ef2ac1f6e72a5961bae39f9093c8884 100644
--- a/net/vmw_vsock/virtio_transport_common.c
+++ b/net/vmw_vsock/virtio_transport_common.c
@@ -1192,27 +1192,6 @@ static void virtio_transport_remove_sock(struct vsock_sock *vsk)
 	vsock_remove_sock(vsk);
 }
 
-static void virtio_transport_wait_close(struct sock *sk, long timeout)
-{
-	DEFINE_WAIT_FUNC(wait, woken_wake_function);
-	ssize_t (*unsent)(struct vsock_sock *vsk);
-	struct vsock_sock *vsk = vsock_sk(sk);
-
-	if (!timeout)
-		return;
-
-	unsent = vsk->transport->unsent_bytes;
-
-	add_wait_queue(sk_sleep(sk), &wait);
-
-	do {
-		if (sk_wait_event(sk, &timeout, unsent(vsk) == 0, &wait))
-			break;
-	} while (!signal_pending(current) && timeout);
-
-	remove_wait_queue(sk_sleep(sk), &wait);
-}
-
 static void virtio_transport_cancel_close_work(struct vsock_sock *vsk,
 					       bool cancel_timeout)
 {
@@ -1283,7 +1262,7 @@ static bool virtio_transport_close(struct vsock_sock *vsk)
 		(void)virtio_transport_shutdown(vsk, SHUTDOWN_MASK);
 
 	if (sock_flag(sk, SOCK_LINGER) && !(current->flags & PF_EXITING))
-		virtio_transport_wait_close(sk, sk->sk_lingertime);
+		vsock_linger(sk, sk->sk_lingertime);
 
 	if (sock_flag(sk, SOCK_DONE)) {
 		return true;

-- 
2.49.0

[PATCH net-next v3 4/4] vsock/test: Expand linger test to ensure close() does not misbehave

[Michal Luczaj]

There was an issue with SO_LINGER: instead of blocking until all queued
messages for the socket have been successfully sent (or the linger timeout
has been reached), close() would block until packets were handled by the
peer.

Add a check to alert on close() lingering when it should not.

Signed-off-by: Michal Luczaj <mhal@rbox.co>
---
 tools/testing/vsock/vsock_test.c | 30 +++++++++++++++++++++++++++---
 1 file changed, 27 insertions(+), 3 deletions(-)

diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c
index d0f6d253ac72d08a957cb81a3c38fcc72bec5a53..82d0bc20dfa75041f04eada1b4310be2f7c3a0c1 100644
--- a/tools/testing/vsock/vsock_test.c
+++ b/tools/testing/vsock/vsock_test.c
@@ -1788,13 +1788,16 @@ static void test_stream_connect_retry_server(const struct test_opts *opts)
 	close(fd);
 }
 
+#define	LINGER_TIMEOUT	1	/* seconds */
+
 static void test_stream_linger_client(const struct test_opts *opts)
 {
 	struct linger optval = {
 		.l_onoff = 1,
-		.l_linger = 1
+		.l_linger = LINGER_TIMEOUT
 	};
-	int fd;
+	int bytes_unsent, fd;
+	time_t ts;
 
 	fd = vsock_stream_connect(opts->peer_cid, opts->peer_port);
 	if (fd < 0) {
@@ -1807,7 +1810,28 @@ static void test_stream_linger_client(const struct test_opts *opts)
 		exit(EXIT_FAILURE);
 	}
 
+	/* Byte left unread to expose any incorrect behaviour. */
+	send_byte(fd, 1, 0);
+
+	/* Reuse LINGER_TIMEOUT to wait for bytes_unsent == 0. */
+	timeout_begin(LINGER_TIMEOUT);
+	do {
+		if (ioctl(fd, SIOCOUTQ, &bytes_unsent) < 0) {
+			perror("ioctl(SIOCOUTQ)");
+			exit(EXIT_FAILURE);
+		}
+		timeout_check("ioctl(SIOCOUTQ) == 0");
+	} while (bytes_unsent != 0);
+	timeout_end();
+
+	ts = current_nsec();
 	close(fd);
+	if ((current_nsec() - ts) / NSEC_PER_SEC > 0) {
+		fprintf(stderr, "Unexpected lingering on close()\n");
+		exit(EXIT_FAILURE);
+	}
+
+	control_writeln("DONE");
 }
 
 static void test_stream_linger_server(const struct test_opts *opts)
@@ -1820,7 +1844,7 @@ static void test_stream_linger_server(const struct test_opts *opts)
 		exit(EXIT_FAILURE);
 	}
 
-	vsock_wait_remote_close(fd);
+	control_expectln("DONE");
 	close(fd);
 }
 

-- 
2.49.0

Re: [PATCH net-next v3 1/4] vsock/virtio: Linger on unsent data

[Stefano Garzarella]

On Wed, Apr 30, 2025 at 11:10:27AM +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 (some interpretation of) lingering was always limited to
>transports that called virtio_transport_wait_close() on transport release.
>This does not change, i.e. under Hyper-V and VMCI 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 | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
>diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
>index 7f7de6d8809655fe522749fbbc9025df71f071bd..49c6617b467195ba385cc3db86caa4321b422d7a 100644
>--- a/net/vmw_vsock/virtio_transport_common.c
>+++ b/net/vmw_vsock/virtio_transport_common.c
>@@ -1196,12 +1196,16 @@ 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);
>+
>+		unsent = vsk->transport->unsent_bytes;

Just use `virtio_transport_unsent_bytes`, we don't need to be generic on 
transport here.

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

Re: [PATCH net-next v3 2/4] vsock/virtio: Reduce indentation in virtio_transport_wait_close()

[Stefano Garzarella]

On Wed, Apr 30, 2025 at 11:10:28AM +0200, Michal Luczaj wrote:
>Flatten the function. Remove the nested block by inverting the condition:
>return early on !timeout.

IIUC we are removing this function in the next commit, so we can skip 
this patch IMO. I suggested this change, if we didn't move the code in 
the core.

Thanks,
Stefano

>
>No functional change intended.
>
>Suggested-by: Stefano Garzarella <sgarzare@redhat.com>
>Signed-off-by: Michal Luczaj <mhal@rbox.co>
>---
> net/vmw_vsock/virtio_transport_common.c | 26 +++++++++++++-------------
> 1 file changed, 13 insertions(+), 13 deletions(-)
>
>diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
>index 49c6617b467195ba385cc3db86caa4321b422d7a..4425802c5d718f65aaea425ea35886ad64e2fe6e 100644
>--- a/net/vmw_vsock/virtio_transport_common.c
>+++ b/net/vmw_vsock/virtio_transport_common.c
>@@ -1194,23 +1194,23 @@ static void virtio_transport_remove_sock(struct vsock_sock *vsk)
>
> 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);
>+	DEFINE_WAIT_FUNC(wait, woken_wake_function);
>+	ssize_t (*unsent)(struct vsock_sock *vsk);
>+	struct vsock_sock *vsk = vsock_sk(sk);
>
>-		unsent = vsk->transport->unsent_bytes;
>+	if (!timeout)
>+		return;
>
>-		add_wait_queue(sk_sleep(sk), &wait);
>+	unsent = vsk->transport->unsent_bytes;
>
>-		do {
>-			if (sk_wait_event(sk, &timeout, unsent(vsk) == 0,
>-					  &wait))
>-				break;
>-		} while (!signal_pending(current) && timeout);
>+	add_wait_queue(sk_sleep(sk), &wait);
>
>-		remove_wait_queue(sk_sleep(sk), &wait);
>-	}
>+	do {
>+		if (sk_wait_event(sk, &timeout, unsent(vsk) == 0, &wait))
>+			break;
>+	} while (!signal_pending(current) && timeout);
>+
>+	remove_wait_queue(sk_sleep(sk), &wait);
> }
>
> static void virtio_transport_cancel_close_work(struct vsock_sock *vsk,
>
>-- 
>2.49.0
>

Re: [PATCH net-next v3 3/4] vsock: Move lingering logic to af_vsock core

[Stefano Garzarella]

On Wed, Apr 30, 2025 at 11:10:29AM +0200, Michal Luczaj wrote:
>Lingering should be transport-independent in the long run. In preparation
>for supporting other transports, as well the linger on shutdown(), move
>code to core.
>
>Guard against an unimplemented vsock_transport::unsent_bytes() callback.
>
>Suggested-by: Stefano Garzarella <sgarzare@redhat.com>
>Signed-off-by: Michal Luczaj <mhal@rbox.co>
>---
> include/net/af_vsock.h                  |  1 +
> net/vmw_vsock/af_vsock.c                | 25 +++++++++++++++++++++++++
> net/vmw_vsock/virtio_transport_common.c | 23 +----------------------
> 3 files changed, 27 insertions(+), 22 deletions(-)
>
>diff --git a/include/net/af_vsock.h b/include/net/af_vsock.h
>index 9e85424c834353d016a527070dd62e15ff3bfce1..bd8b88d70423051dd05fc445fe37971af631ba03 100644
>--- a/include/net/af_vsock.h
>+++ b/include/net/af_vsock.h
>@@ -221,6 +221,7 @@ void vsock_for_each_connected_socket(struct vsock_transport *transport,
> 				     void (*fn)(struct sock *sk));
> int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock *psk);
> bool vsock_find_cid(unsigned int cid);
>+void vsock_linger(struct sock *sk, long timeout);
>
> /**** TAP ****/
>
>diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
>index fc6afbc8d6806a4d98c66abc3af4bd139c583b08..946b37de679a0e68b84cd982a3af2a959c60ee57 100644
>--- a/net/vmw_vsock/af_vsock.c
>+++ b/net/vmw_vsock/af_vsock.c
>@@ -1013,6 +1013,31 @@ static int vsock_getname(struct socket *sock,
> 	return err;
> }
>
>+void vsock_linger(struct sock *sk, long timeout)
>+{
>+	DEFINE_WAIT_FUNC(wait, woken_wake_function);
>+	ssize_t (*unsent)(struct vsock_sock *vsk);
>+	struct vsock_sock *vsk = vsock_sk(sk);
>+
>+	if (!timeout)
>+		return;
>+
>+	/* unsent_bytes() may be unimplemented. */
>+	unsent = vsk->transport->unsent_bytes;
>+	if (!unsent)
>+		return;
>+
>+	add_wait_queue(sk_sleep(sk), &wait);
>+
>+	do {
>+		if (sk_wait_event(sk, &timeout, unsent(vsk) == 0, &wait))
>+			break;
>+	} while (!signal_pending(current) && timeout);
>+
>+	remove_wait_queue(sk_sleep(sk), &wait);
>+}
>+EXPORT_SYMBOL_GPL(vsock_linger);
>+
> static int vsock_shutdown(struct socket *sock, int mode)
> {
> 	int err;
>diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
>index 4425802c5d718f65aaea425ea35886ad64e2fe6e..9230b8358ef2ac1f6e72a5961bae39f9093c8884 100644
>--- a/net/vmw_vsock/virtio_transport_common.c
>+++ b/net/vmw_vsock/virtio_transport_common.c
>@@ -1192,27 +1192,6 @@ static void virtio_transport_remove_sock(struct vsock_sock *vsk)
> 	vsock_remove_sock(vsk);
> }
>
>-static void virtio_transport_wait_close(struct sock *sk, long timeout)
>-{
>-	DEFINE_WAIT_FUNC(wait, woken_wake_function);
>-	ssize_t (*unsent)(struct vsock_sock *vsk);
>-	struct vsock_sock *vsk = vsock_sk(sk);
>-
>-	if (!timeout)
>-		return;
>-
>-	unsent = vsk->transport->unsent_bytes;
>-
>-	add_wait_queue(sk_sleep(sk), &wait);
>-
>-	do {
>-		if (sk_wait_event(sk, &timeout, unsent(vsk) == 0, &wait))
>-			break;
>-	} while (!signal_pending(current) && timeout);
>-
>-	remove_wait_queue(sk_sleep(sk), &wait);
>-}
>-
> static void virtio_transport_cancel_close_work(struct vsock_sock *vsk,
> 					       bool cancel_timeout)
> {
>@@ -1283,7 +1262,7 @@ static bool virtio_transport_close(struct vsock_sock *vsk)
> 		(void)virtio_transport_shutdown(vsk, SHUTDOWN_MASK);
>
> 	if (sock_flag(sk, SOCK_LINGER) && !(current->flags & PF_EXITING))
>-		virtio_transport_wait_close(sk, sk->sk_lingertime);
>+		vsock_linger(sk, sk->sk_lingertime);

What about removing the `sk->sk_lingertime` parameter here?
vsock_linger() can get it from sk.

BTW, the change LGTM, would be great to call vsock_linger() directly in 
__vsock_release(), but we can do it later.

Thanks,
Stefano

>
> 	if (sock_flag(sk, SOCK_DONE)) {
> 		return true;
>
>-- 
>2.49.0
>
>

Re: [PATCH net-next v3 3/4] vsock: Move lingering logic to af_vsock core

[Stefano Garzarella]

On Wed, Apr 30, 2025 at 11:10:29AM +0200, Michal Luczaj wrote:
>Lingering should be transport-independent in the long run. In preparation
>for supporting other transports, as well the linger on shutdown(), move
>code to core.
>
>Guard against an unimplemented vsock_transport::unsent_bytes() callback.
>
>Suggested-by: Stefano Garzarella <sgarzare@redhat.com>
>Signed-off-by: Michal Luczaj <mhal@rbox.co>
>---
> include/net/af_vsock.h                  |  1 +
> net/vmw_vsock/af_vsock.c                | 25 +++++++++++++++++++++++++
> net/vmw_vsock/virtio_transport_common.c | 23 +----------------------
> 3 files changed, 27 insertions(+), 22 deletions(-)
>
>diff --git a/include/net/af_vsock.h b/include/net/af_vsock.h
>index 9e85424c834353d016a527070dd62e15ff3bfce1..bd8b88d70423051dd05fc445fe37971af631ba03 100644
>--- a/include/net/af_vsock.h
>+++ b/include/net/af_vsock.h
>@@ -221,6 +221,7 @@ void vsock_for_each_connected_socket(struct vsock_transport *transport,
> 				     void (*fn)(struct sock *sk));
> int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock *psk);
> bool vsock_find_cid(unsigned int cid);
>+void vsock_linger(struct sock *sk, long timeout);
>
> /**** TAP ****/
>
>diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
>index fc6afbc8d6806a4d98c66abc3af4bd139c583b08..946b37de679a0e68b84cd982a3af2a959c60ee57 100644
>--- a/net/vmw_vsock/af_vsock.c
>+++ b/net/vmw_vsock/af_vsock.c
>@@ -1013,6 +1013,31 @@ static int vsock_getname(struct socket *sock,
> 	return err;
> }
>
>+void vsock_linger(struct sock *sk, long timeout)
>+{
>+	DEFINE_WAIT_FUNC(wait, woken_wake_function);
>+	ssize_t (*unsent)(struct vsock_sock *vsk);
>+	struct vsock_sock *vsk = vsock_sk(sk);
>+
>+	if (!timeout)
>+		return;
>+
>+	/* unsent_bytes() may be unimplemented. */
>+	unsent = vsk->transport->unsent_bytes;
>+	if (!unsent)
>+		return;
>+
>+	add_wait_queue(sk_sleep(sk), &wait);
>+
>+	do {
>+		if (sk_wait_event(sk, &timeout, unsent(vsk) == 0, &wait))
>+			break;
>+	} while (!signal_pending(current) && timeout);
>+
>+	remove_wait_queue(sk_sleep(sk), &wait);
>+}
>+EXPORT_SYMBOL_GPL(vsock_linger);
>+
> static int vsock_shutdown(struct socket *sock, int mode)
> {
> 	int err;
>diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
>index 4425802c5d718f65aaea425ea35886ad64e2fe6e..9230b8358ef2ac1f6e72a5961bae39f9093c8884 100644
>--- a/net/vmw_vsock/virtio_transport_common.c
>+++ b/net/vmw_vsock/virtio_transport_common.c
>@@ -1192,27 +1192,6 @@ static void virtio_transport_remove_sock(struct vsock_sock *vsk)
> 	vsock_remove_sock(vsk);
> }
>
>-static void virtio_transport_wait_close(struct sock *sk, long timeout)
>-{
>-	DEFINE_WAIT_FUNC(wait, woken_wake_function);
>-	ssize_t (*unsent)(struct vsock_sock *vsk);
>-	struct vsock_sock *vsk = vsock_sk(sk);
>-
>-	if (!timeout)
>-		return;
>-
>-	unsent = vsk->transport->unsent_bytes;
>-
>-	add_wait_queue(sk_sleep(sk), &wait);
>-
>-	do {
>-		if (sk_wait_event(sk, &timeout, unsent(vsk) == 0, &wait))
>-			break;
>-	} while (!signal_pending(current) && timeout);
>-
>-	remove_wait_queue(sk_sleep(sk), &wait);
>-}
>-
> static void virtio_transport_cancel_close_work(struct vsock_sock *vsk,
> 					       bool cancel_timeout)
> {
>@@ -1283,7 +1262,7 @@ static bool virtio_transport_close(struct vsock_sock *vsk)
> 		(void)virtio_transport_shutdown(vsk, SHUTDOWN_MASK);
>
> 	if (sock_flag(sk, SOCK_LINGER) && !(current->flags & PF_EXITING))
>-		virtio_transport_wait_close(sk, sk->sk_lingertime);
>+		vsock_linger(sk, sk->sk_lingertime);

Ah, I'd also move the check in that function, I mean:

void vsock_linger(struct sock *sk) {
	...
	if (!sock_flag(sk, SOCK_LINGER) || (current->flags & PF_EXITING))
		return;

	...
}

Or, if we move the call to vsock_linger() in __vsock_release(), we can
do the check there.

Thanks,
Stefano

>
> 	if (sock_flag(sk, SOCK_DONE)) {
> 		return true;
>
>-- 
>2.49.0
>
>

Re: [PATCH net-next v3 1/4] vsock/virtio: Linger on unsent data

[Michal Luczaj]

On 4/30/25 11:26, Stefano Garzarella wrote:
> On Wed, Apr 30, 2025 at 11:10:27AM +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 (some interpretation of) lingering was always limited to
>> transports that called virtio_transport_wait_close() on transport release.
>> This does not change, i.e. under Hyper-V and VMCI 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 | 8 ++++++--
>> 1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
>> index 7f7de6d8809655fe522749fbbc9025df71f071bd..49c6617b467195ba385cc3db86caa4321b422d7a 100644
>> --- a/net/vmw_vsock/virtio_transport_common.c
>> +++ b/net/vmw_vsock/virtio_transport_common.c
>> @@ -1196,12 +1196,16 @@ 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);
>> +
>> +		unsent = vsk->transport->unsent_bytes;
> 
> Just use `virtio_transport_unsent_bytes`, we don't need to be generic on 
> transport here.

All right.

Thanks

Re: [PATCH net-next v3 2/4] vsock/virtio: Reduce indentation in virtio_transport_wait_close()

[Michal Luczaj]

On 4/30/25 11:28, Stefano Garzarella wrote:
> On Wed, Apr 30, 2025 at 11:10:28AM +0200, Michal Luczaj wrote:
>> Flatten the function. Remove the nested block by inverting the condition:
>> return early on !timeout.
> 
> IIUC we are removing this function in the next commit, so we can skip 
> this patch IMO. I suggested this change, if we didn't move the code in 
> the core.
Right, I remember your suggestion. Sorry, I'm still a bit uncertain as to
what should and shouldn't be done in a single commit.

Michal

Re: [PATCH net-next v3 3/4] vsock: Move lingering logic to af_vsock core

[Michal Luczaj]

On 4/30/25 11:33, Stefano Garzarella wrote:
> On Wed, Apr 30, 2025 at 11:10:29AM +0200, Michal Luczaj wrote:
>> Lingering should be transport-independent in the long run. In preparation
>> for supporting other transports, as well the linger on shutdown(), move
>> code to core.
>>
>> Guard against an unimplemented vsock_transport::unsent_bytes() callback.
>>
>> Suggested-by: Stefano Garzarella <sgarzare@redhat.com>
>> Signed-off-by: Michal Luczaj <mhal@rbox.co>
>> ---
>> include/net/af_vsock.h                  |  1 +
>> net/vmw_vsock/af_vsock.c                | 25 +++++++++++++++++++++++++
>> net/vmw_vsock/virtio_transport_common.c | 23 +----------------------
>> 3 files changed, 27 insertions(+), 22 deletions(-)
>>
>> diff --git a/include/net/af_vsock.h b/include/net/af_vsock.h
>> index 9e85424c834353d016a527070dd62e15ff3bfce1..bd8b88d70423051dd05fc445fe37971af631ba03 100644
>> --- a/include/net/af_vsock.h
>> +++ b/include/net/af_vsock.h
>> @@ -221,6 +221,7 @@ void vsock_for_each_connected_socket(struct vsock_transport *transport,
>> 				     void (*fn)(struct sock *sk));
>> int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock *psk);
>> bool vsock_find_cid(unsigned int cid);
>> +void vsock_linger(struct sock *sk, long timeout);
>>
>> /**** TAP ****/
>>
>> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
>> index fc6afbc8d6806a4d98c66abc3af4bd139c583b08..946b37de679a0e68b84cd982a3af2a959c60ee57 100644
>> --- a/net/vmw_vsock/af_vsock.c
>> +++ b/net/vmw_vsock/af_vsock.c
>> @@ -1013,6 +1013,31 @@ static int vsock_getname(struct socket *sock,
>> 	return err;
>> }
>>
>> +void vsock_linger(struct sock *sk, long timeout)
>> +{
>> +	DEFINE_WAIT_FUNC(wait, woken_wake_function);
>> +	ssize_t (*unsent)(struct vsock_sock *vsk);
>> +	struct vsock_sock *vsk = vsock_sk(sk);
>> +
>> +	if (!timeout)
>> +		return;
>> +
>> +	/* unsent_bytes() may be unimplemented. */
>> +	unsent = vsk->transport->unsent_bytes;
>> +	if (!unsent)
>> +		return;
>> +
>> +	add_wait_queue(sk_sleep(sk), &wait);
>> +
>> +	do {
>> +		if (sk_wait_event(sk, &timeout, unsent(vsk) == 0, &wait))
>> +			break;
>> +	} while (!signal_pending(current) && timeout);
>> +
>> +	remove_wait_queue(sk_sleep(sk), &wait);
>> +}
>> +EXPORT_SYMBOL_GPL(vsock_linger);
>> +
>> static int vsock_shutdown(struct socket *sock, int mode)
>> {
>> 	int err;
>> diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
>> index 4425802c5d718f65aaea425ea35886ad64e2fe6e..9230b8358ef2ac1f6e72a5961bae39f9093c8884 100644
>> --- a/net/vmw_vsock/virtio_transport_common.c
>> +++ b/net/vmw_vsock/virtio_transport_common.c
>> @@ -1192,27 +1192,6 @@ static void virtio_transport_remove_sock(struct vsock_sock *vsk)
>> 	vsock_remove_sock(vsk);
>> }
>>
>> -static void virtio_transport_wait_close(struct sock *sk, long timeout)
>> -{
>> -	DEFINE_WAIT_FUNC(wait, woken_wake_function);
>> -	ssize_t (*unsent)(struct vsock_sock *vsk);
>> -	struct vsock_sock *vsk = vsock_sk(sk);
>> -
>> -	if (!timeout)
>> -		return;
>> -
>> -	unsent = vsk->transport->unsent_bytes;
>> -
>> -	add_wait_queue(sk_sleep(sk), &wait);
>> -
>> -	do {
>> -		if (sk_wait_event(sk, &timeout, unsent(vsk) == 0, &wait))
>> -			break;
>> -	} while (!signal_pending(current) && timeout);
>> -
>> -	remove_wait_queue(sk_sleep(sk), &wait);
>> -}
>> -
>> static void virtio_transport_cancel_close_work(struct vsock_sock *vsk,
>> 					       bool cancel_timeout)
>> {
>> @@ -1283,7 +1262,7 @@ static bool virtio_transport_close(struct vsock_sock *vsk)
>> 		(void)virtio_transport_shutdown(vsk, SHUTDOWN_MASK);
>>
>> 	if (sock_flag(sk, SOCK_LINGER) && !(current->flags & PF_EXITING))
>> -		virtio_transport_wait_close(sk, sk->sk_lingertime);
>> +		vsock_linger(sk, sk->sk_lingertime);
> 
> What about removing the `sk->sk_lingertime` parameter here?
> vsock_linger() can get it from sk.

Certainly. I assume this does not need a separate patch and can be done
while moving (and de-indenting) the code?

> BTW, the change LGTM, would be great to call vsock_linger() directly in 
> __vsock_release(), but we can do it later.
> 
> Thanks,
> Stefano
> 
>>
>> 	if (sock_flag(sk, SOCK_DONE)) {
>> 		return true;
>>
>> -- 
>> 2.49.0

Re: [PATCH net-next v3 3/4] vsock: Move lingering logic to af_vsock core

[Michal Luczaj]

On 4/30/25 11:36, Stefano Garzarella wrote:
> On Wed, Apr 30, 2025 at 11:10:29AM +0200, Michal Luczaj wrote:
>> Lingering should be transport-independent in the long run. In preparation
>> for supporting other transports, as well the linger on shutdown(), move
>> code to core.
>>
>> Guard against an unimplemented vsock_transport::unsent_bytes() callback.
>>
>> Suggested-by: Stefano Garzarella <sgarzare@redhat.com>
>> Signed-off-by: Michal Luczaj <mhal@rbox.co>
>> ---
>> include/net/af_vsock.h                  |  1 +
>> net/vmw_vsock/af_vsock.c                | 25 +++++++++++++++++++++++++
>> net/vmw_vsock/virtio_transport_common.c | 23 +----------------------
>> 3 files changed, 27 insertions(+), 22 deletions(-)
>>
>> diff --git a/include/net/af_vsock.h b/include/net/af_vsock.h
>> index 9e85424c834353d016a527070dd62e15ff3bfce1..bd8b88d70423051dd05fc445fe37971af631ba03 100644
>> --- a/include/net/af_vsock.h
>> +++ b/include/net/af_vsock.h
>> @@ -221,6 +221,7 @@ void vsock_for_each_connected_socket(struct vsock_transport *transport,
>> 				     void (*fn)(struct sock *sk));
>> int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock *psk);
>> bool vsock_find_cid(unsigned int cid);
>> +void vsock_linger(struct sock *sk, long timeout);
>>
>> /**** TAP ****/
>>
>> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
>> index fc6afbc8d6806a4d98c66abc3af4bd139c583b08..946b37de679a0e68b84cd982a3af2a959c60ee57 100644
>> --- a/net/vmw_vsock/af_vsock.c
>> +++ b/net/vmw_vsock/af_vsock.c
>> @@ -1013,6 +1013,31 @@ static int vsock_getname(struct socket *sock,
>> 	return err;
>> }
>>
>> +void vsock_linger(struct sock *sk, long timeout)
>> +{
>> +	DEFINE_WAIT_FUNC(wait, woken_wake_function);
>> +	ssize_t (*unsent)(struct vsock_sock *vsk);
>> +	struct vsock_sock *vsk = vsock_sk(sk);
>> +
>> +	if (!timeout)
>> +		return;
>> +
>> +	/* unsent_bytes() may be unimplemented. */
>> +	unsent = vsk->transport->unsent_bytes;
>> +	if (!unsent)
>> +		return;
>> +
>> +	add_wait_queue(sk_sleep(sk), &wait);
>> +
>> +	do {
>> +		if (sk_wait_event(sk, &timeout, unsent(vsk) == 0, &wait))
>> +			break;
>> +	} while (!signal_pending(current) && timeout);
>> +
>> +	remove_wait_queue(sk_sleep(sk), &wait);
>> +}
>> +EXPORT_SYMBOL_GPL(vsock_linger);
>> +
>> static int vsock_shutdown(struct socket *sock, int mode)
>> {
>> 	int err;
>> diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
>> index 4425802c5d718f65aaea425ea35886ad64e2fe6e..9230b8358ef2ac1f6e72a5961bae39f9093c8884 100644
>> --- a/net/vmw_vsock/virtio_transport_common.c
>> +++ b/net/vmw_vsock/virtio_transport_common.c
>> @@ -1192,27 +1192,6 @@ static void virtio_transport_remove_sock(struct vsock_sock *vsk)
>> 	vsock_remove_sock(vsk);
>> }
>>
>> -static void virtio_transport_wait_close(struct sock *sk, long timeout)
>> -{
>> -	DEFINE_WAIT_FUNC(wait, woken_wake_function);
>> -	ssize_t (*unsent)(struct vsock_sock *vsk);
>> -	struct vsock_sock *vsk = vsock_sk(sk);
>> -
>> -	if (!timeout)
>> -		return;
>> -
>> -	unsent = vsk->transport->unsent_bytes;
>> -
>> -	add_wait_queue(sk_sleep(sk), &wait);
>> -
>> -	do {
>> -		if (sk_wait_event(sk, &timeout, unsent(vsk) == 0, &wait))
>> -			break;
>> -	} while (!signal_pending(current) && timeout);
>> -
>> -	remove_wait_queue(sk_sleep(sk), &wait);
>> -}
>> -
>> static void virtio_transport_cancel_close_work(struct vsock_sock *vsk,
>> 					       bool cancel_timeout)
>> {
>> @@ -1283,7 +1262,7 @@ static bool virtio_transport_close(struct vsock_sock *vsk)
>> 		(void)virtio_transport_shutdown(vsk, SHUTDOWN_MASK);
>>
>> 	if (sock_flag(sk, SOCK_LINGER) && !(current->flags & PF_EXITING))
>> -		virtio_transport_wait_close(sk, sk->sk_lingertime);
>> +		vsock_linger(sk, sk->sk_lingertime);
> 
> Ah, I'd also move the check in that function, I mean:
> 
> void vsock_linger(struct sock *sk) {
> 	...
> 	if (!sock_flag(sk, SOCK_LINGER) || (current->flags & PF_EXITING))
> 		return;
> 
> 	...
> }

One note: if we ever use vsock_linger() in vsock_shutdown(), the PF_EXITING
condition would be unnecessary checked for that caller, right?

> Or, if we move the call to vsock_linger() in __vsock_release(), we can
> do the check there.
> 
> Thanks,
> Stefano
> 
>>
>> 	if (sock_flag(sk, SOCK_DONE)) {
>> 		return true;
>>
>> -- 
>> 2.49.0
>>
>>

Re: [PATCH net-next v3 2/4] vsock/virtio: Reduce indentation in virtio_transport_wait_close()

[Stefano Garzarella]

On Wed, 30 Apr 2025 at 12:30, Michal Luczaj <mhal@rbox.co> wrote:
>
> On 4/30/25 11:28, Stefano Garzarella wrote:
> > On Wed, Apr 30, 2025 at 11:10:28AM +0200, Michal Luczaj wrote:
> >> Flatten the function. Remove the nested block by inverting the condition:
> >> return early on !timeout.
> >
> > IIUC we are removing this function in the next commit, so we can skip
> > this patch IMO. I suggested this change, if we didn't move the code in
> > the core.
> Right, I remember your suggestion. Sorry, I'm still a bit uncertain as to
> what should and shouldn't be done in a single commit.

Sorry for the confusion :-)

The rule I usually follow is this (but may not be the perfect one):
- try to make the fewest changes in a commit, to simplify both
backports, but also for debug/revert/bisection/etc.
- when I move code around and edit it a bit, then it's okay to edit
style, comments, etc.

Thanks,
Stefano

Re: [PATCH net-next v3 3/4] vsock: Move lingering logic to af_vsock core

[Stefano Garzarella]

On Wed, 30 Apr 2025 at 12:30, Michal Luczaj <mhal@rbox.co> wrote:
>
> On 4/30/25 11:33, Stefano Garzarella wrote:
> > On Wed, Apr 30, 2025 at 11:10:29AM +0200, Michal Luczaj wrote:
> >> Lingering should be transport-independent in the long run. In preparation
> >> for supporting other transports, as well the linger on shutdown(), move
> >> code to core.
> >>
> >> Guard against an unimplemented vsock_transport::unsent_bytes() callback.
> >>
> >> Suggested-by: Stefano Garzarella <sgarzare@redhat.com>
> >> Signed-off-by: Michal Luczaj <mhal@rbox.co>
> >> ---
> >> include/net/af_vsock.h                  |  1 +
> >> net/vmw_vsock/af_vsock.c                | 25 +++++++++++++++++++++++++
> >> net/vmw_vsock/virtio_transport_common.c | 23 +----------------------
> >> 3 files changed, 27 insertions(+), 22 deletions(-)
> >>
> >> diff --git a/include/net/af_vsock.h b/include/net/af_vsock.h
> >> index 9e85424c834353d016a527070dd62e15ff3bfce1..bd8b88d70423051dd05fc445fe37971af631ba03 100644
> >> --- a/include/net/af_vsock.h
> >> +++ b/include/net/af_vsock.h
> >> @@ -221,6 +221,7 @@ void vsock_for_each_connected_socket(struct vsock_transport *transport,
> >>                                   void (*fn)(struct sock *sk));
> >> int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock *psk);
> >> bool vsock_find_cid(unsigned int cid);
> >> +void vsock_linger(struct sock *sk, long timeout);
> >>
> >> /**** TAP ****/
> >>
> >> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
> >> index fc6afbc8d6806a4d98c66abc3af4bd139c583b08..946b37de679a0e68b84cd982a3af2a959c60ee57 100644
> >> --- a/net/vmw_vsock/af_vsock.c
> >> +++ b/net/vmw_vsock/af_vsock.c
> >> @@ -1013,6 +1013,31 @@ static int vsock_getname(struct socket *sock,
> >>      return err;
> >> }
> >>
> >> +void vsock_linger(struct sock *sk, long timeout)
> >> +{
> >> +    DEFINE_WAIT_FUNC(wait, woken_wake_function);
> >> +    ssize_t (*unsent)(struct vsock_sock *vsk);
> >> +    struct vsock_sock *vsk = vsock_sk(sk);
> >> +
> >> +    if (!timeout)
> >> +            return;
> >> +
> >> +    /* unsent_bytes() may be unimplemented. */
> >> +    unsent = vsk->transport->unsent_bytes;
> >> +    if (!unsent)
> >> +            return;
> >> +
> >> +    add_wait_queue(sk_sleep(sk), &wait);
> >> +
> >> +    do {
> >> +            if (sk_wait_event(sk, &timeout, unsent(vsk) == 0, &wait))
> >> +                    break;
> >> +    } while (!signal_pending(current) && timeout);
> >> +
> >> +    remove_wait_queue(sk_sleep(sk), &wait);
> >> +}
> >> +EXPORT_SYMBOL_GPL(vsock_linger);
> >> +
> >> static int vsock_shutdown(struct socket *sock, int mode)
> >> {
> >>      int err;
> >> diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
> >> index 4425802c5d718f65aaea425ea35886ad64e2fe6e..9230b8358ef2ac1f6e72a5961bae39f9093c8884 100644
> >> --- a/net/vmw_vsock/virtio_transport_common.c
> >> +++ b/net/vmw_vsock/virtio_transport_common.c
> >> @@ -1192,27 +1192,6 @@ static void virtio_transport_remove_sock(struct vsock_sock *vsk)
> >>      vsock_remove_sock(vsk);
> >> }
> >>
> >> -static void virtio_transport_wait_close(struct sock *sk, long timeout)
> >> -{
> >> -    DEFINE_WAIT_FUNC(wait, woken_wake_function);
> >> -    ssize_t (*unsent)(struct vsock_sock *vsk);
> >> -    struct vsock_sock *vsk = vsock_sk(sk);
> >> -
> >> -    if (!timeout)
> >> -            return;
> >> -
> >> -    unsent = vsk->transport->unsent_bytes;
> >> -
> >> -    add_wait_queue(sk_sleep(sk), &wait);
> >> -
> >> -    do {
> >> -            if (sk_wait_event(sk, &timeout, unsent(vsk) == 0, &wait))
> >> -                    break;
> >> -    } while (!signal_pending(current) && timeout);
> >> -
> >> -    remove_wait_queue(sk_sleep(sk), &wait);
> >> -}
> >> -
> >> static void virtio_transport_cancel_close_work(struct vsock_sock *vsk,
> >>                                             bool cancel_timeout)
> >> {
> >> @@ -1283,7 +1262,7 @@ static bool virtio_transport_close(struct vsock_sock *vsk)
> >>              (void)virtio_transport_shutdown(vsk, SHUTDOWN_MASK);
> >>
> >>      if (sock_flag(sk, SOCK_LINGER) && !(current->flags & PF_EXITING))
> >> -            virtio_transport_wait_close(sk, sk->sk_lingertime);
> >> +            vsock_linger(sk, sk->sk_lingertime);
> >
> > What about removing the `sk->sk_lingertime` parameter here?
> > vsock_linger() can get it from sk.
>
> Certainly. I assume this does not need a separate patch and can be done
> while moving (and de-indenting) the code?

Yep, single patch to implement vsock_linger() in af_vsock.c is fine.

Stefano

Re: [PATCH net-next v3 3/4] vsock: Move lingering logic to af_vsock core

[Stefano Garzarella]

On Wed, 30 Apr 2025 at 12:33, Michal Luczaj <mhal@rbox.co> wrote:
>
> On 4/30/25 11:36, Stefano Garzarella wrote:
> > On Wed, Apr 30, 2025 at 11:10:29AM +0200, Michal Luczaj wrote:
> >> Lingering should be transport-independent in the long run. In preparation
> >> for supporting other transports, as well the linger on shutdown(), move
> >> code to core.
> >>
> >> Guard against an unimplemented vsock_transport::unsent_bytes() callback.
> >>
> >> Suggested-by: Stefano Garzarella <sgarzare@redhat.com>
> >> Signed-off-by: Michal Luczaj <mhal@rbox.co>
> >> ---
> >> include/net/af_vsock.h                  |  1 +
> >> net/vmw_vsock/af_vsock.c                | 25 +++++++++++++++++++++++++
> >> net/vmw_vsock/virtio_transport_common.c | 23 +----------------------
> >> 3 files changed, 27 insertions(+), 22 deletions(-)
> >>
> >> diff --git a/include/net/af_vsock.h b/include/net/af_vsock.h
> >> index 9e85424c834353d016a527070dd62e15ff3bfce1..bd8b88d70423051dd05fc445fe37971af631ba03 100644
> >> --- a/include/net/af_vsock.h
> >> +++ b/include/net/af_vsock.h
> >> @@ -221,6 +221,7 @@ void vsock_for_each_connected_socket(struct vsock_transport *transport,
> >>                                   void (*fn)(struct sock *sk));
> >> int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock *psk);
> >> bool vsock_find_cid(unsigned int cid);
> >> +void vsock_linger(struct sock *sk, long timeout);
> >>
> >> /**** TAP ****/
> >>
> >> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
> >> index fc6afbc8d6806a4d98c66abc3af4bd139c583b08..946b37de679a0e68b84cd982a3af2a959c60ee57 100644
> >> --- a/net/vmw_vsock/af_vsock.c
> >> +++ b/net/vmw_vsock/af_vsock.c
> >> @@ -1013,6 +1013,31 @@ static int vsock_getname(struct socket *sock,
> >>      return err;
> >> }
> >>
> >> +void vsock_linger(struct sock *sk, long timeout)
> >> +{
> >> +    DEFINE_WAIT_FUNC(wait, woken_wake_function);
> >> +    ssize_t (*unsent)(struct vsock_sock *vsk);
> >> +    struct vsock_sock *vsk = vsock_sk(sk);
> >> +
> >> +    if (!timeout)
> >> +            return;
> >> +
> >> +    /* unsent_bytes() may be unimplemented. */
> >> +    unsent = vsk->transport->unsent_bytes;
> >> +    if (!unsent)
> >> +            return;
> >> +
> >> +    add_wait_queue(sk_sleep(sk), &wait);
> >> +
> >> +    do {
> >> +            if (sk_wait_event(sk, &timeout, unsent(vsk) == 0, &wait))
> >> +                    break;
> >> +    } while (!signal_pending(current) && timeout);
> >> +
> >> +    remove_wait_queue(sk_sleep(sk), &wait);
> >> +}
> >> +EXPORT_SYMBOL_GPL(vsock_linger);
> >> +
> >> static int vsock_shutdown(struct socket *sock, int mode)
> >> {
> >>      int err;
> >> diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
> >> index 4425802c5d718f65aaea425ea35886ad64e2fe6e..9230b8358ef2ac1f6e72a5961bae39f9093c8884 100644
> >> --- a/net/vmw_vsock/virtio_transport_common.c
> >> +++ b/net/vmw_vsock/virtio_transport_common.c
> >> @@ -1192,27 +1192,6 @@ static void virtio_transport_remove_sock(struct vsock_sock *vsk)
> >>      vsock_remove_sock(vsk);
> >> }
> >>
> >> -static void virtio_transport_wait_close(struct sock *sk, long timeout)
> >> -{
> >> -    DEFINE_WAIT_FUNC(wait, woken_wake_function);
> >> -    ssize_t (*unsent)(struct vsock_sock *vsk);
> >> -    struct vsock_sock *vsk = vsock_sk(sk);
> >> -
> >> -    if (!timeout)
> >> -            return;
> >> -
> >> -    unsent = vsk->transport->unsent_bytes;
> >> -
> >> -    add_wait_queue(sk_sleep(sk), &wait);
> >> -
> >> -    do {
> >> -            if (sk_wait_event(sk, &timeout, unsent(vsk) == 0, &wait))
> >> -                    break;
> >> -    } while (!signal_pending(current) && timeout);
> >> -
> >> -    remove_wait_queue(sk_sleep(sk), &wait);
> >> -}
> >> -
> >> static void virtio_transport_cancel_close_work(struct vsock_sock *vsk,
> >>                                             bool cancel_timeout)
> >> {
> >> @@ -1283,7 +1262,7 @@ static bool virtio_transport_close(struct vsock_sock *vsk)
> >>              (void)virtio_transport_shutdown(vsk, SHUTDOWN_MASK);
> >>
> >>      if (sock_flag(sk, SOCK_LINGER) && !(current->flags & PF_EXITING))
> >> -            virtio_transport_wait_close(sk, sk->sk_lingertime);
> >> +            vsock_linger(sk, sk->sk_lingertime);
> >
> > Ah, I'd also move the check in that function, I mean:
> >
> > void vsock_linger(struct sock *sk) {
> >       ...
> >       if (!sock_flag(sk, SOCK_LINGER) || (current->flags & PF_EXITING))
> >               return;
> >
> >       ...
> > }
>
> One note: if we ever use vsock_linger() in vsock_shutdown(), the PF_EXITING
> condition would be unnecessary checked for that caller, right?

Right, for shutdown it should always be false, so maybe better to keep
the check in the caller.

Thanks,
Stefano

>
> > Or, if we move the call to vsock_linger() in __vsock_release(), we can
> > do the check there.
> >
> > Thanks,
> > Stefano
> >
> >>
> >>      if (sock_flag(sk, SOCK_DONE)) {
> >>              return true;
> >>
> >> --
> >> 2.49.0
> >>
> >>
>

Re: [PATCH net-next v3 3/4] vsock: Move lingering logic to af_vsock core

[Michal Luczaj]

On 4/30/25 12:37, Stefano Garzarella wrote:
> On Wed, 30 Apr 2025 at 12:33, Michal Luczaj <mhal@rbox.co> wrote:
>>
>> On 4/30/25 11:36, Stefano Garzarella wrote:
>>> On Wed, Apr 30, 2025 at 11:10:29AM +0200, Michal Luczaj wrote:
>>>> Lingering should be transport-independent in the long run. In preparation
>>>> for supporting other transports, as well the linger on shutdown(), move
>>>> code to core.
>>>>
>>>> Guard against an unimplemented vsock_transport::unsent_bytes() callback.
>>>>
>>>> Suggested-by: Stefano Garzarella <sgarzare@redhat.com>
>>>> Signed-off-by: Michal Luczaj <mhal@rbox.co>
>>>> ---
>>>> include/net/af_vsock.h                  |  1 +
>>>> net/vmw_vsock/af_vsock.c                | 25 +++++++++++++++++++++++++
>>>> net/vmw_vsock/virtio_transport_common.c | 23 +----------------------
>>>> 3 files changed, 27 insertions(+), 22 deletions(-)
>>>>
>>>> diff --git a/include/net/af_vsock.h b/include/net/af_vsock.h
>>>> index 9e85424c834353d016a527070dd62e15ff3bfce1..bd8b88d70423051dd05fc445fe37971af631ba03 100644
>>>> --- a/include/net/af_vsock.h
>>>> +++ b/include/net/af_vsock.h
>>>> @@ -221,6 +221,7 @@ void vsock_for_each_connected_socket(struct vsock_transport *transport,
>>>>                                   void (*fn)(struct sock *sk));
>>>> int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock *psk);
>>>> bool vsock_find_cid(unsigned int cid);
>>>> +void vsock_linger(struct sock *sk, long timeout);
>>>>
>>>> /**** TAP ****/
>>>>
>>>> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
>>>> index fc6afbc8d6806a4d98c66abc3af4bd139c583b08..946b37de679a0e68b84cd982a3af2a959c60ee57 100644
>>>> --- a/net/vmw_vsock/af_vsock.c
>>>> +++ b/net/vmw_vsock/af_vsock.c
>>>> @@ -1013,6 +1013,31 @@ static int vsock_getname(struct socket *sock,
>>>>      return err;
>>>> }
>>>>
>>>> +void vsock_linger(struct sock *sk, long timeout)
>>>> +{
>>>> +    DEFINE_WAIT_FUNC(wait, woken_wake_function);
>>>> +    ssize_t (*unsent)(struct vsock_sock *vsk);
>>>> +    struct vsock_sock *vsk = vsock_sk(sk);
>>>> +
>>>> +    if (!timeout)
>>>> +            return;
>>>> +
>>>> +    /* unsent_bytes() may be unimplemented. */
>>>> +    unsent = vsk->transport->unsent_bytes;
>>>> +    if (!unsent)
>>>> +            return;
>>>> +
>>>> +    add_wait_queue(sk_sleep(sk), &wait);
>>>> +
>>>> +    do {
>>>> +            if (sk_wait_event(sk, &timeout, unsent(vsk) == 0, &wait))
>>>> +                    break;
>>>> +    } while (!signal_pending(current) && timeout);
>>>> +
>>>> +    remove_wait_queue(sk_sleep(sk), &wait);
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(vsock_linger);
>>>> +
>>>> static int vsock_shutdown(struct socket *sock, int mode)
>>>> {
>>>>      int err;
>>>> diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
>>>> index 4425802c5d718f65aaea425ea35886ad64e2fe6e..9230b8358ef2ac1f6e72a5961bae39f9093c8884 100644
>>>> --- a/net/vmw_vsock/virtio_transport_common.c
>>>> +++ b/net/vmw_vsock/virtio_transport_common.c
>>>> @@ -1192,27 +1192,6 @@ static void virtio_transport_remove_sock(struct vsock_sock *vsk)
>>>>      vsock_remove_sock(vsk);
>>>> }
>>>>
>>>> -static void virtio_transport_wait_close(struct sock *sk, long timeout)
>>>> -{
>>>> -    DEFINE_WAIT_FUNC(wait, woken_wake_function);
>>>> -    ssize_t (*unsent)(struct vsock_sock *vsk);
>>>> -    struct vsock_sock *vsk = vsock_sk(sk);
>>>> -
>>>> -    if (!timeout)
>>>> -            return;
>>>> -
>>>> -    unsent = vsk->transport->unsent_bytes;
>>>> -
>>>> -    add_wait_queue(sk_sleep(sk), &wait);
>>>> -
>>>> -    do {
>>>> -            if (sk_wait_event(sk, &timeout, unsent(vsk) == 0, &wait))
>>>> -                    break;
>>>> -    } while (!signal_pending(current) && timeout);
>>>> -
>>>> -    remove_wait_queue(sk_sleep(sk), &wait);
>>>> -}
>>>> -
>>>> static void virtio_transport_cancel_close_work(struct vsock_sock *vsk,
>>>>                                             bool cancel_timeout)
>>>> {
>>>> @@ -1283,7 +1262,7 @@ static bool virtio_transport_close(struct vsock_sock *vsk)
>>>>              (void)virtio_transport_shutdown(vsk, SHUTDOWN_MASK);
>>>>
>>>>      if (sock_flag(sk, SOCK_LINGER) && !(current->flags & PF_EXITING))
>>>> -            virtio_transport_wait_close(sk, sk->sk_lingertime);
>>>> +            vsock_linger(sk, sk->sk_lingertime);
>>>
>>> Ah, I'd also move the check in that function, I mean:
>>>
>>> void vsock_linger(struct sock *sk) {
>>>       ...
>>>       if (!sock_flag(sk, SOCK_LINGER) || (current->flags & PF_EXITING))
>>>               return;
>>>
>>>       ...
>>> }
>>
>> One note: if we ever use vsock_linger() in vsock_shutdown(), the PF_EXITING
>> condition would be unnecessary checked for that caller, right?
> 
> Right, for shutdown it should always be false, so maybe better to keep
> the check in the caller.

Or split it? Check `!sock_flag(sk, SOCK_LINGER) || !timeout` in
vsock_linger() and defer `!(flags & PF_EXITING)` to whoever does the socket
release?

Re: [PATCH net-next v3 3/4] vsock: Move lingering logic to af_vsock core

[Stefano Garzarella]

On Wed, Apr 30, 2025 at 01:11:48PM +0200, Michal Luczaj wrote:
>On 4/30/25 12:37, Stefano Garzarella wrote:
>> On Wed, 30 Apr 2025 at 12:33, Michal Luczaj <mhal@rbox.co> wrote:
>>>
>>> On 4/30/25 11:36, Stefano Garzarella wrote:
>>>> On Wed, Apr 30, 2025 at 11:10:29AM +0200, Michal Luczaj wrote:
>>>>> Lingering should be transport-independent in the long run. In preparation
>>>>> for supporting other transports, as well the linger on shutdown(), move
>>>>> code to core.
>>>>>
>>>>> Guard against an unimplemented vsock_transport::unsent_bytes() callback.
>>>>>
>>>>> Suggested-by: Stefano Garzarella <sgarzare@redhat.com>
>>>>> Signed-off-by: Michal Luczaj <mhal@rbox.co>
>>>>> ---
>>>>> include/net/af_vsock.h                  |  1 +
>>>>> net/vmw_vsock/af_vsock.c                | 25 +++++++++++++++++++++++++
>>>>> net/vmw_vsock/virtio_transport_common.c | 23 +----------------------
>>>>> 3 files changed, 27 insertions(+), 22 deletions(-)
>>>>>
>>>>> diff --git a/include/net/af_vsock.h b/include/net/af_vsock.h
>>>>> index 9e85424c834353d016a527070dd62e15ff3bfce1..bd8b88d70423051dd05fc445fe37971af631ba03 100644
>>>>> --- a/include/net/af_vsock.h
>>>>> +++ b/include/net/af_vsock.h
>>>>> @@ -221,6 +221,7 @@ void vsock_for_each_connected_socket(struct vsock_transport *transport,
>>>>>                                   void (*fn)(struct sock *sk));
>>>>> int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock *psk);
>>>>> bool vsock_find_cid(unsigned int cid);
>>>>> +void vsock_linger(struct sock *sk, long timeout);
>>>>>
>>>>> /**** TAP ****/
>>>>>
>>>>> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
>>>>> index fc6afbc8d6806a4d98c66abc3af4bd139c583b08..946b37de679a0e68b84cd982a3af2a959c60ee57 100644
>>>>> --- a/net/vmw_vsock/af_vsock.c
>>>>> +++ b/net/vmw_vsock/af_vsock.c
>>>>> @@ -1013,6 +1013,31 @@ static int vsock_getname(struct socket *sock,
>>>>>      return err;
>>>>> }
>>>>>
>>>>> +void vsock_linger(struct sock *sk, long timeout)
>>>>> +{
>>>>> +    DEFINE_WAIT_FUNC(wait, woken_wake_function);
>>>>> +    ssize_t (*unsent)(struct vsock_sock *vsk);
>>>>> +    struct vsock_sock *vsk = vsock_sk(sk);
>>>>> +
>>>>> +    if (!timeout)
>>>>> +            return;
>>>>> +
>>>>> +    /* unsent_bytes() may be unimplemented. */
>>>>> +    unsent = vsk->transport->unsent_bytes;
>>>>> +    if (!unsent)
>>>>> +            return;
>>>>> +
>>>>> +    add_wait_queue(sk_sleep(sk), &wait);
>>>>> +
>>>>> +    do {
>>>>> +            if (sk_wait_event(sk, &timeout, unsent(vsk) == 0, &wait))
>>>>> +                    break;
>>>>> +    } while (!signal_pending(current) && timeout);
>>>>> +
>>>>> +    remove_wait_queue(sk_sleep(sk), &wait);
>>>>> +}
>>>>> +EXPORT_SYMBOL_GPL(vsock_linger);
>>>>> +
>>>>> static int vsock_shutdown(struct socket *sock, int mode)
>>>>> {
>>>>>      int err;
>>>>> diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
>>>>> index 4425802c5d718f65aaea425ea35886ad64e2fe6e..9230b8358ef2ac1f6e72a5961bae39f9093c8884 100644
>>>>> --- a/net/vmw_vsock/virtio_transport_common.c
>>>>> +++ b/net/vmw_vsock/virtio_transport_common.c
>>>>> @@ -1192,27 +1192,6 @@ static void virtio_transport_remove_sock(struct vsock_sock *vsk)
>>>>>      vsock_remove_sock(vsk);
>>>>> }
>>>>>
>>>>> -static void virtio_transport_wait_close(struct sock *sk, long timeout)
>>>>> -{
>>>>> -    DEFINE_WAIT_FUNC(wait, woken_wake_function);
>>>>> -    ssize_t (*unsent)(struct vsock_sock *vsk);
>>>>> -    struct vsock_sock *vsk = vsock_sk(sk);
>>>>> -
>>>>> -    if (!timeout)
>>>>> -            return;
>>>>> -
>>>>> -    unsent = vsk->transport->unsent_bytes;
>>>>> -
>>>>> -    add_wait_queue(sk_sleep(sk), &wait);
>>>>> -
>>>>> -    do {
>>>>> -            if (sk_wait_event(sk, &timeout, unsent(vsk) == 0, &wait))
>>>>> -                    break;
>>>>> -    } while (!signal_pending(current) && timeout);
>>>>> -
>>>>> -    remove_wait_queue(sk_sleep(sk), &wait);
>>>>> -}
>>>>> -
>>>>> static void virtio_transport_cancel_close_work(struct vsock_sock *vsk,
>>>>>                                             bool cancel_timeout)
>>>>> {
>>>>> @@ -1283,7 +1262,7 @@ static bool virtio_transport_close(struct vsock_sock *vsk)
>>>>>              (void)virtio_transport_shutdown(vsk, SHUTDOWN_MASK);
>>>>>
>>>>>      if (sock_flag(sk, SOCK_LINGER) && !(current->flags & PF_EXITING))
>>>>> -            virtio_transport_wait_close(sk, sk->sk_lingertime);
>>>>> +            vsock_linger(sk, sk->sk_lingertime);
>>>>
>>>> Ah, I'd also move the check in that function, I mean:
>>>>
>>>> void vsock_linger(struct sock *sk) {
>>>>       ...
>>>>       if (!sock_flag(sk, SOCK_LINGER) || (current->flags & PF_EXITING))
>>>>               return;
>>>>
>>>>       ...
>>>> }
>>>
>>> One note: if we ever use vsock_linger() in vsock_shutdown(), the PF_EXITING
>>> condition would be unnecessary checked for that caller, right?
>>
>> Right, for shutdown it should always be false, so maybe better to keep
>> the check in the caller.
>
>Or split it? Check `!sock_flag(sk, SOCK_LINGER) || !timeout` in
>vsock_linger() and defer `!(flags & PF_EXITING)` to whoever does the socket
>release?
>

Yep, this is also fine with me!

Stefano