On Mon, Sep 9, 2024 at 7:44 AM Juraj Linkeš <juraj.lin...@pantheon.tech> 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=_find)
> >
> > + @staticmethod
> > + def find_all(
> > + pattern: str | re.Pattern[str],
> > + flags: re.RegexFlag = 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
> > expression matches in the text.
> > +
> > + If there are no matches found in the text than None will be
> > returned, 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/framework/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, LogicalCoreList
> > 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 = field(metadata=TextParser.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 MBUF
> > 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 = 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 = 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 = 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 |= 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,
> > + )
> > +
> > +
> > +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 = cls(0)
> > + for name in arr:
> > + if name in cls.__members__:
> > + flag |= 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 software
> > 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` metadata
> > argument containing a
> > + parser function that makes an instance of this flag from
> > text.
> > + """
> > + return TextParser.wrap(
> > + TextParser.wrap(TextParser.find(f"{'hw' if hw else 'sw'}
> > ptype: ([^-]+)"), str.split),
> > + cls.from_str_list,
> > + )
> > +
> > +
> > +@dataclass
> > +class TestPmdVerbosePacket(TextParser):
>
> > + ol_flags: OLFlag = field(metadata=OLFlag.make_parser())
> > + #: RSS has of the packet in hex.
>
> typo: hash
Ack.
>
>
> > class TestPmdShell(DPDKShell):
>
> > + @staticmethod
> > + def extract_verbose_output(output: str) -> list[TestPmdVerbosePacket]:
> > + """Extract the verbose information present in given testpmd output.
> > +
> > + This method extracts sections of verbose output that begin with
> > 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
> > information in `output`.
> > + """
> > + out: list[TestPmdVerbosePacket] = []
> > + prev_header: str = ""
> > + iter = re.finditer(
> > + r"(?P<HEADER>(?: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 =
> > "/[0-9a-fA-F]{4}:[0-9a-fA-F]{2}:[0-9a-fA-F]{2}.[0-9]{1}/"
> > +REGEX_FOR_MAC_ADDRESS: str = 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.
>
>
