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 2F79E456BE; Fri, 26 Jul 2024 21:00:23 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 0A76241133; Fri, 26 Jul 2024 21:00:23 +0200 (CEST) Received: from mail-lj1-f171.google.com (mail-lj1-f171.google.com [209.85.208.171]) by mails.dpdk.org (Postfix) with ESMTP id D972140E6E for ; Fri, 26 Jul 2024 21:00:20 +0200 (CEST) Received: by mail-lj1-f171.google.com with SMTP id 38308e7fff4ca-2ef18ca13f2so2891211fa.3 for ; Fri, 26 Jul 2024 12:00:20 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=iol.unh.edu; s=unh-iol; t=1722020420; x=1722625220; 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=JOWiomXgkj/3Ve12A30xVDtzbu4FJ684Qlnmg2izWGk=; b=FMwmUw+mMr2hkwNpR4qtlkEdK1oC5P8s4Vhz4G7VMORE09M5Sq74zvgZ70DCfvdwv2 /yV7jzdNaDznoSx04/bVhTYR/mFF+KudJXHTpQ5Nd8oqYquZHm3bAdmVPKMbfvGpdOHx tCM1CdFmXnpnrhXqYGfVGbe4dUpMFKI3NpVVs= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1722020420; x=1722625220; 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=JOWiomXgkj/3Ve12A30xVDtzbu4FJ684Qlnmg2izWGk=; b=c6B8usFKp+sN5zdOSarhNzsC6R1sZqlIu3l40xvJWp7IwI9YRFwpbRyspui4/M5PB8 3CBYz6Qw/c+dL0ysGXAAnhydklfXl3L+zUrEkUUwhc8Kqs5McWI5UrB57P8hUoGvq/L4 Q/9+xtvkbrzAdAB6jvTQIr2Fnr9t2LtBmNjN/H+t8ox9NBKz0b40wVV8Sm7lyw5zy12Z /b+zUDipu4A3FhSXeRMLd0uBySTKRRUIGlfDAEZhdpBXhyiBV+XwT1xRr7BFnCvt7Vq5 9X5sOxYDypl5RiVF0NaMsO6TamSZ6tDRbbHoAVfW7J8XIsl5/HTPucrycMLtESyOalmB KThA== X-Forwarded-Encrypted: i=1; AJvYcCVinxCSaGokyIiJKSxRlgb/LX9RUSWElDfRDj2gJ5HUA2oVvy5FSL7fsLa4yOMHINMUP+V7Ea2MMjNm5X4= X-Gm-Message-State: AOJu0YywMsizDngjckLBP8ES/X3Ct3BiBi0KxSYvmaZRIMYQY+r3Zf+j xeV+jKoAhncxcCUxi5EjRpfes0Mgbyo4CPo7AmxBX3w11UfuKDoGJURiZTXaGJHZY5+2EhccENi hiQXNDVfmmsnQFHgCZQrV0ka73ldNR+VDRUIoVA== X-Google-Smtp-Source: AGHT+IGQEA/DJSGTUchQY8Ku40i7erWWe9B7C0Ni7BzefHe+6OfB1GHsNByt/OFxAl0Ftdriv+Kc3GiUjuNCTHIGauQ= X-Received: by 2002:a2e:2206:0:b0:2ef:24a9:6aa8 with SMTP id 38308e7fff4ca-2f03c603434mr25805451fa.0.1722020419921; Fri, 26 Jul 2024 12:00:19 -0700 (PDT) MIME-Version: 1.0 References: <20240625155332.2400-1-jspewock@iol.unh.edu> <20240724150714.226970-1-jspewock@iol.unh.edu> <20240724150714.226970-2-jspewock@iol.unh.edu> In-Reply-To: <20240724150714.226970-2-jspewock@iol.unh.edu> From: Nicholas Pratte Date: Fri, 26 Jul 2024 15:00:08 -0400 Message-ID: Subject: Re: [PATCH v3 1/4] dts: add send_packets to test suites and rework packet addressing To: jspewock@iol.unh.edu Cc: thomas@monjalon.net, Luca.Vizzarro@arm.com, Honnappa.Nagarahalli@arm.com, probb@iol.unh.edu, yoan.picchi@foss.arm.com, wathsala.vithanage@arm.com, juraj.linkes@pantheon.tech, paul.szczepanek@arm.com, dev@dpdk.org Content-Type: text/plain; charset="UTF-8" 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 I'll make sure to look over the other parts of this series and leave reviews at some point next week, but I prioritized this since I will be using this patch at some point in my GRE suites. > diff --git a/dts/framework/test_suite.py b/dts/framework/test_suite.py > index 694b2eba65..0b678ed62d 100644 > --- a/dts/framework/test_suite.py > +++ b/dts/framework/test_suite.py > @@ -199,7 +199,7 @@ def send_packet_and_capture( > Returns: > A list of received packets. > """ > - packet = self._adjust_addresses(packet) > + packet = self._adjust_addresses([packet])[0] > return self.tg_node.send_packet_and_capture( > packet, > self._tg_port_egress, > @@ -208,6 +208,18 @@ def send_packet_and_capture( > duration, > ) > > + def send_packets( > + self, > + packets: list[Packet], > + ) -> None: > + """Send packets using the traffic generator and do not capture received traffic. > + > + Args: > + packets: Packets to send. > + """ > + packets = self._adjust_addresses(packets) > + self.tg_node.send_packets(packets, self._tg_port_egress) > + > def get_expected_packet(self, packet: Packet) -> Packet: > """Inject the proper L2/L3 addresses into `packet`. > > @@ -219,39 +231,59 @@ def get_expected_packet(self, packet: Packet) -> Packet: > """ > return self._adjust_addresses(packet, expected=True) > > - def _adjust_addresses(self, packet: Packet, expected: bool = False) -> Packet: > + def _adjust_addresses(self, packets: list[Packet], expected: bool = False) -> list[Packet]: > """L2 and L3 address additions in both directions. > > + Only missing addresses are added to packets, existing addressed will not be overridden. addressed should be addresses. Only saw this because of Chrome's built-in grammar correction. > + > 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. > """ > - if expected: > - # The packet enters the TG from SUT > - # update l2 addresses > - packet.src = self._sut_port_egress.mac_address > - packet.dst = self._tg_port_ingress.mac_address > + ret_packets = [] > + for packet in packets: > + default_pkt_src = type(packet)().src > + default_pkt_dst = type(packet)().dst This is really just a probing question for my sake, but what is the difference between the solution you have above type(packet)().src and Ether().src? Is there a preferred means of doing this? > + default_pkt_payload_src = IP().src if hasattr(packet.payload, "src") else None > + default_pkt_payload_dst = IP().dst if hasattr(packet.payload, "dst") else None > + # If `expected` is :data:`True`, the packet enters the TG from SUT, otherwise the > + # packet leaves the TG towards the SUT > > - # The packet is routed from TG egress to TG ingress > - # update l3 addresses > - packet.payload.src = self._tg_ip_address_egress.ip.exploded > - packet.payload.dst = self._tg_ip_address_ingress.ip.exploded This is where it gets a little tricky. There will be circumstances, albeit probably infrequently, where a user-created packet has more than one IP layer, such as the ones I am using in the ipgre and nvgre test suites that I am writing. In these cases, you need to specify an index of the IP layer you want to modify, otherwise it will modify the outermost IP layer in the packet (the IP layer outside the GRE layer. See my previous comment for an example packet). Should be pretty easy to fix, you just need to check if a packet contains an GRE layer, and if it does, modify the packet by doing something like packet[IP][1].src = self._tg_ip_address_egress.ip.exploded. > - else: > - # The packet leaves TG towards SUT > # update l2 addresses > - packet.src = self._tg_port_egress.mac_address > - packet.dst = self._sut_port_ingress.mac_address You wouldn't need to make changes to how Ether addresses get allocated if accounting for GRE as described above, since I'm pretty sure there aren't really circumstances where packets would have more than one Ethernet header, at least not that I've seen (GRE packets only have one). > + if packet.src == default_pkt_src: > + packet.src = ( > + self._sut_port_egress.mac_address > + if expected > + else self._tg_port_egress.mac_address > + ) > + if packet.dst == default_pkt_dst: > + packet.dst = ( > + 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 regardless of if it is expected or Maybe change 'regardless of if it is expected' to 'regardless of whether it is expected.' It's admittedly picky of me, but I think it reads a little bit better. > + # not. > > - # The packet is routed from TG egress to TG ingress > # update l3 addresses > - packet.payload.src = self._tg_ip_address_egress.ip.exploded > - packet.payload.dst = self._tg_ip_address_ingress.ip.exploded > - > - return Ether(packet.build()) > + if ( > + default_pkt_payload_src is not None > + and packet.payload.src == default_pkt_payload_src > + ): > + packet.payload.src = self._tg_ip_address_egress.ip.exploded > + if ( > + default_pkt_payload_dst is not None > + and packet.payload.dst == default_pkt_payload_dst > + ): > + packet.payload.dst = self._tg_ip_address_ingress.ip.exploded > + ret_packets.append(Ether(packet.build())) > + > + return ret_packets > > def verify(self, condition: bool, failure_description: str) -> None: > """Verify `condition` and handle failures. > diff --git a/dts/framework/testbed_model/tg_node.py b/dts/framework/testbed_model/tg_node.py > index 4ee326e99c..758b676258 100644 > --- a/dts/framework/testbed_model/tg_node.py > +++ b/dts/framework/testbed_model/tg_node.py > @@ -83,6 +83,15 @@ def send_packet_and_capture( > duration, > ) > > + def send_packets(self, packets: list[Packet], port: Port): > + """Send packets without capturing resulting received packets. > + > + Args: > + packets: Packets to send. > + port: Port to send the packets on. > + """ > + self.traffic_generator.send_packets(packets, port) > + > def close(self) -> None: > """Free all resources used by the node. > > -- > 2.45.2 >