Re: [RFC PATCH v3 2/2] dts: Initial Implementation For Jumbo Frames Test Suite

[Nicholas Pratte <npratte@iol.unh.edu>]

Hi Alex, thanks for the review!

See my comments below.

<snip>
> > +IP_HEADER_LEN = 20
> > +ETHER_STANDARD_FRAME = 1500
> > +ETHER_JUMBO_FRAME_MTU = 9000
>
> For these constants, I am confused why one is "FRAME" and the other is
> "MTU". The value of 'ETHER_STANDARD_FRAME' is 1500 (the standard MTU
> size), it would make sense to rename it to 'ETHER_STANDARD_MTU', to keep
> naming consistent.
>
> If the value was 1518 instead of 1500, then `ETHER_STANDARD_FRAME` would
> be appropriate.

You are correct! I will make the changes.
>
<snip>
> Renaming 'ETHER_STANDARD_FRAME' to 'ETHER_STANDARD_MTU' would reduce
> confusion here too.
> e.g.
> `testpmd.configure_port_mtu_all(ETHER_STANDARD_MTU)`
>
> Additionally, you state you are sending packets of sizes 1517 and 1518.
> but you then call:
> `self.send_packet_and_verify(ETHER_STANDARD_FRAME - 5)`
> `self.send_packet_and_verify(ETHER_STANDARD_FRAME)`

Ack. I must have missed the docstring when I adjusted the boundaries
at which we test jumbo frame sizes ethernet overhead issues we've been
having. Adding the +5, -5 bytes is sort of a temporary
measure/placeholder while we wait to gather more information on how to
properly assess and test the ethernet overhead issue. See my next
comment for more clarification.
>
> Calculating to:
> `self.send_packet_and_verify(1495)`
> `self.send_packet_and_verify(1500)`
>
> Which is confusing.
> I believe this is because you are accounting for the 4 bytes of VLAN's
> in your calculations, but you might want to explain this.

What the +5 bytes situation really is for, currently, is to assess the
boundaries of a set MTU size, sorry for the confusion there. For
example, if we have a MTU of 1500, some vendors assume an ethernet
overhead of +22 bytes on top of the 1500 byte MTU (for a total frame
size of 1522 bytes), and other vendors, such as Mellanox, add +18
bytes of ethernet overhead +1500 byte MTU (for a total frame size of
1518). The +5 tries to compensate for this by adding +5 or -5 bytes to
properly test greater than or less than a 1500 byte MTU for all
vendors, but this gets tricky when you are trying to run tests at the
MTU size itself. If you look back to the oldest version I have of this
suite, you'll see that each test case was originally sending 1499 byte
packets, 1501 packets, and 1500 packets in some cases, but we can't
run tests like this because of the differing assumptions from vendor
to vendor (you can find the old suite in a different email thread, I
messed up the sending of the series so I apologize for that).

Here's the original version of this patch:
https://inbox.dpdk.org/dev/20240524183604.6925-2-npratte@iol.unh.edu/

The calculation of ethernet overhead basically comes down to a single
'if' statement in testpmd's code. You can find this method in
testpmd.c and do some digging if you're interested (look under
'app/test-pmd/testpmd.c' and search for 'get_eth_overhead')

>
>
> Overall very solid and clean test suite, just wanted to get
> clarification on a few areas 🙂.
> Alex

I wrote this test suite a while back when I was just starting out, and
a lot of this information was new to me at the time, so it's not
surprising to see some of the misuse of definitions you've laid out
here; I appreciate the feedback!

-Nicholas