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

On Thu, Dec 21, 2023 at 9:53 AM Warner Losh <imp@bsdimp.com> wrote:

>
>
> On Thu, Dec 21, 2023, 8:54 AM Mark Johnston <markj@freebsd.org> wrote:
>
>> 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.
>>
>
> Yea. We need to do this at time of use not time of alloc.
>
> > 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.
>>
>
> None of the ones I looked at had this... maybe I got unlucky. I'd think
> we'd have better performance if things are aligned all the way... is there
> a reason not to?
>

Yea, I see a lot of other drivers now...  I'll follow the trend. I think it
gives the ability to receive a full jumbo frame if I don't adjust.

https://reviews.freebsd.org/D43161

has the updated patches, including backing out this change and moving it
from alloc time to use time which seems better since we're chaining and
should work for all the cases we're talking about here.

Warner


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