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 E340A459D7; Fri, 20 Sep 2024 17:54:08 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 6BF0F433A0; Fri, 20 Sep 2024 17:54:08 +0200 (CEST) Received: from mail-pg1-f172.google.com (mail-pg1-f172.google.com [209.85.215.172]) by mails.dpdk.org (Postfix) with ESMTP id 58E184067E for ; Fri, 20 Sep 2024 17:54:07 +0200 (CEST) Received: by mail-pg1-f172.google.com with SMTP id 41be03b00d2f7-7d7a9200947so1418038a12.3 for ; Fri, 20 Sep 2024 08:54:07 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=iol.unh.edu; s=unh-iol; t=1726847646; x=1727452446; 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=L99ImdXUjYCFDXJ1aePKmaKJg4i3FR9aZTZoWXmrApE=; b=JtlKU5Bw+u7KCR99DDirid3cdXVhVyR+g8LPksk9x9RfINg1fNC2yyFbU9B+eU7zpZ w5lWvRF2JVJ5+V7L8BY/vA79tqEKiydq3E4LMYVxpyRprLGoqvTHTpAu3ATlBcQhpHeK 2pClVYCL7rxMknP8yq3TK4lP/9yaaFV3G63Vg= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1726847646; x=1727452446; 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=L99ImdXUjYCFDXJ1aePKmaKJg4i3FR9aZTZoWXmrApE=; b=Qtf/TPzcldsr5qcFUluIoxVaKp29hfw7yzJrg9U1vFGPtamtfrWN4KAhw9v5eOWaP4 AYTSv6CsIkvAAkTk1EdkB+rI9tsKRknw265pydgs72G+TOK/9tHDqbwTGDT4INzIfGBb MuTUTmwP1osAs+6QP3jjAkqN8/evY+HvjZcWZo4PXrB27hiYJ7OJIf+SCyjM44IIiNyE PS+XhzYo3R1DGv+GBVTEUpn+QC6ZYvo6SizWQmSE42bRIN8PG9KZO6XiMU+CSdE5FN3o uz2zDju4Y95ubhMnJiCGWK6c/Ki4lDirxnzR6siCK6NoDbHfT9QJV1fbK152FnBcMVwg Nvbg== X-Forwarded-Encrypted: i=1; AJvYcCWPgvQovv36Q0R2ExbtpufWfSZUAUtXL2HSpI7iIMI/Yr1BFJS6BkL3dOrCZMG7w45hWNY=@dpdk.org X-Gm-Message-State: AOJu0Yyh2qHfa+XPr9jkltke98TlZB4X53zIY4FVylVLXyOXV5jWcIXB FAoxqrGCiuhdhJrcLSROSqebWFX9Kc1ohrkPWCUpFQrMroUyLkWag6+vAjxzwVicxVpvzBJUl8a AYVy/gdGb/bR9e9+Ph8aMxBkZ9QbLcjrTz+vHeg== X-Google-Smtp-Source: AGHT+IGhEUI8P48yecHEZTJbklWhGl9GCaz0/twlEr664lQeCgi84Kc/hP2Ye7PK8P93BZWwe2QcgQhNiwMlE2CfQEc= X-Received: by 2002:a05:6a20:9d92:b0:1d1:17c6:7a34 with SMTP id adf61e73a8af0-1d30a9d0368mr5637624637.45.1726847646139; Fri, 20 Sep 2024 08:54:06 -0700 (PDT) MIME-Version: 1.0 References: <20240729203955.267942-1-jspewock@iol.unh.edu> <20240918170528.14545-1-jspewock@iol.unh.edu> <20240918170528.14545-2-jspewock@iol.unh.edu> <92be8e4d-cfc6-4b07-b014-1d9dec051b91@pantheon.tech> In-Reply-To: <92be8e4d-cfc6-4b07-b014-1d9dec051b91@pantheon.tech> From: Jeremy Spewock Date: Fri, 20 Sep 2024 11:53:54 -0400 Message-ID: Subject: Re: [PATCH v5 1/1] dts: add text parser for testpmd verbose output To: =?UTF-8?Q?Juraj_Linke=C5=A1?= Cc: 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, 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 Thu, Sep 19, 2024 at 5:02=E2=80=AFAM Juraj Linke=C5=A1 wrote: > > > diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framew= ork/remote_session/testpmd_shell.py > > > @@ -577,6 +577,497 @@ class TestPmdPortStats(TextParser): > > tx_bps: int =3D field(metadata=3DTextParser.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 MBU= F core library in DPDK > > + located in lib/mbuf/rte_mbuf_core.h. It is expected that flag valu= es 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_mbu= f_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 va= lues 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 =3D 1 << 23 > > Good idea with the UKNOWN flag values. > > > + #: The IP checksum in the packet is wrong. > > + RTE_MBUF_F_RX_IP_CKSUM_BAD =3D 1 << 4 > > + #: The IP checksum in the packet is valid. > > + RTE_MBUF_F_RX_IP_CKSUM_GOOD =3D 1 << 7 > > I see you kept the order and just used the corresponding flag values. > Makes sense. > > > > + #: > > + RTE_MBUF_F_TX_TUNNEL_VXLAN =3D 1 << 45 > > + #: > > + RTE_MBUF_F_TX_TUNNEL_GRE =3D 2 << 45 > > + #: > > + RTE_MBUF_F_TX_TUNNEL_IPIP =3D 3 << 45 > > + #: > > + RTE_MBUF_F_TX_TUNNEL_GENEVE =3D 4 << 45 > > + """ TX packet with MPLS-in-UDP RFC 7510 header. """ > > This should be one line below after :# Ack > > > > + #: > > + RTE_MBUF_F_TX_TUNNEL_MPLSINUDP =3D 5 << 45 > > + #: > > + RTE_MBUF_F_TX_TUNNEL_VXLAN_GPE =3D 6 << 45 > > + #: > > + RTE_MBUF_F_TX_TUNNEL_GTP =3D 7 << 45 > > + #: > > + RTE_MBUF_F_TX_TUNNEL_ESP =3D 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 =3D=3D > 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 =3D=3D 3, even identity returns True and one fla= g > 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: Right, I recognized this was also weird and I questioned it when I was writing the values, but I guess my assumption was that if DPDK was using mixtures of these values which they are getting just through bit masks, it would only make sense if they were mutually exclusive (or at least that certain combinations aren't possible that would cause these problems). I can do some more digging on this though to try and see where they are used and if that is the case. They don't seem like mutually exclusive values to me, so there must be some way they are being very careful about it. > 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. I agree that it is pretty outlandish to combine these values like this, but they must have a clever way of handling it. > > > > + #: TCP cksum of TX pkt. Computed by NIC. > > + RTE_MBUF_F_TX_TCP_CKSUM =3D 1 << 52 > > + #: SCTP cksum of TX pkt. Computed by NIC. > > + RTE_MBUF_F_TX_SCTP_CKSUM =3D 2 << 52 > > + #: UDP cksum of TX pkt. Computed by NIC. > > + RTE_MBUF_F_TX_UDP_CKSUM =3D 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 =3D 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. Yes, I was trying to just avoid the exception being thrown. > > 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. This is a good point. Realistically if it is ever false that would mean we have a gap in implementation. I like the idea of flagging a loud warning over throwing an exception in this case though since if we threw an exception that would stop all test cases that use OL flags to stop working even if they don't require the new flag. That would definitely get the problem fixed sooner, but would also shutdown automated testing until it then. > > > + flag |=3D cls[name] > > + return flag > > + > > + @classmethod > > + def make_parser(cls) -> ParserFn: > > + """Makes a parser function. > > + > > + Returns: > > + ParserFn: A dictionary for the `dataclasses.field` metadat= a argument containing a > > + parser function that makes an instance of this flag fr= om text. > > + """ > > + return TextParser.wrap( > > + TextParser.wrap(TextParser.find(r"ol_flags: ([^\n]+)"), st= r.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. I like this idea a lot. Basically all of these flags that are used in parsers are going to need something like that which is going to be basically the same so just implementing it one time would be great. I'm not sure if it fits the scope of this series though, do you think I should write it and add it here or in a separate patch? > > > > @@ -656,6 +1147,9 @@ def stop(self, verify: bool =3D True) -> None: > > Raises: > > InteractiveCommandExecutionError: If `verify` is :data:`T= rue` 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? Technically yes, but that's just due to the nature of how interactive shells aren't perfect when it comes to asynchronous output. That's why I tried to be sneaky and say that it is the "output gathered from sending the stop command" trying to imply that it is not just the output of the `stop` command, but all the output that is gathered from sending it. I can update this though. > > > > 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 =3D "/[0-9a-fA-F]{4}:[0-9a-fA-F]{2}:[0-9a-= fA-F]{2}.[0-9]{1}/" > > +_REGEX_FOR_COLON_SEP_MAC: str =3D r"(?:[\da-fA-F]{2}:){5}[\da-fA-F]{2}= " > > +_REGEX_FOR_HYPHEN_SEP_MAC: str =3D r"(?:[\da-fA-F]{2}-){5,7}[\da-fA-F]= {2}" > > {5,7} should be just 5 repetitions. When could it be more? I added it for EUI-64 addresses, but maybe this isn't very relevant here since I just read that they are encouraged on non-ethernet devices. I can remove it if it doesn't seem worth it to capture. >