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 83315459BE; Tue, 17 Sep 2024 15:40:39 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 0BD7440E0B; Tue, 17 Sep 2024 15:40:37 +0200 (CEST) Received: from mail-lj1-f173.google.com (mail-lj1-f173.google.com [209.85.208.173]) by mails.dpdk.org (Postfix) with ESMTP id F150E40DDD for ; Tue, 17 Sep 2024 15:40:35 +0200 (CEST) Received: by mail-lj1-f173.google.com with SMTP id 38308e7fff4ca-2f761461150so60679121fa.0 for ; Tue, 17 Sep 2024 06:40:35 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=iol.unh.edu; s=unh-iol; t=1726580435; x=1727185235; 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=g2D0HS8nE7jaHSk9CC3VO8zS2Qh/AiSon1vb2L82Wqk=; b=YDVzZyqQa3GAfdsbrd6R/2S/kcfr6pqj+kdoNpF1836DGttVPoMEaczFHxtKgOuWMa 3Aexgezi+y8mQQhOUAq0WDT1Iu6lB3GZSTCwqtbt3JG1cJiCYltWbO8jX2w67Fms2j6K bgF1bdJgofcmlzNFHkaVGswHj5fpHofLATI8Q= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1726580435; x=1727185235; 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=g2D0HS8nE7jaHSk9CC3VO8zS2Qh/AiSon1vb2L82Wqk=; b=bokJX8vAAmNtjg5tEE5A9NhBwGPuBrVp2bY/mDZT4uH7X4j/vezOm+9gZn2pC3C5xL 4VzuMSPs0Z59CAjWJB5TeyL2d3Uw0nS5pFOI0xUoJKAXETpEtWcGTb7AGlDy4m/M/pRv wzcMHCMLj4i2TGj6cfrbrgfhC0j+ao6qSgLgAeHoMd0gNDrVV9YbELzoFYM0JxfByiGO xteEDRXtEnGRI7HrpzSzk3FJEv6I/CxjlIeu1Tb89WN94st3WLPLn0rpSqc462LlQBf/ OBUyLmtoYey+Lw2hDMFiif8cTqtrBiyTmGLFNyD4yI8rwUcloIJjwOnw78Xhdf05BrYW um3w== X-Forwarded-Encrypted: i=1; AJvYcCVUaDbjgp3k99W4kY100wz4kaHDr0aSWk5+i2nMdZddBcYeCKkB2wf87a4awY2rzFWp1WM=@dpdk.org X-Gm-Message-State: AOJu0Yw0GM2V+0qrWZ2OM3tEKhIq93ciZ1xGuaBUCnTYiqQbLxPyHmoq 4LbAJcbCwdhYQ6r5ceJ+PRjtU2N3GS+taXTqO8yElg9pLjBybyEcArJmXNmbe8RlfOlv0IEoma2 yl0TgRFAnn+FWMNwPsxiY6ZVOsUmjZqAl0MvS2w== X-Google-Smtp-Source: AGHT+IGi+BRGYmbmHlhi4pGGy4frZ0QZ0Bpp1S+kSVWLzqpRxY1JR1cA712ckrou79co5DP0EtT/HHvXCiYo7NJbrpM= X-Received: by 2002:a2e:b892:0:b0:2f6:6198:1cfa with SMTP id 38308e7fff4ca-2f791b667bbmr94602851fa.41.1726580434726; Tue, 17 Sep 2024 06:40:34 -0700 (PDT) MIME-Version: 1.0 References: <20240729203955.267942-1-jspewock@iol.unh.edu> <20240808203612.329540-1-jspewock@iol.unh.edu> <20240808203612.329540-2-jspewock@iol.unh.edu> <3627c429-9a3e-48af-8c42-d85c83ee2998@pantheon.tech> In-Reply-To: <3627c429-9a3e-48af-8c42-d85c83ee2998@pantheon.tech> From: Jeremy Spewock Date: Tue, 17 Sep 2024 09:40:22 -0400 Message-ID: Subject: Re: [PATCH v3 1/1] dts: add text parser for testpmd verbose output To: =?UTF-8?Q?Juraj_Linke=C5=A1?= Cc: thomas@monjalon.net, yoan.picchi@foss.arm.com, paul.szczepanek@arm.com, Honnappa.Nagarahalli@arm.com, probb@iol.unh.edu, wathsala.vithanage@arm.com, Luca.Vizzarro@arm.com, npratte@iol.unh.edu, alex.chapman@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 Mon, Sep 9, 2024 at 7:44=E2=80=AFAM Juraj Linke=C5=A1 wrote: > > > > diff --git a/dts/framework/parser.py b/dts/framework/parser.py > > index 741dfff821..0b39025a48 100644 > > --- a/dts/framework/parser.py > > +++ b/dts/framework/parser.py > > @@ -160,6 +160,36 @@ def _find(text: str) -> Any: > > > > return ParserFn(TextParser_fn=3D_find) > > > > + @staticmethod > > + def find_all( > > + pattern: str | re.Pattern[str], > > + flags: re.RegexFlag =3D re.RegexFlag(0), > > + ) -> ParserFn: > > I'd remove this if it's not used, the rule being let's not introduce > unused code because it's not going to be maintained. We can always add > it when needed. Since submitting this I did actually find one use for it in the Rx/Tx suite, but that one has yet to undergo review, so it could be the case that people don't like that implementation. > > > + """Makes a parser function that finds all of the regular expre= ssion matches in the text. > > + > > + If there are no matches found in the text than None will be re= turned, otherwise a list > > then None, but maybe a comma would be better (found in the text, None > will be returned) Ack. > > > + containing all matches will be returned. Patterns that contain= multiple groups will pack > > + the matches for each group into a tuple. > > + > > > diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framew= ork/remote_session/testpmd_shell.py > > index 43e9f56517..7d0b5a374c 100644 > > --- a/dts/framework/remote_session/testpmd_shell.py > > +++ b/dts/framework/remote_session/testpmd_shell.py > > @@ -31,7 +31,7 @@ > > from framework.settings import SETTINGS > > from framework.testbed_model.cpu import LogicalCoreCount, LogicalCore= List > > from framework.testbed_model.sut_node import SutNode > > -from framework.utils import StrEnum > > +from framework.utils import REGEX_FOR_MAC_ADDRESS, StrEnum > > > > > > class TestPmdDevice: > > @@ -577,6 +577,377 @@ class TestPmdPortStats(TextParser): > > tx_bps: int =3D field(metadata=3DTextParser.find_int(r"Tx-bps:\s+= (\d+)")) > > > > > > +class OLFlag(Flag): > > We should come up with a consistent naming scheme for the various > offloads. In the capabilities patch, I've introduced > RxOffloadCapability. I think we can use the full word Offload and we > should also capture in the name what sort of offload it is. In this > case, would PacketOffloadFlag be a good name? This is a good point, I was just naming according to what they called it in the verbose output, but this probably should be more consistent. I think that PacketOffloadFlag would make sense. > > > + """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. > > I like the reference, let's also mention the name of the file > rte_mbuf_core.h. Maybe we should add more references like these to other > flags. Ack. > > > + """ > > + > > + # RX flags > > + #: > > + RTE_MBUF_F_RX_RSS_HASH =3D auto() > > + > > + #: > > I noticed the flags are not sorted the same way as in rte_mbuf_core.h. I > think there's value in using the same flag values. > > We could also add descriptions to the flag if there are some to be found > in rte_mbuf_core.h. Yeah, I reordered some to try and keep consistent groupings between things like checksums or what I thought made logical sense. I figured that the order wouldn't matter all that much since the verbose output just uses the flag names, but you're right, there could be some value in keeping them the same. Especially if we try to write parsers for the new verbose output modes that are being worked on in testpmd since one of them is just a hexdump. > > > + > > + # TX flags > > + #: > > + RTE_MBUF_F_TX_OUTER_UDP_CKSUM =3D auto() > > Since there is a gap between RX and TX flags, you can just assign the > actual value here (1 << 41) and the continue using auto(). Good point, I can make that change. > > > > + @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 name in cls.__members__: > > We could also do if cls[name] in cls. It's basically the same thing, but > doesn't use the dunder method. Ack. > > > + 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, > > + ) > > + > > + > > +class RtePTypes(Flag): > > + """Flag representing possible packet types in DPDK verbose output.= """ > > Now this docstring doesn't reference from where these come from. > > I found these in rte_mbuf_ptype.h, but from what I can tell, they're not > actual flags, just regular numbers: Right, I did take them from there. Probably good to mention where these come from as well. > #define RTE_PTYPE_L2_ETHER 0x00000001 > #define RTE_PTYPE_L2_ETHER_TIMESYNC 0x00000002 > > etc., so we're basically converting that to flags. I think this is OK > and we don't really need to concern ourselves with the actual values, > just the order. > Ack. > > > + @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. > > ol_flag looks like a copy-paste. Oops, good catch! > > > + > > + Returns: > > + A new instance of the flag. > > + """ > > + flag =3D cls(0) > > + for name in arr: > > + if name in cls.__members__: > > + flag |=3D cls[name] > > + return flag > > + > > + @classmethod > > + def make_parser(cls, hw: bool) -> ParserFn: > > + """Makes a parser function. > > + > > + Args: > > + hw: Whether to make a parser for hardware ptypes or softwa= re ptypes. If :data:`True` > > I think there should be a comma before hardware (on the next line). Ack. > > > + hardware ptypes will be collected, otherwise software = pytpes will. > > + > > + 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(f"{'hw' if hw else 'sw'} p= type: ([^-]+)"), str.split), > > + cls.from_str_list, > > + ) > > + > > + > > +@dataclass > > +class TestPmdVerbosePacket(TextParser): > > > + ol_flags: OLFlag =3D field(metadata=3DOLFlag.make_parser()) > > + #: RSS has of the packet in hex. > > typo: hash Ack. > > > > class TestPmdShell(DPDKShell): > > > + @staticmethod > > + def extract_verbose_output(output: str) -> list[TestPmdVerbosePack= et]: > > + """Extract the verbose information present in given testpmd ou= tput. > > + > > + This method extracts sections of verbose output that begin wit= h the line > > + "port X/queue Y: sent/received Z packets" and end with the ol_= flags of a packet. > > + > > + Args: > > + output: Testpmd output that contains verbose information > > + > > + Returns: > > + List of parsed packet information gathered from verbose in= formation in `output`. > > + """ > > + out: list[TestPmdVerbosePacket] =3D [] > > + prev_header: str =3D "" > > + iter =3D re.finditer( > > + r"(?P
(?:port \d+/queue \d+: received \d packets)?)= \s*" > > Looks like sent packets won't be captured by this. Right, I think I wrote this implementation for received first and then made it more dynamic so it could support both but missed this. Good catch! > > > > diff --git a/dts/framework/utils.py b/dts/framework/utils.py > > index 6b5d5a805f..9c64cf497f 100644 > > --- a/dts/framework/utils.py > > +++ b/dts/framework/utils.py > > @@ -27,6 +27,7 @@ > > 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_MAC_ADDRESS: str =3D r"(?:[\da-fA-F]{2}:){5}[\da-fA-F]{2}" > > Is this the only format that testpmd returns? I believe so, but because I'm not completely sure I can change this regex to support other variants as well. The hyphen separated one is easy enough that it might as well be included, the group of 4 separated by a dot might be a little more involved but I can probably get it to work. > >