From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 3ADA04586E; Mon, 26 Aug 2024 23:17:48 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 0643D40A7A; Mon, 26 Aug 2024 23:17:48 +0200 (CEST) Received: from mail-pf1-f178.google.com (mail-pf1-f178.google.com [209.85.210.178]) by mails.dpdk.org (Postfix) with ESMTP id 85A54400EF for ; Mon, 26 Aug 2024 23:17:46 +0200 (CEST) Received: by mail-pf1-f178.google.com with SMTP id d2e1a72fcca58-7148912a1ebso1020674b3a.0 for ; Mon, 26 Aug 2024 14:17:46 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=iol.unh.edu; s=unh-iol; t=1724707066; x=1725311866; darn=dpdk.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=/6xb20gQRzyIA9bjlqCC0pNaom0FujOvMBPDsOe26PA=; b=ZnqtjKtsmx7vKZmuQvXoszhefUQ6GzQ9Ao/OIEnwFawjE2/EdVdz1/W49CJ/Zk8dhv ZCHwEn2jdJdXLQLviZcvvYGuWGdhaCLhU9AOe1YMNlBd3mGHVChJX9qhOasuCxDFyZQ/ PmBRi+saR0TtBp1EKKyAeg3qxZPORzNg09q8k= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1724707066; x=1725311866; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=/6xb20gQRzyIA9bjlqCC0pNaom0FujOvMBPDsOe26PA=; b=xMqsdGODKpGKX/iRMgxiPud4Sd3X5ywKoTMg8LI44eTsSXlPWeMHsNQVJWCFZa+gpo yJUdoIgD0x5KlwkDsdCdMg/UkDOHW8R+jm2bQ+RupFMNvNussTuNJ3ttAMrpUVAhD5HQ E+l2LBA6+t9rq7uc9vewY0jyVCLhYR40mqHjuaHlhx+s/wfZ+T5mIaxp1Vu8YPh8JukN fni60TgCcVyD8zb/b9WIIJMuOlj5ZJ5VbwxMOaezNiba2oxgu7wjPl4lxMwENPJmVSox javN9hXTWhBL1Gw1jG8JMQHdNYY+ejw4P+nxVZvMv2haYvmg9RKnwfN79YnIV47FsD/P 51Xw== X-Forwarded-Encrypted: i=1; AJvYcCVa+Jmx0SMmXyOO6NQ8AMt94GABZ3QhgEg9nYANffdvle7OwSXvmztWpENyUuk4lFSn/YA=@dpdk.org X-Gm-Message-State: AOJu0YxlAY52ogCCRRNfgaOR+rgAdgKZ1Hd3e4kbYOSADAhLOscC/D1c t6NhPGIvJ8mk0aa5CVrKAVvGtGIVS9EP4FuFFAGgtf6O2YzGokj8BM7paDAzvZuSrSJd4+b7HBt XnqaPC0W2itHA67sGs/uIDFhWsvYGFa3rv5ESxQ== X-Google-Smtp-Source: AGHT+IH9eu8oBbAtARuYrf0Vi49nO4gROnN+OBfHdkSxFfe1Cg0lOD6omyej6i0KV1CUPCaqs9U2OZDIxRdGJiUoN4M= X-Received: by 2002:a05:6a00:1384:b0:70d:11d9:8a3c with SMTP id d2e1a72fcca58-715c00cbba1mr1169878b3a.26.1724707065511; Mon, 26 Aug 2024 14:17:45 -0700 (PDT) MIME-Version: 1.0 References: <20240816142031.15150-1-dmarx@iol.unh.edu> <20240821162550.1163-1-dmarx@iol.unh.edu> <20240821162550.1163-3-dmarx@iol.unh.edu> In-Reply-To: From: Dean Marx Date: Mon, 26 Aug 2024 17:17:57 -0400 Message-ID: Subject: Re: [PATCH v2 2/2] dts: checksum offload test suite To: Jeremy Spewock Cc: probb@iol.unh.edu, npratte@iol.unh.edu, luca.vizzarro@arm.com, yoan.picchi@foss.arm.com, Honnappa.Nagarahalli@arm.com, paul.szczepanek@arm.com, juraj.linkes@pantheon.tech, dev@dpdk.org Content-Type: multipart/alternative; boundary="000000000000fb5fac06209ca7bf" X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org --000000000000fb5fac06209ca7bf Content-Type: text/plain; charset="UTF-8" > You could probably combine this line with the previous since they are > from the same module. > > > +from scapy.packet import Raw # type: ignore[import-untyped] > > I think you can also import `Packet` from this module if you wanted to > combine another two lines as well. > > Wow I didn't even notice that good catch > > + > > +from framework.remote_session.testpmd_shell import ( > > + SimpleForwardingModes, > > This reminds me of a question I've had for a little while now which > is, should this be imported from the module that it originates from > (params) or is it fine to just grab it from the testpmd shell where it > is also imported? I guess I don't really see this causing a problem at > all since there isn't really a chance of any circular imports in this > case or things that would be breaking, but I just don't know if there > is any kind of guideline regarding these scenarios. > I briefly looked for some best practice guidelines about this kind of thing but I couldn't find anything explicit. However, I'm going to assume it's probably preferred to do a direct import like you mentioned so I'm going to change that. > > + testpmd.start() > > + self.send_packet_and_capture(packet=packet) > > + verbose_output = testpmd.extract_verbose_output(testpmd.stop()) > > + for packet in verbose_output: > > + if packet.dst_mac == "00:00:00:00:00:01": > > Since this method is the one that relies on this MAC address being set > on the packet, it might be helpful to set that MAC on the packet > before sending it in the same method. Why this address is the one you > were searching for would then be clear at just a glance at the send > method which could be useful. That or you could note in the doc-string > what you expect the MAC to be. > Funny that you mentioned this because I actually tried to set the destination mac address within the send_packet_and_verify_checksums method and I couldn't get it to work no matter what I tried. For some reason even though the mac address was the one I set after send_packet_and_capture, none of the packets in verbose_output would have that mac address. I tried debugging for a while but I just couldn't get it to work, even with the adjust_addresses patch applied it was breaking so I just removed it entirely and set them all in the test cases, which worked fine. I did add an extra arg to the send_and_verify_checksums method called ID, which I passed the mac_id variable to so that it's cleaner. > > > + if OLFlag.RTE_MBUF_F_RX_L4_CKSUM_GOOD in > packet.ol_flags: > > + isIP = True > > + else: > > + isIP = False > > + if OLFlag.RTE_MBUF_F_RX_L4_CKSUM_GOOD in > packet.ol_flags: > > + isL4 = True > > + else: > > + isL4 = False > > + else: > > + isIP = False > > + isL4 = False > > Would having this else statement break the booleans if there was > another noise packet after the one that you sent in the verbose > output? I think that would make both of these booleans false by the > end of the loop even if one was supposed to be true. You might be able > to fix this however with either a break statement after you find the > first packet with the right MAC address, or you could use the built-in > `any()` function that python offers. > Yeah you're right, not sure what I was thinking there haha. I simplified it back down to two lines in the new version. > > Not really important at all (especially because it won't affect the > order that the cases are run in) but it might make sense to put the > two individual tests of l3_rx and l4_rx before the test of both of > them combined. Again, super nit-picky, but it makes sense in my head > to see the individual parts tested and then see them tested together. > I'll leave it up to you if you think it makes sense to just leave it > as is :). > Makes sense to me, swapped them > > > + > > + def test_vlan_checksum(self) -> None: > > + """Tests VLAN Rx checksum hardware offload and verify packet > reception.""" > > What is this testing? Just that the checksums work with VLANs also > set? That's fine if so I just wasn't sure initially since it looks > like the method is checking to see if you can receive the packets and > then if the checksums are right. > Yes it's just testing to ensure checksums work with VLAN packets according to the test plan. I'm not entirely sure why they also check to make sure they're received, but it was in the test plan so I just left it in there. Other than that, all the docstring errors and function names were fixed in both patches. Thanks for the review Jeremy! --000000000000fb5fac06209ca7bf Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
<snip>=C2=A0
You could probably combine = this line with the previous since they are
from the same module.

