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 BB1AF459D3; Thu, 19 Sep 2024 11:02:53 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 41F11402D4; Thu, 19 Sep 2024 11:02:53 +0200 (CEST) Received: from mail-ej1-f45.google.com (mail-ej1-f45.google.com [209.85.218.45]) by mails.dpdk.org (Postfix) with ESMTP id CAA84400D5 for ; Thu, 19 Sep 2024 11:02:51 +0200 (CEST) Received: by mail-ej1-f45.google.com with SMTP id a640c23a62f3a-a8d43657255so84587166b.0 for ; Thu, 19 Sep 2024 02:02:51 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pantheon.tech; s=google; t=1726736571; x=1727341371; 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=evxkKCcSI6E9LllS8ZQIf3bHtRbaUA+NMFerJZ/VG6k=; b=arHWz3fUp0+ZU1cIuskhv3AQmelFKzKAym8OdMfFU0DttMotEsKv/wKBbZYpRzaJs2 1JVNFJEiHOhkiMDe4x5gRpyWPOWLuwrsyRUbRNQt8HJXzZP/XqVFPAI9aKcDXdi9tlDq 9y9q9bOKHxd2NZl2d4I9KHmHyowYb6T+q1K4s8BnlIitjkiTU/KHW1ZHaEMenngKOM90 eeYL9ipk6BjOhzY9ONGCVP09jtZPmZe9lUxRolFyqm5JmP8/yr8QbYgiNe9D/qPb8Gmd 9BL1OhwH3cjOFnhxaTGm40+pAGnax5y3AduE6iN1E4ABfPSKehIkSZLOAtUoCanCo8f1 eQAw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1726736571; x=1727341371; 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=evxkKCcSI6E9LllS8ZQIf3bHtRbaUA+NMFerJZ/VG6k=; b=ApBhXXfTWVYJXRLu2E61IBmkzitYl3EmAMqAuA2AHIo1VT+jxJ64lld9/miB0tQf7N +Hm+/YvSpoiyFrHdt2+w8zyrEQC5s/+0Oq0U1u+NYiBfxxKnGAbv8NwPU51m+sy7d2+R qjxnfzrJIQYPKbvXfTXb0OkN0az3wTot1OwRwr7F/tLgOONbH3Tn3JNateyNcXI+Z0/H Yo5jg54kRG4NIQTKoDT8cvDvmEg2Vcr0tO6uy7WzeI2EDerlEsHpj4h8mv1HLqV6JxRC faheWloBhfek6DrQ6TTN7BKVpZRhM80LFeFO2pDJzAc3bYI/gwCrimOD32hI5yxOhPrW xNOw== X-Gm-Message-State: AOJu0YxPHqYDjFF+fA3Tx8R3fInq2KAMibu3IrJcg3c1zhrlPkYWe1DU BKShkOxxFIMrnVlKtXkriUGoCjKVpOUT8CDqFTO7TVdhthAiJK3haD4Cyjtl3b/aI1QLHrKUhNd H5iA= X-Google-Smtp-Source: AGHT+IEXpuEPhLdpStH8casOeSb+k5daYW3fryzyhlFIfqahtO4LtKItwdpC8+odTEoU6g5/Bu05Cw== X-Received: by 2002:a17:907:7da1:b0:a86:96ca:7f54 with SMTP id a640c23a62f3a-a9047ca3bebmr2034682866b.21.1726736571061; Thu, 19 Sep 2024 02:02:51 -0700 (PDT) Received: from [192.168.200.22] ([84.245.121.62]) by smtp.gmail.com with ESMTPSA id a640c23a62f3a-a90610f43c7sm692126266b.62.2024.09.19.02.02.50 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 19 Sep 2024 02:02:50 -0700 (PDT) Message-ID: <92be8e4d-cfc6-4b07-b014-1d9dec051b91@pantheon.tech> Date: Thu, 19 Sep 2024 11:02:49 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v5 1/1] dts: add text parser for testpmd verbose output To: jspewock@iol.unh.edu, alex.chapman@arm.com, paul.szczepanek@arm.com, Luca.Vizzarro@arm.com, Honnappa.Nagarahalli@arm.com, wathsala.vithanage@arm.com, probb@iol.unh.edu, npratte@iol.unh.edu, thomas@monjalon.net, yoan.picchi@foss.arm.com Cc: dev@dpdk.org References: <20240729203955.267942-1-jspewock@iol.unh.edu> <20240918170528.14545-1-jspewock@iol.unh.edu> <20240918170528.14545-2-jspewock@iol.unh.edu> Content-Language: en-US From: =?UTF-8?Q?Juraj_Linke=C5=A1?= In-Reply-To: <20240918170528.14545-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 > diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py > @@ -577,6 +577,497 @@ class TestPmdPortStats(TextParser): > tx_bps: int = field(metadata=TextParser.find_int(r"Tx-bps:\s+(\d+)")) > > > +class PacketOffloadFlag(Flag): > + """Flag representing the Packet Offload Features Flags in DPDK. > + > + Values in this class are taken from the definitions in the RTE MBUF core library in DPDK > + located in lib/mbuf/rte_mbuf_core.h. It is expected that flag values in this class will match > + the values they are set to in said DPDK library with one exception; all values must be unique. > + For example, the definitions for unknown checksum flags in rte_mbuf_core.h are all set to > + :data:`0`, but it is valuable to distinguish between them in this framework. For this reason > + flags that are not unique in the DPDK library are set either to values within the > + RTE_MBUF_F_FIRST_FREE-RTE_MBUF_F_LAST_FREE range for Rx or shifted 61+ bits for Tx. > + """ > + #: No information about the RX IP checksum. > + RTE_MBUF_F_RX_IP_CKSUM_UNKNOWN = 1 << 23 Good idea with the UKNOWN flag values. > + #: The IP checksum in the packet is wrong. > + RTE_MBUF_F_RX_IP_CKSUM_BAD = 1 << 4 > + #: The IP checksum in the packet is valid. > + RTE_MBUF_F_RX_IP_CKSUM_GOOD = 1 << 7 I see you kept the order and just used the corresponding flag values. Makes sense. > + #: > + RTE_MBUF_F_TX_TUNNEL_VXLAN = 1 << 45 > + #: > + RTE_MBUF_F_TX_TUNNEL_GRE = 2 << 45 > + #: > + RTE_MBUF_F_TX_TUNNEL_IPIP = 3 << 45 > + #: > + RTE_MBUF_F_TX_TUNNEL_GENEVE = 4 << 45 > + """ TX packet with MPLS-in-UDP RFC 7510 header. """ This should be one line below after :# > + #: > + RTE_MBUF_F_TX_TUNNEL_MPLSINUDP = 5 << 45 > + #: > + RTE_MBUF_F_TX_TUNNEL_VXLAN_GPE = 6 << 45 > + #: > + RTE_MBUF_F_TX_TUNNEL_GTP = 7 << 45 > + #: > + RTE_MBUF_F_TX_TUNNEL_ESP = 8 << 45 So the DPDK code mixes values withing flags? Would this work? We have to be careful with how we use this: PacketOffloadFlag.RTE_MBUF_F_TX_TUNNEL_VXLAN | PacketOffloadFlag.RTE_MBUF_F_TX_TUNNEL_GRE == PacketOffloadFlag.RTE_MBUF_F_TX_TUNNEL_IPIP True PacketOffloadFlag.RTE_MBUF_F_TX_TUNNEL_VXLAN | PacketOffloadFlag.RTE_MBUF_F_TX_TUNNEL_GRE is PacketOffloadFlag.RTE_MBUF_F_TX_TUNNEL_IPIP True PacketOffloadFlag.RTE_MBUF_F_TX_TUNNEL_VXLAN in PacketOffloadFlag.RTE_MBUF_F_TX_TUNNEL_IPIP True The combination of 1 | 2 == 3, even identity returns True and one flag is part of another. If we're looking at verbose_output.ol_flags and checking the RTE_MBUF_F_TX_TUNNEL_VXLAN flag, True would be returned for all flag that have the first bit set: RTE_MBUF_F_TX_TUNNEL_VXLAN RTE_MBUF_F_TX_TUNNEL_IPIP RTE_MBUF_F_TX_TUNNEL_MPLSINUDP RTE_MBUF_F_TX_TUNNEL_GTP Do you know how this is handled in DPDK? Or how testpmd processes this to return the proper flag? This mixing seems pretty wild to me (I guess this is to not waste space, since ULL is max 64 bits). We need to think this through thoroughly. > + #: TCP cksum of TX pkt. Computed by NIC. > + RTE_MBUF_F_TX_TCP_CKSUM = 1 << 52 > + #: SCTP cksum of TX pkt. Computed by NIC. > + RTE_MBUF_F_TX_SCTP_CKSUM = 2 << 52 > + #: UDP cksum of TX pkt. Computed by NIC. > + RTE_MBUF_F_TX_UDP_CKSUM = 3 << 52 This is the same thing as above. > + @classmethod > + def from_str_list(cls, arr: list[str]) -> Self: > + """Makes an instance from a list containing the flag members. > + > + Args: > + arr: A list of strings containing ol_flag values. > + > + Returns: > + A new instance of the flag. > + """ > + flag = cls(0) > + for name in arr: > + if hasattr(cls, name): So you used hasattr instead of cls[name] in cls. Is this to avoid the exception? I now realize that if we could ignore the exception then we won't need the condition. The question is when the exception would be raised, or, in other words, what should we do when hasattr(cls, name) is False. If I understand this correctly, is it's False, then name is not among the flags and that means testpmd returned an unsupported flag, which shouldn't happen, but if it does in the future, we would be better off throwing an exception, or at very least, log a warning, so that we have an indication that we need to add support for a new flag. > + flag |= cls[name] > + return flag > + > + @classmethod > + def make_parser(cls) -> ParserFn: > + """Makes a parser function. > + > + Returns: > + ParserFn: A dictionary for the `dataclasses.field` metadata argument containing a > + parser function that makes an instance of this flag from text. > + """ > + return TextParser.wrap( > + TextParser.wrap(TextParser.find(r"ol_flags: ([^\n]+)"), str.split), > + cls.from_str_list, > + ) The RSSOffloadTypesFlag does the split in its from_list_string method. Do we want to do the same here? Maybe could create a ParsableFlag (or Creatable? Or something else) superclass that would implement these from_* methods (from_list_string, from_str) and subclass it. Flags should be subclassable if they don't contain members. The superclass would be useful so that we don't redefine the same method over and over and so that it's clear what's already available. > @@ -656,6 +1147,9 @@ def stop(self, verify: bool = True) -> None: > Raises: > InteractiveCommandExecutionError: If `verify` is :data:`True` and the command to stop > forwarding results in an error. > + > + Returns: > + Output gathered from sending the stop command. This not just from sending the stop command, but everything else that preceded (when collecting the verbose output), right? > diff --git a/dts/framework/utils.py b/dts/framework/utils.py > @@ -27,6 +27,12 @@ > from .exception import ConfigurationError > > REGEX_FOR_PCI_ADDRESS: str = "/[0-9a-fA-F]{4}:[0-9a-fA-F]{2}:[0-9a-fA-F]{2}.[0-9]{1}/" > +_REGEX_FOR_COLON_SEP_MAC: str = r"(?:[\da-fA-F]{2}:){5}[\da-fA-F]{2}" > +_REGEX_FOR_HYPHEN_SEP_MAC: str = r"(?:[\da-fA-F]{2}-){5,7}[\da-fA-F]{2}" {5,7} should be just 5 repetitions. When could it be more?