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 4439745A03; Tue, 24 Sep 2024 16:30:09 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id F180840274; Tue, 24 Sep 2024 16:30:08 +0200 (CEST) Received: from mail-ed1-f54.google.com (mail-ed1-f54.google.com [209.85.208.54]) by mails.dpdk.org (Postfix) with ESMTP id 43033400D5 for ; Tue, 24 Sep 2024 16:30:07 +0200 (CEST) Received: by mail-ed1-f54.google.com with SMTP id 4fb4d7f45d1cf-5c5bca59416so3394896a12.1 for ; Tue, 24 Sep 2024 07:30:07 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pantheon.tech; s=google; t=1727188207; x=1727793007; darn=dpdk.org; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=jvG4MDFGBB7taLwCqOnWOjjywR7vUie+IUkHJem7H8M=; b=lIcmtiMuRZiDSM8bIm3A7SsgHOgkU56ErML1Z7nMoNUIJHzHx4dbXlKEI+3/PWPlZf bd6yCvlzxlNcr81sUOB+VgCY8837Qrm1EkF45er7T0lYNAyryRqO0ii9bE7wukKeDd9b oCqpFKoqC4oEm6faywNGio3ldcm1O/TSe9Yf/0oX/L5tmBMKkyGNAOKA6p1quVms/LLU T+Qrt7VLtWDe+H1bEMb0BXbNDmWGGvsr21X5gWIB+xTAphdCBegv3yu/cc7cLqA3z+e5 x1/bYMS9iONTHvnWrUjJ5FI0NaQIvxx+rHVy9dcysVLcRCuADct+bcroBT+swEE0EYuC P0GA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1727188207; x=1727793007; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=jvG4MDFGBB7taLwCqOnWOjjywR7vUie+IUkHJem7H8M=; b=nUdBgIqE5YmhMRkv3T8clMJWoa9dI1+GPEhhVUOWt35FplDfncxWzcPgbxkTAtWnxu MfRZEAeQH+OgGY+HeE/XA0mBoLS7x9IItwtJ4tLUatOISCHpdbEUH2p+MkXlhpP1bUzh hYOy8BNZysOz1TUd4bvo47HZ67cxMii87H8FU4kcahxhdqM3K8Q5SnX5LdWfbjQUxPhb gjERsY/bDAb0Gh+fAjrQwfLHlmSqF80eVkyYzVlQz+DYYMRI2GsjhaxOYslg4f0wYJ2d 3UaBQTc9+g9mlascekVvaLYtP2Cxt52KvzrnVmGXsmrVvbyNy6r3RAeZ3KZyrF3XLNmn TyGA== X-Gm-Message-State: AOJu0YxZHYldm2sOZy0nrviG2uX9hQe3owQjJh3liSplsh2CL3dCjr/I qRQ4v1+kR2Nuzd/AeKDQ35GpuHB89+Pi8w+6t4Y5VKbkx5mbwfdu/tituHqshn0= X-Google-Smtp-Source: AGHT+IH6CVatxzTPfZTDgZ5oHGXTmItDA40ZPdqERCeFBwJnlWUXRMaGgJW9V3T1DdeexTcv6yF0+w== X-Received: by 2002:a05:6402:274a:b0:5c5:c5fb:57d5 with SMTP id 4fb4d7f45d1cf-5c5c5fb5846mr3748639a12.33.1727188206604; Tue, 24 Sep 2024 07:30:06 -0700 (PDT) Received: from [192.168.200.22] ([84.245.121.62]) by smtp.gmail.com with ESMTPSA id 4fb4d7f45d1cf-5c5cf49db0bsm802158a12.50.2024.09.24.07.30.05 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 24 Sep 2024 07:30:06 -0700 (PDT) Message-ID: Date: Tue, 24 Sep 2024 16:30:04 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v2 1/1] dts: add send_packets to test suites and rework packet addressing To: jspewock@iol.unh.edu, 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 Cc: dev@dpdk.org References: <20240904152827.11914-1-jspewock@iol.unh.edu> <20240920180852.34792-1-jspewock@iol.unh.edu> <20240920180852.34792-2-jspewock@iol.unh.edu> Content-Language: en-US From: =?UTF-8?Q?Juraj_Linke=C5=A1?= In-Reply-To: <20240920180852.34792-2-jspewock@iol.unh.edu> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit 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 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. > 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=True) > + return self._adjust_addresses([packet], expected=True)[0] > > - 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. > > + 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 keep the two lists distinct. > + Do we need to do this? I guess you needed this in a test suite - what is the reason? > + Only missing addresses are added to packets, existing addresses 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 = self._sut_port_egress.mac_address > - packet.dst = self._tg_port_ingress.mac_address > + ret_packets = [] > + 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 = "src" not in packet.fields > + pkt_dst_is_unset = "dst" not in packet.fields > + num_ip_layers = packet.layers().count(IP) > + > + # Update the last IP layer if there are multiple to account for GRE addressing (the This comment should be more generic as it's going to apply to other things than just GRE. > + # framework should be modifying the packet address instead of the tunnel). > + l3_to_use = packet.getlayer(IP, num_ip_layers) Would this make more sense inside the positive if branch right below? > + if num_ip_layers > 0: > + ip_src_is_unset = "src" not in l3_to_use.fields > + ip_dst_is_unset = "dst" not in l3_to_use.fields > + else: > + ip_src_is_unset = None > + ip_dst_is_unset = None > > - # 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 > - 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 > + # 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 = ( > + self._sut_port_egress.mac_address > + if expected > + else self._tg_port_egress.mac_address > + ) > + if pkt_dst_is_unset: > + 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 > # update l3 addresses > - packet.payload.src = self._tg_ip_address_egress.ip.exploded > - packet.payload.dst = self._tg_ip_address_ingress.ip.exploded > + # The packet is routed from TG egress to TG ingress regardless 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? > + if ip_src_is_unset: > + l3_to_use.src = self._tg_ip_address_egress.ip.exploded > + > + if ip_dst_is_unset: > + l3_to_use.dst = self._tg_ip_address_ingress.ip.exploded > + ret_packets.append(Ether(packet.build())) >