Date: Thu, 21 Dec 2023 08:31:36 -0700 From: Warner Losh <imp@bsdimp.com> To: Mark Johnston <markj@freebsd.org> Cc: Warner Losh <imp@freebsd.org>, src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org Subject: Re: git: 9e6d11ce9a51 - main - vtnet: Adjust rx buffer so IP header 32-bit aligned Message-ID: <CANCZdfqtGww8-6ObNo_rwTvcRJWH8UT%2B-f21tXodM54sqaWLpw@mail.gmail.com> In-Reply-To: <ZYRWH4YEb6eO5hGD@nuc> References: <202312210421.3BL4Lb9T036317@gitrepo.freebsd.org> <ZYRWH4YEb6eO5hGD@nuc>
next in thread | previous in thread | raw e-mail | index | archive | help
--000000000000374067060d06cc61 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Thu, Dec 21, 2023 at 8:13=E2=80=AFAM Mark Johnston <markj@freebsd.org> w= rote: > On Thu, Dec 21, 2023 at 04:21:37AM +0000, Warner Losh wrote: > > The branch main has been updated by imp: > > > > URL: > https://cgit.FreeBSD.org/src/commit/?id=3D9e6d11ce9a51d75ed6a94e180f2fb4e= 9188a2ba4 > > > > commit 9e6d11ce9a51d75ed6a94e180f2fb4e9188a2ba4 > > Author: Warner Losh <imp@FreeBSD.org> > > AuthorDate: 2023-12-20 19:09:09 +0000 > > Commit: Warner Losh <imp@FreeBSD.org> > > CommitDate: 2023-12-21 04:16:45 +0000 > > > > vtnet: Adjust rx buffer so IP header 32-bit aligned > > > > Call madj(m, ETHER_ALIGN) to offset rx buffers when allocating them= . > > This improves performance everywhere, and allows armv7 to work at > all. > > > > PR: 271288 (PR had a different fix than I wound > up with) > > MFC After: 3 days > > Sponsored by: Netflix > > Differential Revision: https://reviews.freebsd.org/D43136 > > --- > > sys/dev/virtio/network/if_vtnet.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/sys/dev/virtio/network/if_vtnet.c > b/sys/dev/virtio/network/if_vtnet.c > > index 287af7751066..360176e4f845 100644 > > --- a/sys/dev/virtio/network/if_vtnet.c > > +++ b/sys/dev/virtio/network/if_vtnet.c > > @@ -1532,8 +1532,8 @@ vtnet_rx_alloc_buf(struct vtnet_softc *sc, int > nbufs, struct mbuf **m_tailp) > > m_freem(m_head); > > return (NULL); > > } > > - > > m->m_len =3D size; > > + m_adj(m, ETHER_ALIGN); > > The driver is expecting to get a cluster of size sc->vtnet_rx_clustersz, > but now it's getting one of size sc->vtnet_rx_clustersz - 2. I don't > see how this change can be sufficient on its own: what prevents virtio > from writing sc->vtnet_rx_clustersz bytes and thereby overwriting the > two bytes following the cluster? > The only trouble that I saw was here: /* * Every mbuf should have the expected cluster size since that * is also used to allocate the replacements. */ KASSERT(m->m_len =3D=3D clustersz, ("%s: mbuf size %d not expected cluster size %d", __func__, m->m_len, clustersz)); and even that's fine: We can adjust that assert. The next lines I think make it fine: m->m_len =3D MIN(m->m_len, len); so we only use what we say is there to land bytes into the mbuf. I'm now not sure though what to do about a few lines later: m_new =3D vtnet_rx_alloc_buf(sc, nreplace, &m_tail); if (m_new =3D=3D NULL) { m_prev->m_len =3D clustersz; return (ENOBUFS); } which likely needs to be adjusted (or the old saved size). I likely didn't see asserts in my testing because I didn't have LRO slow path packets. Do you see other places that would need adjusting? All the other places seem to use m_len correctly on the rx path, but I'm not a nic driver guy, and I might be missing something. Warner > > if (m_head !=3D NULL) { > > m_tail->m_next =3D m; > > m_tail =3D m; > --000000000000374067060d06cc61 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable <div dir=3D"ltr"><div dir=3D"ltr"><br></div><br><div class=3D"gmail_quote">= <div dir=3D"ltr" class=3D"gmail_attr">On Thu, Dec 21, 2023 at 8:13=E2=80=AF= AM Mark Johnston <<a href=3D"mailto:markj@freebsd.org">markj@freebsd.org= </a>> wrote:<br></div><blockquote class=3D"gmail_quote" style=3D"margin:= 0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">= On Thu, Dec 21, 2023 at 04:21:37AM +0000, Warner Losh wrote:<br> > The branch main has been updated by imp:<br> > <br> > URL: <a href=3D"https://cgit.FreeBSD.org/src/commit/?id=3D9e6d11ce9a51= d75ed6a94e180f2fb4e9188a2ba4" rel=3D"noreferrer" target=3D"_blank">https://= cgit.FreeBSD.org/src/commit/?id=3D9e6d11ce9a51d75ed6a94e180f2fb4e9188a2ba4<= /a><br> > <br> > commit 9e6d11ce9a51d75ed6a94e180f2fb4e9188a2ba4<br> > Author:=C2=A0 =C2=A0 =C2=A0Warner Losh <imp@FreeBSD.org><br> > AuthorDate: 2023-12-20 19:09:09 +0000<br> > Commit:=C2=A0 =C2=A0 =C2=A0Warner Losh <imp@FreeBSD.org><br> > CommitDate: 2023-12-21 04:16:45 +0000<br> > <br> >=C2=A0 =C2=A0 =C2=A0vtnet: Adjust rx buffer so IP header 32-bit aligned= <br> >=C2=A0 =C2=A0 =C2=A0<br> >=C2=A0 =C2=A0 =C2=A0Call madj(m, ETHER_ALIGN) to offset rx buffers when= allocating them.<br> >=C2=A0 =C2=A0 =C2=A0This improves performance everywhere, and allows ar= mv7 to work at all.<br> >=C2=A0 =C2=A0 =C2=A0<br> >=C2=A0 =C2=A0 =C2=A0PR:=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0271288 (PR had a different fix than I wound up = with)<br> >=C2=A0 =C2=A0 =C2=A0MFC After:=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 3 days<br> >=C2=A0 =C2=A0 =C2=A0Sponsored by:=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0Netflix<br> >=C2=A0 =C2=A0 =C2=A0Differential Revision:=C2=A0 <a href=3D"https://rev= iews.freebsd.org/D43136" rel=3D"noreferrer" target=3D"_blank">https://revie= ws.freebsd.org/D43136</a><br> > ---<br> >=C2=A0 sys/dev/virtio/network/if_vtnet.c | 2 +-<br> >=C2=A0 1 file changed, 1 insertion(+), 1 deletion(-)<br> > <br> > diff --git a/sys/dev/virtio/network/if_vtnet.c b/sys/dev/virtio/networ= k/if_vtnet.c<br> > index 287af7751066..360176e4f845 100644<br> > --- a/sys/dev/virtio/network/if_vtnet.c<br> > +++ b/sys/dev/virtio/network/if_vtnet.c<br> > @@ -1532,8 +1532,8 @@ vtnet_rx_alloc_buf(struct vtnet_softc *sc, int n= bufs, struct mbuf **m_tailp)<br> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0m_freem(m_head);<br> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0return (NULL);<br> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0}<br> > -<br> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0m->m_len =3D = size;<br> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0m_adj(m, ETHER_ALIGN)= ;<br> <br> The driver is expecting to get a cluster of size sc->vtnet_rx_clustersz,= <br> but now it's getting one of size sc->vtnet_rx_clustersz - 2.=C2=A0 I= don't<br> see how this change can be sufficient on its own: what prevents virtio<br> from writing sc->vtnet_rx_clustersz bytes and thereby overwriting the<br= > two bytes following the cluster?<br></blockquote><div><br></div><div>The on= ly trouble that I saw was here:</div><div><br></div><div>=C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /*<br>=C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* Every mbuf should have the expected clu= ster size since that<br>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0* is also used to allocate the replacements.<br>=C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0*/<br>=C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 KASSERT(m->m_len =3D=3D clustersz,<br= >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (&qu= ot;%s: mbuf size %d not expected cluster size %d", __func__,<br>=C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 m->m_len= , clustersz));<br></div><div><br></div><div>and even that's fine: We ca= n adjust that assert. The next lines I think make it fine:</div><div>=C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 m->m_len =3D MIN(m->= ;m_len, len);<br></div><div><br></div><div>so we only use what we say is th= ere to land bytes into the mbuf.=C2=A0 I'm now not sure though what to = do about a few lines later:</div><div><br></div><div>=C2=A0 =C2=A0 =C2=A0 = =C2=A0m_new =3D vtnet_rx_alloc_buf(sc, nreplace, &m_tail);<br>=C2=A0 = =C2=A0 =C2=A0 =C2=A0 if (m_new =3D=3D NULL) {<br>=C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 m_prev->m_len =3D clustersz;<br>=C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return (ENOBUFS);<br>=C2= =A0 =C2=A0 =C2=A0 =C2=A0 }<br></div><div><br></div><div>which likely needs = to be adjusted (or the old saved size).</div><div><br></div><div>I likely d= idn't see asserts in my testing because I didn't have LRO slow path= packets.</div><div><br></div><div>Do you see other places that would need = adjusting? All the other places seem to use m_len correctly on the rx path,= but I'm not a nic driver guy, and I might be missing something.</div><= div><br></div><div>Warner</div><div><br></div><div>=C2=A0</div><blockquote = class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-left:1px sol= id rgb(204,204,204);padding-left:1ex"> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (m_head !=3D = NULL) {<br> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0m_tail->m_next =3D m;<br> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0m_tail =3D m;<br> </blockquote></div></div> --000000000000374067060d06cc61--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CANCZdfqtGww8-6ObNo_rwTvcRJWH8UT%2B-f21tXodM54sqaWLpw>