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 2278D45A2C; Wed, 25 Sep 2024 18:09:53 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id DEE8F4025D; Wed, 25 Sep 2024 18:09:52 +0200 (CEST) Received: from mail-pj1-f43.google.com (mail-pj1-f43.google.com [209.85.216.43]) by mails.dpdk.org (Postfix) with ESMTP id 5E4A2400EF for ; Wed, 25 Sep 2024 18:09:51 +0200 (CEST) Received: by mail-pj1-f43.google.com with SMTP id 98e67ed59e1d1-2d87176316eso886131a91.0 for ; Wed, 25 Sep 2024 09:09:51 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=iol.unh.edu; s=unh-iol; t=1727280590; x=1727885390; darn=dpdk.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=63WvnMi8FWs1j24Criu8FigofP8q38R0vhAOR7AQ9cY=; b=Q8Q6PBg00ydZakBx4n7ET/bIsVc/f6DOLDhFNVin92tqHBrBwl0Q+0xkhVmreyXxO/ +FTfoOkP67BDIvisWAvKAtIvmDzFsrXdKpZQMrvKDsv30rIyUXgLKDTfXE1GFKfSlRch lhnAFw3n6sIn6BfhmpPlz/DyQp4PJhMVjs4HY= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1727280590; x=1727885390; h=content-transfer-encoding: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=63WvnMi8FWs1j24Criu8FigofP8q38R0vhAOR7AQ9cY=; b=YP0CfcdRLyIy9RoZ3ykR/qyp1zzOIPfohmeA/5djmzKDywk7AsHFDzZmrVaM73j2Iu gNksEmjNFKft3BSQD1m6zTD1RnQ0FvJ8xX5eZvqTDHt1F1lt/UX2/MUNlI/gBOnynmW1 1VqSnbrjZS8cCv0+5psTarINjL1FXTpAzSCPjzfTNdz/ogSVtePpAIT93Q0XmQ1kOje1 SeCeXGbql0Oh1DYh8rtFYllZ0h4Z1FQJ7/m6k8QEcfGz7CK9t9q/aUDrsqqV/19FLcpL Xax88olxhlRQ4QrqDv+Ev9/GXjbgxREUre+gGnER3raXMm6ZjgQrq7yS8jgHuiAPYcW/ vs/w== X-Forwarded-Encrypted: i=1; AJvYcCV0RIlYjH/NI23DKJNKolMlkGdpWcx275+ZsAgPkWumAY6PfoV+EAiodqT+j6Ho7C4pReo=@dpdk.org X-Gm-Message-State: AOJu0Yx7TQ2/z62Wcw396Lno09wWH/4nUSHN3BP7xN7Vi53HIR63lmvy f6mH+IhMOh4UBMseNcBv86fWV+wnFNacMwjzhA5Xxxmc2CZfPzc14Md9zppN74iFi53wkQSmr8q vSUYxofgEd+WEdftcPt9Bm4PSANh+MQI0VeE7nA== X-Google-Smtp-Source: AGHT+IHP90K5o4Desbgy5B+P8NN7aunU/apyiYWGIfa12PwS9WjAHWQgbOFPHbL2U9woJEyQhPN3IJivHEjSGtufwxQ= X-Received: by 2002:a17:90a:db8d:b0:2d8:adfc:b3d3 with SMTP id 98e67ed59e1d1-2e090ed243cmr262333a91.0.1727280590356; Wed, 25 Sep 2024 09:09:50 -0700 (PDT) MIME-Version: 1.0 References: <20240904152827.11914-1-jspewock@iol.unh.edu> <20240920180852.34792-1-jspewock@iol.unh.edu> <20240920180852.34792-2-jspewock@iol.unh.edu> In-Reply-To: From: Jeremy Spewock Date: Wed, 25 Sep 2024 12:09:31 -0400 Message-ID: Subject: Re: [PATCH v2 1/1] dts: add send_packets to test suites and rework packet addressing To: =?UTF-8?Q?Juraj_Linke=C5=A1?= Cc: paul.szczepanek@arm.com, Honnappa.Nagarahalli@arm.com, probb@iol.unh.edu, alex.chapman@arm.com, thomas@monjalon.net, Luca.Vizzarro@arm.com, npratte@iol.unh.edu, yoan.picchi@foss.arm.com, wathsala.vithanage@arm.com, dev@dpdk.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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 On Tue, Sep 24, 2024 at 10:30=E2=80=AFAM Juraj Linke=C5=A1 wrote: > > > > On 20. 9. 2024 20:08, jspewock@iol.unh.edu wrote: > > From: Jeremy Spewock > > > > Currently the only method provided in the test suite class for sending > > packets sends a single packet and then captures the results. There is, > > in some cases, a need to send multiple packets at once while not really > > needing to capture any traffic received back. The method to do this > > exists in the traffic generator already, but this patch exposes the > > method to test suites. > > > > Patrick mentioned there is now a method that does it > (send_packets_and_capture()), but that one captures the packets. I think > we should add this method though, as it does something different, but > maybe the most important point is that non-capturing TGs are going to > support send_packets_and_capture(), so we need it either way. > > > This patch also updates the _adjust_addresses method of test suites so > > that addresses of packets are only modified if the developer did not > > configure them beforehand. This allows for developers to have more > > control over the content of their packets when sending them through the > > framework. > > > > I think this could and should be split into two patches. They aren't > really connected. Sure, this makes sense. They were in the same one originally when this was tied to the test suite I was writing it for because they were just smaller framework updates that allowed the suite to function. Probably didn't make much sense to keep them together then, but it makes even less sense when the patch is standalone like this. > > > > diff --git a/dts/framework/test_suite.py b/dts/framework/test_suite.py > > > @@ -243,41 +255,74 @@ def get_expected_packet(self, packet: Packet) -> = Packet: > > Returns: > > `packet` with injected L2/L3 addresses. > > """ > > - return self._adjust_addresses(packet, expected=3DTrue) > > + return self._adjust_addresses([packet], expected=3DTrue)[0] > > > > - def _adjust_addresses(self, packet: Packet, expected: bool =3D Fal= se) -> Packet: > > + def _adjust_addresses(self, packets: list[Packet], expected: bool = =3D False) -> list[Packet]: > > """L2 and L3 address additions in both directions. > > > > + Packets in `packets` will be directly modified in this method.= The returned list of packets > > + however will be copies of the modified packets in order to kee= p the two lists distinct. > > + > > Do we need to do this? I guess you needed this in a test suite - what is > the reason? This is actually just me documenting something that was already happening behind the scenes in this method that I thought would be helpful for people to know. Right now this method ends in `Ether(packet.build())` which essentially just copies the packet, but it does so after the modifications were made. I think there is some logical reason to do this which is this method is called internally in the framework, so it allows the send methods where these packets are used to essentially modify the list and do whatever they want to/need to to get the packets sent and the only change that is reflected on the user is the addresses are added. > > > + Only missing addresses are added to packets, existing addresse= s will not be overridden. If > > + any packet in `packets` has multiple IP layers (using GRE, for= example) only the inner-most > > + IP layer will have its addresses adjusted. > > + > > Assumptions: > > Two links between SUT and TG, one link is TG -> SUT, the = other SUT -> TG. > > > > Args: > > - packet: The packet to modify. > > + packets: The packets to modify. > > expected: If :data:`True`, the direction is SUT -> TG, > > otherwise the direction is TG -> SUT. > > + > > + Returns: > > + A list containing copies of all packets in `packets` after= modification. > > """ > > - if expected: > > - # The packet enters the TG from SUT > > - # update l2 addresses > > - packet.src =3D self._sut_port_egress.mac_address > > - packet.dst =3D self._tg_port_ingress.mac_address > > + ret_packets =3D [] > > + for packet in packets: > > + # The fields parameter of a packet does not include fields= of the payload, so this can > > + # only be the Ether src/dst. > > + pkt_src_is_unset =3D "src" not in packet.fields > > + pkt_dst_is_unset =3D "dst" not in packet.fields > > + num_ip_layers =3D packet.layers().count(IP) > > + > > + # Update the last IP layer if there are multiple to accoun= t for GRE addressing (the > > This comment should be more generic as it's going to apply to other > things than just GRE. Ack. > > > + # framework should be modifying the packet address instead= of the tunnel). > > + l3_to_use =3D packet.getlayer(IP, num_ip_layers) > > Would this make more sense inside the positive if branch right below? Yeah, it could be defined in the positive branch. The main reason I left it out was just because it was also used later in the method, but the only way that part of the code is reached is if this positive branch is hit anyway. I can update it. > > > + if num_ip_layers > 0: > > + ip_src_is_unset =3D "src" not in l3_to_use.fields > > + ip_dst_is_unset =3D "dst" not in l3_to_use.fields > > + else: > > + ip_src_is_unset =3D None > > + ip_dst_is_unset =3D None > > > > - # The packet is routed from TG egress to TG ingress > > - # update l3 addresses > > - packet.payload.src =3D self._tg_ip_address_egress.ip.explo= ded > > - packet.payload.dst =3D self._tg_ip_address_ingress.ip.expl= oded > > - else: > > - # The packet leaves TG towards SUT > > # update l2 addresses > > - packet.src =3D self._tg_port_egress.mac_address > > - packet.dst =3D self._sut_port_ingress.mac_address > > + # If `expected` is :data:`True`, the packet enters the TG = from SUT, otherwise the > > + # packet leaves the TG towards the SUT > > + if pkt_src_is_unset: > > + packet.src =3D ( > > + self._sut_port_egress.mac_address > > + if expected > > + else self._tg_port_egress.mac_address > > + ) > > + if pkt_dst_is_unset: > > + packet.dst =3D ( > > + self._tg_port_ingress.mac_address > > + if expected > > + else self._sut_port_ingress.mac_address > > + ) > > > > - # The packet is routed from TG egress to TG ingress > > # update l3 addresses > > - packet.payload.src =3D self._tg_ip_address_egress.ip.explo= ded > > - packet.payload.dst =3D self._tg_ip_address_ingress.ip.expl= oded > > + # The packet is routed from TG egress to TG ingress regard= less of whether it is > > + # expected or not. > > Is this true? This might've been an error in the original > implementation. If it's expected (that is, the returning packet), it > should be routed from TG ingress to TG egress, no? I guess I'm not completely sure. It would make sense that the L3 addresses should be switched as well based on either expected or not, but currently it isn't modified, and os_udp still works which makes me think the addresses aren't switched by the kernel interfaces, which I believe is the only time these addresses are actually used (since we use sendp in scapy). > > > + if ip_src_is_unset: > > + l3_to_use.src =3D self._tg_ip_address_egress.ip.explod= ed > > + > > + if ip_dst_is_unset: > > + l3_to_use.dst =3D self._tg_ip_address_ingress.ip.explo= ded > > + ret_packets.append(Ether(packet.build())) > > > >