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 <m...@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 <sgarz...@redhat.com>
Signed-off-by: Michal Luczaj <m...@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