From: David Hill Subject: Re: m_getuio - prepend mbuf allocations To: tech@openbsd.org Date: Fri, 1 Nov 2024 13:22:46 +0000 On 10/31/24 19:10, David Hill wrote: > I am attempting to cleanup 'prepend mbuf allocations' and have included > a diff below. > > These sendto's are the most frequest now that wireguard fixes are in. > I assume these are mostly unbound doing DNS. > > m_prepend: len 48 > -+- mbuf 0xfffffd8013662000, dat, off 0, len 33, pktlen 33, size 144 >  \\- total chain 1, len 33, size 144 > Starting stack trace... > m_print_debug(fffffd8013662000,ffffffff819eb167,30) at m_print_debug+0x31d > m_prepend(fffffd8013662000,30,2) at m_prepend+0xbe > udp6_output(fffffd824dab66d0,fffffd8013662000,0,0) at udp6_output+0x167 > sosend(ffff8000003b2040,0,ffff80003172d0c0,0,0,80) at sosend+0x3e2 > sendit(ffff80003162e540,6,ffff80003172d1b8,0,ffff80003172d250) at > sendit+0x395 > sys_sendto(ffff80003162e540,ffff80003172d2e0,ffff80003172d250) at > sys_sendto+0x6c > syscall(ffff80003172d2e0) at syscall+0x620 > Xsyscall() at Xsyscall+0x128 > end of kernel > end trace frame: 0x7cc803157ff0, count: 249 > End of stack trace. > > m_prepend: len 28 > -+- mbuf 0xfffffd800a2f5a00, dat, off 0, len 23, pktlen 23, size 144 >   \- total chain 1, len 23, size 144 > Starting stack trace... > m_print_debug(fffffd800a2f5a00,ffffffff819e7e0d,1c) at m_print_debug+0x31d > m_prepend(fffffd800a2f5a00,1c,2) at m_prepend+0xbe > udp_output(fffffd820aef7938,fffffd800a2f5a00,fffffd801483b600,0) at > udp_output+0x2f2 > sosend(ffff80000089e548,fffffd801483b600,ffff800031614ff0,0,0,80) at > sosend+0x3e2 > sendit(ffff80003161c2a8,3,ffff8000316150e8,0,ffff800031615180) at > sendit+0x395 > sys_sendto(ffff80003161c2a8,ffff800031615210,ffff800031615180) at > sys_sendto+0x6c > syscall(ffff800031615210) at syscall+0x620 > Xsyscall() at Xsyscall+0x128 > end of kernel > end trace frame: 0x7560c50f07b0, count: 249 > End of stack trace. > > The following diff does not produce any of the debugs above.  Please > tell me if this is correct or have caused another issue. > > 1.) on first iteration, only write at most (MHLEN-max_hdr) bytes, saving > room for max_hdr. > > 2.) since MINCLSIZE > (MHLEN-max_hdr), delete the code to move m_data > forward since it m will never equal top here. > > 3.) fix off by one in comparison. > > Index: kern/uipc_socket.c > =================================================================== > RCS file: /cvs/src/sys/kern/uipc_socket.c,v > diff -u -p -r1.344 uipc_socket.c > --- kern/uipc_socket.c    31 Oct 2024 12:51:55 -0000    1.344 > +++ kern/uipc_socket.c    31 Oct 2024 17:41:27 -0000 > @@ -795,6 +795,7 @@ m_getuio(struct mbuf **mp, int atomic, l >          if (top == NULL) { >              MGETHDR(m, M_WAIT, MT_DATA); >              mlen = MHLEN; > +            resid = ulmin(resid, MHLEN - max_hdr); >          } else { >              MGET(m, M_WAIT, MT_DATA); >              mlen = MLEN; > @@ -812,12 +813,6 @@ m_getuio(struct mbuf **mp, int atomic, l >                  goto nopages; >              mlen = m->m_ext.ext_size; >              len = ulmin(mlen, resid); > -            /* > -             * For datagram protocols, leave room > -             * for protocol headers in first mbuf. > -             */ > -            if (atomic && m == top && len < mlen - max_hdr) > -                m->m_data += max_hdr; >          } else { >  nopages: >              len = ulmin(mlen, resid); > @@ -825,7 +820,7 @@ nopages: >               * For datagram protocols, leave room >               * for protocol headers in first mbuf. >               */ > -            if (atomic && m == top && len < mlen - max_hdr) > +            if (atomic && m == top && len <= mlen - max_hdr) >                  m_align(m, len); >          } > I missed checking for atomic. Which makes the diff larger and the code smaller: 1.) if atomic, put at most (MHLEN-max_hdr) bytes into the header, leaving room for max_hdr; This removes the need for the atomic checks in each conditional. 2.) By reversing the logic check for (m->m_flags & M_EXT), we can remove the else {} block. Index: uipc_socket.c =================================================================== RCS file: /cvs/src/sys/kern/uipc_socket.c,v diff -u -p -r1.344 uipc_socket.c --- uipc_socket.c 31 Oct 2024 12:51:55 -0000 1.344 +++ uipc_socket.c 1 Nov 2024 12:13:04 -0000 @@ -795,6 +795,14 @@ m_getuio(struct mbuf **mp, int atomic, l if (top == NULL) { MGETHDR(m, M_WAIT, MT_DATA); mlen = MHLEN; + /* + * For datagram protocols, leave room + * for protocol headers in first mbuf. + */ + if (atomic) { + resid = ulmin(resid, MHLEN - max_hdr); + m_align(m, resid); + } } else { MGET(m, M_WAIT, MT_DATA); mlen = MLEN; @@ -808,26 +816,10 @@ m_getuio(struct mbuf **mp, int atomic, l MCLGETL(m, M_NOWAIT, ulmin(resid, MAXMCLBYTES)); if ((m->m_flags & M_EXT) == 0) MCLGETL(m, M_NOWAIT, MCLBYTES); - if ((m->m_flags & M_EXT) == 0) - goto nopages; - mlen = m->m_ext.ext_size; - len = ulmin(mlen, resid); - /* - * For datagram protocols, leave room - * for protocol headers in first mbuf. - */ - if (atomic && m == top && len < mlen - max_hdr) - m->m_data += max_hdr; - } else { -nopages: - len = ulmin(mlen, resid); - /* - * For datagram protocols, leave room - * for protocol headers in first mbuf. - */ - if (atomic && m == top && len < mlen - max_hdr) - m_align(m, len); + if ((m->m_flags & M_EXT) != 0) + mlen = m->m_ext.ext_size; } + len = ulmin(mlen, resid); error = uiomove(mtod(m, caddr_t), len, uio); if (error) {