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