> +from scapy.packet import Raw=C2=A0 # type: ignore[import-untyped]

I think you can also import `Packet` from this module if you wanted to
combine another two lines as well.


Wow I didn't even notice that good= catch
=C2=A0
> +
> +from framework.remote_session.testpmd_shell import (
> +=C2=A0 =C2=A0 SimpleForwardingModes,

This reminds me of a question I've had for a little while now which
is, should this be imported from the module that it originates from
(params) or is it fine to just grab it from the testpmd shell where it
is also imported? I guess I don't really see this causing a problem at<= br> all since there isn't really a chance of any circular imports in this case or things that would be breaking, but I just don't know if there is any kind of guideline regarding these scenarios.
I briefly looked for some best practice guidelines about this = kind of thing but I couldn't find anything explicit. However, I'm g= oing to assume it's probably preferred to do a direct import like you m= entioned so I'm going to change that.
=C2=A0
<sn= ip>
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 testpmd.start()
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 self.send_packet_and_capture(packet=3Dpac= ket)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 verbose_output =3D testpmd.extract_verbos= e_output(testpmd.stop())
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 for packet in verbose_output:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if packet.dst_mac =3D=3D &q= uot;00:00:00:00:00:01":

Since this method is the one that relies on this MAC address being set
on the packet, it might be helpful to set that MAC on the packet
before sending it in the same method. Why this address is the one you
were searching for would then be clear at just a glance at the send
method which could be useful. That or you could note in the doc-string
what you expect the MAC to be.

Funny th= at you mentioned this because I actually tried to set the destination mac a= ddress within the send_packet_and_verify_checksums method and I couldn'= t get it to work no matter what I tried. For some reason even though the ma= c address was the one I set after send_packet_and_capture, none of the pack= ets in verbose_output would have that mac address. I tried debugging for a = while but I just couldn't get it to work, even with the adjust_addresse= s patch applied it was breaking so I just removed it entirely and set them = all in the test cases, which worked fine. I did add an extra arg to the sen= d_and_verify_checksums method called ID, which I passed the mac_id variable= to so that it's cleaner.
=C2=A0

> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if OLFlag.RTE= _MBUF_F_RX_L4_CKSUM_GOOD in packet.ol_flags:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= isIP =3D True
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 else:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= isIP =3D False
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if OLFlag.RTE= _MBUF_F_RX_L4_CKSUM_GOOD in packet.ol_flags:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= isL4 =3D True
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 else:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= isL4 =3D False
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 else:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 isIP =3D Fals= e
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 isL4 =3D Fals= e

Would having this else statement break the booleans if there was
another noise packet after the one that you sent in the verbose
output? I think that would make both of these booleans false by the
end of the loop even if one was supposed to be true. You might be able
to fix this however with either a break statement after you find the
first packet with the right MAC address, or you could use the built-in
`any()` function that python offers.

Ye= ah you're right, not sure what I was thinking there haha. I simplified = it back down to two lines in the new version.
=C2=A0
&l= t;snip>

Not really important at all (especially because it won't affect the
order that the cases are run in) but it might make sense to put the
two individual tests of l3_rx and l4_rx before the test of both of
them combined. Again, super nit-picky, but it makes sense in my head
to see the individual parts tested and then see them tested together.
I'll leave it up to you if you think it makes sense to just leave it as is :).

Makes sense to me, swapped th= em
=C2=A0

> +
> +=C2=A0 =C2=A0 def test_vlan_checksum(self) -> None:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """Tests VLAN Rx checksum = hardware offload and verify packet reception."""

What is this testing? Just that the checksums work with VLANs also
set? That's fine if so I just wasn't sure initially since it looks<= br> like the method is checking to see if you can receive the packets and
then if the checksums are right.

Yes it= 's just testing to ensure checksums work with VLAN packets according to= the test plan. I'm not entirely sure why they also check to make sure = they're received, but it was in the test plan so I just left it in ther= e.
=C2=A0
<snip>

Other t= han that, all the docstring errors and function names were fixed in both pa= tches. Thanks for the review Jeremy!
--000000000000fb5fac06209ca7bf--