Re: git: 9e6d11ce9a51 - main - vtnet: Adjust rx buffer so IP header 32-bit aligned

On Thu, Dec 21, 2023 at 08:31:36AM -0700, Warner Losh wrote:
> On Thu, Dec 21, 2023 at 8:13 AM Mark Johnston <markj@freebsd.org> wrote:
> 
> > 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=9e6d11ce9a51d75ed6a94e180f2fb4e9188a2ba4
> > >
> > > 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 = 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 == 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 = 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 = vtnet_rx_alloc_buf(sc, nreplace, &m_tail);
>         if (m_new == NULL) {
>                 m_prev->m_len = clustersz;
>                 return (ENOBUFS);
>         }
> 
> which likely needs to be adjusted (or the old saved size).

I'm not certain about what it should be doing instead, but yes this code
looks wrong now.

> 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?

Presumably vtnet_rx_cluster_size() needs to be adjusted too?

I also don't understand why we do this adjustment unconditionally
instead of only when __NO_STRICT_ALIGNMENT is not defined, as almost all
other NIC drivers do.

> 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.