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

Sorry for (a) the top post and (b) replying to myself...

https://reviews.freebsd.org/D43656

is what I think you're worried about. Am I right, or is there some place
else that has you uneasy?

Warner

P.S. I'm also thinking about following that up with
https://reviews.freebsd.org/D43654 since that style seems more in fashion
these days.

On Mon, Jan 29, 2024 at 10:14 AM Warner Losh <imp@bsdimp.com> wrote:

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