On Thu, Feb 15, 2024 at 01:20:15PM +0100, Alexander Bluhm wrote:
> On Thu, Feb 15, 2024 at 12:28:28PM +0300, Vitaliy Makkoveev wrote:
> > Use barriers instead of `ssp_idleto' task and timeout(9)
> > re-initialization to wait concurrent somove() and soidle().
> 
> Why do you move the unsplice code before the solinger sleep?
> 
> Are you sure that you don't change the semantics?  Is all data still
> spliced before the connection closes?
> 
> bluhm
> 

The socket is inaccessible from userland, but SO_LINGER keeps it alive,
mostly to perform transmission of pending data. The new transmissions
and the reception are impossible. However, this is not true for spliced
sockets because they are still accessible by the spliced peer, but not
forever.

IIRC, only tcp_ctlinput() will wakeup us, so we leave this loop after
TCP_LINGERTIME or if the remote peer was disconnected.

So, for the spliced case, you can receive data and awake somove(), but
reach `so_linger' timeout and follow the destruction path. Or you could
pru_send() data to dying `sosp' which already reached `so_linger'
timeout. You don't know is your spliced peer closed, so I see no
difference, will it be unspliced now or after TCP_LINGERTIME seconds.

Since the pru_detach() is not called, the remote also doesn't know that
socket is closed.

I wanted to move it from sofree(). Since I see no differences, the idea
to do pru_detach() after disconnect and unsplice looked good.


This diff keeps current notation, so the unsplice moved before sofree().

Index: sys/kern/uipc_socket.c
===================================================================
RCS file: /cvs/src/sys/kern/uipc_socket.c,v
retrieving revision 1.320
diff -u -p -r1.320 uipc_socket.c
--- sys/kern/uipc_socket.c	12 Feb 2024 22:48:27 -0000	1.320
+++ sys/kern/uipc_socket.c	15 Feb 2024 16:24:32 -0000
@@ -62,8 +62,6 @@ int	sosplice(struct socket *, int, off_t
 void	sounsplice(struct socket *, struct socket *, int);
 void	soidle(void *);
 void	sotask(void *);
-void	soreaper(void *);
-void	soput(void *);
 int	somove(struct socket *, int);
 void	sorflush(struct socket *);
 
@@ -250,8 +248,6 @@ solisten(struct socket *so, int backlog)
 	return (0);
 }
 
-#define SOSP_FREEING_READ	1
-#define SOSP_FREEING_WRITE	2
 void
 sofree(struct socket *so, int keep_lock)
 {
@@ -316,36 +312,11 @@ sofree(struct socket *so, int keep_lock)
 	sigio_free(&so->so_sigio);
 	klist_free(&so->so_rcv.sb_klist);
 	klist_free(&so->so_snd.sb_klist);
-#ifdef SOCKET_SPLICE
-	if (issplicedback(so)) {
-		int freeing = SOSP_FREEING_WRITE;
-
-		if (so->so_sp->ssp_soback == so)
-			freeing |= SOSP_FREEING_READ;
-		sounsplice(so->so_sp->ssp_soback, so, freeing);
-	}
-	if (isspliced(so)) {
-		int freeing = SOSP_FREEING_READ;
-
-		if (so == so->so_sp->ssp_socket)
-			freeing |= SOSP_FREEING_WRITE;
-		sounsplice(so, so->so_sp->ssp_socket, freeing);
-	}
-#endif /* SOCKET_SPLICE */
 	sbrelease(so, &so->so_snd);
 	sorflush(so);
 	if (!keep_lock)
 		sounlock(so);
-#ifdef SOCKET_SPLICE
-	if (so->so_sp) {
-		/* Reuse splice idle, sounsplice() has been called before. */
-		timeout_set_proc(&so->so_sp->ssp_idleto, soreaper, so);
-		timeout_add(&so->so_sp->ssp_idleto, 0);
-	} else
-#endif /* SOCKET_SPLICE */
-	{
-		pool_put(&socket_pool, so);
-	}
+	pool_put(&socket_pool, so);
 }
 
 static inline uint64_t
@@ -357,6 +328,8 @@ solinger_nsec(struct socket *so)
 	return SEC_TO_NSEC(so->so_linger);
 }
 
+#define SOSP_FREEING_READ	1
+#define SOSP_FREEING_WRITE	2
 /*
  * Close a socket on last file table reference removal.
  * Initiate disconnect if connected.
@@ -436,6 +409,31 @@ discard:
 	if (so->so_state & SS_NOFDREF)
 		panic("soclose NOFDREF: so %p, so_type %d", so, so->so_type);
 	so->so_state |= SS_NOFDREF;
+
+#ifdef SOCKET_SPLICE
+	if (issplicedback(so)) {
+		int freeing = SOSP_FREEING_WRITE;
+
+		if (so->so_sp->ssp_soback == so)
+			freeing |= SOSP_FREEING_READ;
+		sounsplice(so->so_sp->ssp_soback, so, freeing);
+	}
+	if (isspliced(so)) {
+		int freeing = SOSP_FREEING_READ;
+
+		if (so == so->so_sp->ssp_socket)
+			freeing |= SOSP_FREEING_WRITE;
+		sounsplice(so, so->so_sp->ssp_socket, freeing);
+	}
+	if (so->so_sp) {
+		sounlock(so);
+		taskq_barrier(sosplice_taskq);
+		timeout_del_barrier(&so->so_sp->ssp_idleto);
+		pool_put(&sosplice_pool, so->so_sp);
+		solock(so);
+	}
+#endif /* SOCKET_SPLICE */
+
 	/* sofree() calls sounlock(). */
 	sofree(so, 0);
 	return (error);
@@ -1441,32 +1439,6 @@ sotask(void *arg)
 
 	/* Avoid user land starvation. */
 	yield();
-}
-
-/*
- * The socket splicing task or idle timeout may sleep while grabbing the net
- * lock.  As sofree() can be called anytime, sotask() or soidle() could access
- * the socket memory of a freed socket after wakeup.  So delay the pool_put()
- * after all pending socket splicing tasks or timeouts have finished.  Do this
- * by scheduling it on the same threads.
- */
-void
-soreaper(void *arg)
-{
-	struct socket *so = arg;
-
-	/* Reuse splice task, sounsplice() has been called before. */
-	task_set(&so->so_sp->ssp_task, soput, so);
-	task_add(sosplice_taskq, &so->so_sp->ssp_task);
-}
-
-void
-soput(void *arg)
-{
-	struct socket *so = arg;
-
-	pool_put(&sosplice_pool, so->so_sp);
-	pool_put(&socket_pool, so);
 }
 
 /*