Re: git: 3be59adbb5a2 - main - vtnet: Adjust for ethernet alignment.

On Mon, Jan 29, 2024 at 8:26 AM Bjoern A. Zeeb <
bzeeb-lists@lists.zabbadoz.net> wrote:

> On Mon, 29 Jan 2024, Warner Losh wrote:
>
> > The branch main has been updated by imp:
> >
> > URL:
> https://cgit.FreeBSD.org/src/commit/?id=3be59adbb5a2ae7600d46432d3bc82286e507e95
> >
> > commit 3be59adbb5a2ae7600d46432d3bc82286e507e95
> > Author:     Warner Losh <imp@FreeBSD.org>
> > AuthorDate: 2024-01-29 05:08:55 +0000
> > Commit:     Warner Losh <imp@FreeBSD.org>
> > CommitDate: 2024-01-29 05:08:55 +0000
> >
> >    vtnet: Adjust for ethernet alignment.
> >
> >    If the header that we add to the packet's size is 0 % 4 and we're
> >    strictly aligning, then we need to adjust where we store the header so
> >    the packet that follows will have it's struct ip header properly
> >    aligned.  We do this on allocation (and when we check the length of
> the
> >    mbufs in the lro_nomrg case). We can't just adjust the clustersz in
> the
> >    softc, because it's also used to allocate the mbufs and it needs to be
> >    the proper size for that. Since we otherwise use the size of the mbuf
> >    (or sometimes the smaller size of the received packet) to compute how
> >    much we can buffer, this ensures no overflows. The 2 byte adjustment
> >    also does not affect how many packets we can receive in the lro_nomrg
> >    case.
>
>
> Doesn't this still include at least these two un-asserted/un-documented
> asumptions:
>
> (a) mbuf space is large enough to hold 2 extra bytes?  Is this always
>      the case?
>

I was sure I puzzled through all the cases correctly.  We adjust the length
of the available buffer by 2 and offset everything by 2. this work because
all the vtnet header types only have 1 or 2 byte data fields. It keeps us
from
writing too much into the buffer.

However, in vtnet_rx_cluster_size, we don't adjust the frame size before
allocating. So if the mtu + vlan_header. So if the mtu + 12 + 18 is 2047 or
2048
or mtu = 2017 or 2018 we'll get it wrong (we don't adjust in the case where
we use vtnet_rx_header which is 14 bytes). But I think in that case, we'll
"drop
the last two bytes off the end" get it wrong (since we adjust the total
length
of the mbuf space) rather than "overflow two bytes" get it wrong. For that
case, we'd need to add two as I indicated in the comments below.

static int
vtnet_rx_cluster_size(struct vtnet_softc *sc, int mtu)
{
        int framesz;

        if (sc->vtnet_flags & VTNET_FLAG_MRG_RXBUFS)
                return (MJUMPAGESIZE);
        else if (sc->vtnet_flags & VTNET_FLAG_LRO_NOMRG)
                return (MCLBYTES);

        /*
         * Try to scale the receive mbuf cluster size from the MTU. We
         * could also use the VQ size to influence the selected size,
         * but that would only matter for very small queues.
         */
        if (vtnet_modern(sc)) {
                MPASS(sc->vtnet_hdr_size == sizeof(struct
virtio_net_hdr_v1));
                framesz = sizeof(struct virtio_net_hdr_v1);
        } else
                framesz = sizeof(struct vtnet_rx_header);
        framesz += sizeof(struct ether_vlan_header) + mtu;
// XXX if framesz % 4 == 2 and we're strict alignment we need to add 2
// XXX or equivalently, if vnet_hdr_size % 4 == 0 and ...
        if (framesz <= MCLBYTES)
                return (MCLBYTES);
        else if (framesz <= MJUMPAGESIZE)
                return (MJUMPAGESIZE);
        else if (framesz <= MJUM9BYTES)
                return (MJUM9BYTES);

        /* Sane default; avoid 16KB clusters. */
        return (MCLBYTES);
}

Do you agree? Is this what you are worried about? It's the only hole I
could find
this morning (after going over this a dozen other times trying to get it
right for
the review, and bryanv was happy neither noticed). It also explains why my
tests work: I didn't try to have a weird mtu of 2018 bytes.


> (b) the struct sizes assigned to vtnet_hdr_size are not odd numbers of
>      bytes?  Could add comments or CTASSERTs?
>

True, I'll ctassert the sizes and say we rely on things being even sized
in if_vnetvar.h.

Warner


> >    PR:                     271288
> >    Sponsored by:           Netflix
> >    Reviewed by:            bryanv
> >    Differential Revision:  https://reviews.freebsd.org/D43224
>
> --
> Bjoern A. Zeeb                                                     r15:7
>