Re: git: fb901935f257 - main - socket: Split up sosend_generic()
Date: Tue, 20 Aug 2024 14:37:30 UTC
In message <202408191437.47JEbhOG094258@gitrepo.freebsd.org>, Mark Johnston
wri
tes:
> The branch main has been updated by markj:
>
> URL: https://cgit.FreeBSD.org/src/commit/?id=fb901935f257ddcc492fe9efb605797f
> 181c6597
>
> commit fb901935f257ddcc492fe9efb605797f181c6597
> Author: Mark Johnston <markj@FreeBSD.org>
> AuthorDate: 2024-08-19 14:20:43 +0000
> Commit: Mark Johnston <markj@FreeBSD.org>
> CommitDate: 2024-08-19 14:37:27 +0000
>
> socket: Split up sosend_generic()
>
> Factor out the bits that run with the sock I/O lock held into a separate
> function. In this implementation, we are doing a bit more work under
> the I/O lock than before. However, lock contention is only a problem
> when multiple threads are transmitting on the same socket, which is an
> unusual case that is not expected to perform well in any case.
>
> No functional change intended.
>
> Reviewed by: gallatin, glebius
> MFC after: 2 weeks
> Sponsored by: Klara, Inc.
> Sponsored by: Stormshield
> Differential Revision: https://reviews.freebsd.org/D46305
> ---
> sys/kern/uipc_socket.c | 47 +++++++++++++++++++++++++++++------------------
> 1 file changed, 29 insertions(+), 18 deletions(-)
>
> diff --git a/sys/kern/uipc_socket.c b/sys/kern/uipc_socket.c
> index e7c4a85d5970..a5e88fc7ffc6 100644
> --- a/sys/kern/uipc_socket.c
> +++ b/sys/kern/uipc_socket.c
> @@ -1656,8 +1656,8 @@ out:
> * counts if EINTR/ERESTART are returned. Data and control buffers are free
> d
> * on return.
> */
> -int
> -sosend_generic(struct socket *so, struct sockaddr *addr, struct uio *uio,
> +static int
> +sosend_generic_locked(struct socket *so, struct sockaddr *addr, struct uio *
> uio,
> struct mbuf *top, struct mbuf *control, int flags, struct thread *td)
> {
> long space;
> @@ -1673,6 +1673,9 @@ sosend_generic(struct socket *so, struct sockaddr *addr
> , struct uio *uio,
> tls = NULL;
> tls_rtype = TLS_RLTYPE_APP;
> #endif
> +
> + SOCK_IO_SEND_ASSERT_LOCKED(so);
> +
> if (uio != NULL)
> resid = uio->uio_resid;
> else if ((top->m_flags & M_PKTHDR) != 0)
> @@ -1702,10 +1705,6 @@ sosend_generic(struct socket *so, struct sockaddr *add
> r, struct uio *uio,
> if (control != NULL)
> clen = control->m_len;
>
> - error = SOCK_IO_SEND_LOCK(so, SBLOCKWAIT(flags));
> - if (error)
> - goto out;
> -
> #ifdef KERN_TLS
> tls_send_flag = 0;
> tls = ktls_hold(so->so_snd.sb_tls_info);
> @@ -1728,7 +1727,7 @@ sosend_generic(struct socket *so, struct sockaddr *addr
> , struct uio *uio,
>
> if (resid == 0 && !ktls_permit_empty_frames(tls)) {
> error = EINVAL;
> - goto release;
> + goto out;
> }
> }
> #endif
> @@ -1739,13 +1738,13 @@ restart:
> if (so->so_snd.sb_state & SBS_CANTSENDMORE) {
> SOCKBUF_UNLOCK(&so->so_snd);
> error = EPIPE;
> - goto release;
> + goto out;
> }
> if (so->so_error) {
> error = so->so_error;
> so->so_error = 0;
> SOCKBUF_UNLOCK(&so->so_snd);
> - goto release;
> + goto out;
> }
> if ((so->so_state & SS_ISCONNECTED) == 0) {
> /*
> @@ -1759,7 +1758,7 @@ restart:
> if (!(resid == 0 && clen != 0)) {
> SOCKBUF_UNLOCK(&so->so_snd);
> error = ENOTCONN;
> - goto release;
> + goto out;
> }
> } else if (addr == NULL) {
> SOCKBUF_UNLOCK(&so->so_snd);
> @@ -1767,7 +1766,7 @@ restart:
> error = ENOTCONN;
> else
> error = EDESTADDRREQ;
> - goto release;
> + goto out;
> }
> }
> space = sbspace(&so->so_snd);
> @@ -1777,7 +1776,7 @@ restart:
> clen > so->so_snd.sb_hiwat) {
> SOCKBUF_UNLOCK(&so->so_snd);
> error = EMSGSIZE;
> - goto release;
> + goto out;
> }
> if (space < resid + clen &&
> (atomic || space < so->so_snd.sb_lowat || space < clen)) {
> @@ -1785,12 +1784,12 @@ restart:
> (flags & (MSG_NBIO | MSG_DONTWAIT)) != 0) {
> SOCKBUF_UNLOCK(&so->so_snd);
> error = EWOULDBLOCK;
> - goto release;
> + goto out;
> }
> error = sbwait(so, SO_SND);
> SOCKBUF_UNLOCK(&so->so_snd);
> if (error)
> - goto release;
> + goto out;
> goto restart;
> }
> SOCKBUF_UNLOCK(&so->so_snd);
> @@ -1835,7 +1834,7 @@ restart:
> ((flags & MSG_EOR) ? M_EOR : 0));
> if (top == NULL) {
> error = EFAULT; /* only possible error
> */
> - goto release;
> + goto out;
> }
> space -= resid - uio->uio_resid;
> resid = uio->uio_resid;
> @@ -1899,12 +1898,10 @@ restart:
> control = NULL;
> top = NULL;
> if (error)
> - goto release;
> + goto out;
> } while (resid && space > 0);
> } while (resid);
>
> -release:
> - SOCK_IO_SEND_UNLOCK(so);
> out:
> #ifdef KERN_TLS
> if (tls != NULL)
> @@ -1917,6 +1914,20 @@ out:
> return (error);
> }
>
> +int
> +sosend_generic(struct socket *so, struct sockaddr *addr, struct uio *uio,
> + struct mbuf *top, struct mbuf *control, int flags, struct thread *td)
> +{
> + int error;
> +
> + error = SOCK_IO_SEND_LOCK(so, 0);
> + if (error)
> + return (error);
> + error = sosend_generic_locked(so, addr, uio, top, control, flags, td);
> + SOCK_IO_SEND_UNLOCK(so);
> + return (error);
> +}
> +
> /*
> * Send to a socket from a kernel thread.
> *
>
This commit broke NFS over TCP (V3 and V4). It it results in random
unrecoverable hangs of an NFS share.
My test is to installworld/installkernel over NFS from my main build server.
All the other socket commits in this series had no adverse affect on NFS
over TCP.
--
Cheers,
Cy Schubert <Cy.Schubert@cschubert.com>
FreeBSD UNIX: <cy@FreeBSD.org> Web: https://FreeBSD.org
NTP: <cy@nwtime.org> Web: https://nwtime.org
e^(i*pi)+1=0