[v3,1/1] dts: add text parser for testpmd verbose output

Message ID 20240808203612.329540-2-jspewock@iol.unh.edu (mailing list archive)
State Superseded
Delegated to: Juraj Linkeš
Headers
Series dts: testpmd verbose parser |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/loongarch-compilation success Compilation OK
ci/loongarch-unit-testing success Unit Testing PASS
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS
ci/github-robot: build success github build: passed
ci/intel-Functional success Functional PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-marvell-Functional success Functional Testing PASS
ci/iol-abi-testing warning Testing issues
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-compile-amd64-testing success Testing PASS
ci/iol-compile-arm64-testing success Testing PASS
ci/iol-unit-arm64-testing success Testing PASS
ci/iol-unit-amd64-testing success Testing PASS
ci/iol-sample-apps-testing success Testing PASS

Commit Message

Jeremy Spewock Aug. 8, 2024, 8:36 p.m. UTC
From: Jeremy Spewock <jspewock@iol.unh.edu>

Multiple test suites from the old DTS framework rely on being able to
consume and interpret the verbose output of testpmd. The new framework
doesn't have an elegant way for handling the verbose output, but test
suites are starting to be written that rely on it. This patch creates a
TextParser class that can be used to extract the verbose information
from any testpmd output and also adjusts the `stop` method of the shell
to return all output that it collected.

Signed-off-by: Jeremy Spewock <jspewock@iol.unh.edu>
---
 dts/framework/parser.py                       |  30 ++
 dts/framework/remote_session/testpmd_shell.py | 405 +++++++++++++++++-
 dts/framework/utils.py                        |   1 +
 3 files changed, 434 insertions(+), 2 deletions(-)
  

Comments

Jeremy Spewock Aug. 8, 2024, 9:49 p.m. UTC | #1
On Thu, Aug 8, 2024 at 4:36 PM <jspewock@iol.unh.edu> wrote:
>
> From: Jeremy Spewock <jspewock@iol.unh.edu>
>
> Multiple test suites from the old DTS framework rely on being able to
> consume and interpret the verbose output of testpmd. The new framework
> doesn't have an elegant way for handling the verbose output, but test
> suites are starting to be written that rely on it. This patch creates a
> TextParser class that can be used to extract the verbose information
> from any testpmd output and also adjusts the `stop` method of the shell
> to return all output that it collected.
>
> Signed-off-by: Jeremy Spewock <jspewock@iol.unh.edu>
> ---
>  dts/framework/parser.py                       |  30 ++
>  dts/framework/remote_session/testpmd_shell.py | 405 +++++++++++++++++-
>  dts/framework/utils.py                        |   1 +
>  3 files changed, 434 insertions(+), 2 deletions(-)
>
> 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(

I just realized that I forgot to take this method out since it is no
longer used in this patch now that I removed the idea of a block of
output that contains a burst of packets. The method could still be
useful in the future, but it isn't used anywhere now, so I can remove
it in the next version if no one sees a use for it. I'm also open to
leaving it there for the "just in case" it is needed in the future.
What do you all think?

> +        pattern: str | re.Pattern[str],
> +        flags: re.RegexFlag = re.RegexFlag(0),
> +    ) -> ParserFn:
> +        """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
> +        containing all matches will be returned. Patterns that contain multiple groups will pack
> +        the matches for each group into a tuple.
> +
> +        Args:
> +            pattern: The regular expression pattern.
> +            flags: The regular expression flags. Ignored if the given pattern is already compiled.
> +
> +        Returns:
> +            A :class:`ParserFn` that can be used as metadata for a dataclass field.
> +        """
> +        if isinstance(pattern, str):
> +            pattern = re.compile(pattern, flags)
> +
> +        def _find_all(text: str) -> list[str] | None:
> +            m = pattern.findall(text)
> +            if len(m) == 0:
> +                return None
> +
> +            return m
> +
> +        return ParserFn(TextParser_fn=_find_all)
> +
<snip>
> 2.45.2
>
  
Nicholas Pratte Aug. 12, 2024, 5:32 p.m. UTC | #2
Great work!

I think it'd be fine to just keep if you ask me, I could see this
being used in the future. I'm also looking at it from the perspective
of 'what if i would have to write this myself,' if it turns out that
we need it again for something later. It's easier to remove later if
it turns out we aren't using it, but it'd likely be more
time-consuming to remove it now and implement it again later,
considering that time has already been spent testing and building it.

On Thu, Aug 8, 2024 at 5:49 PM Jeremy Spewock <jspewock@iol.unh.edu> wrote:
>
> On Thu, Aug 8, 2024 at 4:36 PM <jspewock@iol.unh.edu> wrote:
> >
> > From: Jeremy Spewock <jspewock@iol.unh.edu>
> >
> > Multiple test suites from the old DTS framework rely on being able to
> > consume and interpret the verbose output of testpmd. The new framework
> > doesn't have an elegant way for handling the verbose output, but test
> > suites are starting to be written that rely on it. This patch creates a
> > TextParser class that can be used to extract the verbose information
> > from any testpmd output and also adjusts the `stop` method of the shell
> > to return all output that it collected.
> >
> > Signed-off-by: Jeremy Spewock <jspewock@iol.unh.edu>
> > ---
> >  dts/framework/parser.py                       |  30 ++
> >  dts/framework/remote_session/testpmd_shell.py | 405 +++++++++++++++++-
> >  dts/framework/utils.py                        |   1 +
> >  3 files changed, 434 insertions(+), 2 deletions(-)
> >
> > 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(
>
> I just realized that I forgot to take this method out since it is no
> longer used in this patch now that I removed the idea of a block of
> output that contains a burst of packets. The method could still be
> useful in the future, but it isn't used anywhere now, so I can remove
> it in the next version if no one sees a use for it. I'm also open to
> leaving it there for the "just in case" it is needed in the future.
> What do you all think?
>
> > +        pattern: str | re.Pattern[str],
> > +        flags: re.RegexFlag = re.RegexFlag(0),
> > +    ) -> ParserFn:
> > +        """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
> > +        containing all matches will be returned. Patterns that contain multiple groups will pack
> > +        the matches for each group into a tuple.
> > +
> > +        Args:
> > +            pattern: The regular expression pattern.
> > +            flags: The regular expression flags. Ignored if the given pattern is already compiled.
> > +
> > +        Returns:
> > +            A :class:`ParserFn` that can be used as metadata for a dataclass field.
> > +        """
> > +        if isinstance(pattern, str):
> > +            pattern = re.compile(pattern, flags)
> > +
> > +        def _find_all(text: str) -> list[str] | None:
> > +            m = pattern.findall(text)
> > +            if len(m) == 0:
> > +                return None
> > +
> > +            return m
> > +
> > +        return ParserFn(TextParser_fn=_find_all)
> > +
> <snip>
> > 2.45.2
> >
  
Juraj Linkeš Sept. 9, 2024, 11:44 a.m. UTC | #3
> 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.

> +        """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)

> +        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?

> +    """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.

> +    """
> +
> +    # 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.

> +
> +    # 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().


> +    @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.

> +                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:
#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.


> +    @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.

> +
> +        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).

> +                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


>   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.


> 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?
  
Jeremy Spewock Sept. 17, 2024, 1:40 p.m. UTC | #4
On Mon, Sep 9, 2024 at 7:44 AM Juraj Linkeš <juraj.linkes@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.

>
>
  
Juraj Linkeš Sept. 18, 2024, 8:09 a.m. UTC | #5
On 17. 9. 2024 15:40, Jeremy Spewock wrote:
> On Mon, Sep 9, 2024 at 7:44 AM Juraj Linkeš <juraj.linkes@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.
> 

Ok, we can (and probably should) add it in that test suite patchset if 
needed. The required infrastructure is in this patchset and additions to 
it can be added in individual test suites (if only that one uses that 
addition).

>>> 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.
> 

Ok, might as well be safe, doesn't sound like a big change.

A small point is to make the regex as readable as possible - we could 
split it into multiple regexes (and try to match multiple times) or put 
the one big regex string on mutliple lines if possible (with each line 
being a separate variant or multiple closely related variants).
  

Patch

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:
+        """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
+        containing all matches will be returned. Patterns that contain multiple groups will pack
+        the matches for each group into a tuple.
+
+        Args:
+            pattern: The regular expression pattern.
+            flags: The regular expression flags. Ignored if the given pattern is already compiled.
+
+        Returns:
+            A :class:`ParserFn` that can be used as metadata for a dataclass field.
+        """
+        if isinstance(pattern, str):
+            pattern = re.compile(pattern, flags)
+
+        def _find_all(text: str) -> list[str] | None:
+            m = pattern.findall(text)
+            if len(m) == 0:
+                return None
+
+            return m
+
+        return ParserFn(TextParser_fn=_find_all)
+
     @staticmethod
     def find_int(
         pattern: str | re.Pattern[str],
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):
+    """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.
+    """
+
+    # RX flags
+    #:
+    RTE_MBUF_F_RX_RSS_HASH = auto()
+
+    #:
+    RTE_MBUF_F_RX_L4_CKSUM_GOOD = auto()
+    #:
+    RTE_MBUF_F_RX_L4_CKSUM_BAD = auto()
+    #:
+    RTE_MBUF_F_RX_L4_CKSUM_UNKNOWN = auto()
+    #:
+    RTE_MBUF_F_RX_L4_CKSUM_NONE = auto()
+
+    #:
+    RTE_MBUF_F_RX_IP_CKSUM_GOOD = auto()
+    #:
+    RTE_MBUF_F_RX_IP_CKSUM_BAD = auto()
+    #:
+    RTE_MBUF_F_RX_IP_CKSUM_UNKNOWN = auto()
+    #:
+    RTE_MBUF_F_RX_IP_CKSUM_NONE = auto()
+
+    #:
+    RTE_MBUF_F_RX_OUTER_L4_CKSUM_GOOD = auto()
+    #:
+    RTE_MBUF_F_RX_OUTER_L4_CKSUM_BAD = auto()
+    #:
+    RTE_MBUF_F_RX_OUTER_L4_CKSUM_UNKNOWN = auto()
+    #:
+    RTE_MBUF_F_RX_OUTER_L4_CKSUM_INVALID = auto()
+
+    #:
+    RTE_MBUF_F_RX_VLAN = auto()
+    #:
+    RTE_MBUF_F_RX_FDIR = auto()
+    #:
+    RTE_MBUF_F_RX_OUTER_IP_CKSUM_BAD = auto()
+    #:
+    RTE_MBUF_F_RX_VLAN_STRIPPED = auto()
+    #: RX IEEE1588 L2 Ethernet PT Packet.
+    RTE_MBUF_F_RX_IEEE1588_PTP = auto()
+    #: RX IEEE1588 L2/L4 timestamped packet.
+    RTE_MBUF_F_RX_IEEE1588_TMST = auto()
+    #: FD id reported if FDIR match.
+    RTE_MBUF_F_RX_FDIR_ID = auto()
+    #: Flexible bytes reported if FDIR match.
+    RTE_MBUF_F_RX_FDIR_FLX = auto()
+    #:
+    RTE_MBUF_F_RX_QINQ_STRIPPED = auto()
+    #:
+    RTE_MBUF_F_RX_LRO = auto()
+    #:
+    RTE_MBUF_F_RX_SEC_OFFLOAD_FAILED = auto()
+    #:
+    RTE_MBUF_F_RX_QINQ = auto()
+
+    # TX flags
+    #:
+    RTE_MBUF_F_TX_OUTER_UDP_CKSUM = auto()
+    #:
+    RTE_MBUF_F_TX_UDP_SEG = auto()
+    #:
+    RTE_MBUF_F_TX_SEC_OFFLOAD = auto()
+    #:
+    RTE_MBUF_F_TX_MACSEC = auto()
+
+    #:
+    RTE_MBUF_F_TX_TUNNEL_VXLAN = auto()
+    #:
+    RTE_MBUF_F_TX_TUNNEL_GRE = auto()
+    #:
+    RTE_MBUF_F_TX_TUNNEL_IPIP = auto()
+    #:
+    RTE_MBUF_F_TX_TUNNEL_GENEVE = auto()
+    #:
+    RTE_MBUF_F_TX_TUNNEL_MPLSINUDP = auto()
+    #:
+    RTE_MBUF_F_TX_TUNNEL_VXLAN_GPE = auto()
+    #:
+    RTE_MBUF_F_TX_TUNNEL_GTP = auto()
+    #:
+    RTE_MBUF_F_TX_TUNNEL_ESP = auto()
+    #:
+    RTE_MBUF_F_TX_TUNNEL_IP = auto()
+    #:
+    RTE_MBUF_F_TX_TUNNEL_UDP = auto()
+
+    #:
+    RTE_MBUF_F_TX_QINQ = auto()
+    #:
+    RTE_MBUF_F_TX_TCP_SEG = auto()
+    #: TX IEEE1588 packet to timestamp.
+    RTE_MBUF_F_TX_IEEE1588_TMST = auto()
+
+    #:
+    RTE_MBUF_F_TX_L4_NO_CKSUM = auto()
+    #:
+    RTE_MBUF_F_TX_TCP_CKSUM = auto()
+    #:
+    RTE_MBUF_F_TX_SCTP_CKSUM = auto()
+    #:
+    RTE_MBUF_F_TX_UDP_CKSUM = auto()
+    #:
+    RTE_MBUF_F_TX_L4_MASK = auto()
+    #:
+    RTE_MBUF_F_TX_IP_CKSUM = auto()
+    #:
+    RTE_MBUF_F_TX_OUTER_IP_CKSUM = auto()
+
+    #:
+    RTE_MBUF_F_TX_IPV4 = auto()
+    #:
+    RTE_MBUF_F_TX_IPV6 = auto()
+    #:
+    RTE_MBUF_F_TX_VLAN = auto()
+    #:
+    RTE_MBUF_F_TX_OUTER_IPV4 = auto()
+    #:
+    RTE_MBUF_F_TX_OUTER_IPV6 = auto()
+
+    @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__:
+                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."""
+
+    # L2
+    #:
+    L2_ETHER = auto()
+    #:
+    L2_ETHER_TIMESYNC = auto()
+    #:
+    L2_ETHER_ARP = auto()
+    #:
+    L2_ETHER_LLDP = auto()
+    #:
+    L2_ETHER_NSH = auto()
+    #:
+    L2_ETHER_VLAN = auto()
+    #:
+    L2_ETHER_QINQ = auto()
+    #:
+    L2_ETHER_PPPOE = auto()
+    #:
+    L2_ETHER_FCOE = auto()
+    #:
+    L2_ETHER_MPLS = auto()
+    #:
+    L2_UNKNOWN = auto()
+
+    # L3
+    #:
+    L3_IPV4 = auto()
+    #:
+    L3_IPV4_EXT = auto()
+    #:
+    L3_IPV6 = auto()
+    #:
+    L3_IPV4_EXT_UNKNOWN = auto()
+    #:
+    L3_IPV6_EXT = auto()
+    #:
+    L3_IPV6_EXT_UNKNOWN = auto()
+    #:
+    L3_UNKNOWN = auto()
+
+    # L4
+    #:
+    L4_TCP = auto()
+    #:
+    L4_UDP = auto()
+    #:
+    L4_FRAG = auto()
+    #:
+    L4_SCTP = auto()
+    #:
+    L4_ICMP = auto()
+    #:
+    L4_NONFRAG = auto()
+    #:
+    L4_IGMP = auto()
+    #:
+    L4_UNKNOWN = auto()
+
+    # Tunnel
+    #:
+    TUNNEL_IP = auto()
+    #:
+    TUNNEL_GRE = auto()
+    #:
+    TUNNEL_VXLAN = auto()
+    #:
+    TUNNEL_NVGRE = auto()
+    #:
+    TUNNEL_GENEVE = auto()
+    #:
+    TUNNEL_GRENAT = auto()
+    #:
+    TUNNEL_GTPC = auto()
+    #:
+    TUNNEL_GTPU = auto()
+    #:
+    TUNNEL_ESP = auto()
+    #:
+    TUNNEL_L2TP = auto()
+    #:
+    TUNNEL_VXLAN_GPE = auto()
+    #:
+    TUNNEL_MPLS_IN_UDP = auto()
+    #:
+    TUNNEL_MPLS_IN_GRE = auto()
+    #:
+    TUNNEL_UNKNOWN = auto()
+
+    # Inner L2
+    #:
+    INNER_L2_ETHER = auto()
+    #:
+    INNER_L2_ETHER_VLAN = auto()
+    #:
+    INNER_L2_ETHER_QINQ = auto()
+    #:
+    INNER_L2_UNKNOWN = auto()
+
+    # Inner L3
+    #:
+    INNER_L3_IPV4 = auto()
+    #:
+    INNER_L3_IPV4_EXT = auto()
+    #:
+    INNER_L3_IPV6 = auto()
+    #:
+    INNER_L3_IPV4_EXT_UNKNOWN = auto()
+    #:
+    INNER_L3_IPV6_EXT = auto()
+    #:
+    INNER_L3_IPV6_EXT_UNKNOWN = auto()
+    #:
+    INNER_L3_UNKNOWN = auto()
+
+    # Inner L4
+    #:
+    INNER_L4_TCP = auto()
+    #:
+    INNER_L4_UDP = auto()
+    #:
+    INNER_L4_FRAG = auto()
+    #:
+    INNER_L4_SCTP = auto()
+    #:
+    INNER_L4_ICMP = auto()
+    #:
+    INNER_L4_NONFRAG = auto()
+    #:
+    INNER_L4_UNKNOWN = auto()
+
+    @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__:
+                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`
+                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):
+    """Packet information provided by verbose output in Testpmd.
+
+    This dataclass expects that packet information be prepended with the starting line of packet
+    bursts. Specifically, the line that reads "port X/queue Y: sent/received Z packets".
+    """
+
+    #: ID of the port that handled the packet.
+    port_id: int = field(metadata=TextParser.find_int(r"port (\d+)/queue \d+"))
+    #: ID of the queue that handled the packet.
+    queue_id: int = field(metadata=TextParser.find_int(r"port \d+/queue (\d+)"))
+    #: Whether the packet was received or sent by the queue/port.
+    was_received: bool = field(metadata=TextParser.find(r"received \d+ packets"))
+    #:
+    src_mac: str = field(metadata=TextParser.find(f"src=({REGEX_FOR_MAC_ADDRESS})"))
+    #:
+    dst_mac: str = field(metadata=TextParser.find(f"dst=({REGEX_FOR_MAC_ADDRESS})"))
+    #: Memory pool the packet was handled on.
+    pool: str = field(metadata=TextParser.find(r"pool=(\S+)"))
+    #: Packet type in hex.
+    p_type: int = field(metadata=TextParser.find_int(r"type=(0x[a-fA-F\d]+)"))
+    #:
+    length: int = field(metadata=TextParser.find_int(r"length=(\d+)"))
+    #: Number of segments in the packet.
+    nb_segs: int = field(metadata=TextParser.find_int(r"nb_segs=(\d+)"))
+    #: Hardware packet type.
+    hw_ptype: RtePTypes = field(metadata=RtePTypes.make_parser(hw=True))
+    #: Software packet type.
+    sw_ptype: RtePTypes = field(metadata=RtePTypes.make_parser(hw=False))
+    #:
+    l2_len: int = field(metadata=TextParser.find_int(r"l2_len=(\d+)"))
+    #:
+    ol_flags: OLFlag = field(metadata=OLFlag.make_parser())
+    #: RSS has of the packet in hex.
+    rss_hash: int | None = field(
+        default=None, metadata=TextParser.find_int(r"RSS hash=(0x[a-fA-F\d]+)")
+    )
+    #: RSS queue that handled the packet in hex.
+    rss_queue: int | None = field(
+        default=None, metadata=TextParser.find_int(r"RSS queue=(0x[a-fA-F\d]+)")
+    )
+    #:
+    l3_len: int | None = field(default=None, metadata=TextParser.find_int(r"l3_len=(\d+)"))
+    #:
+    l4_len: int | None = field(default=None, metadata=TextParser.find_int(r"l4_len=(\d+)"))
+
+
 class TestPmdShell(DPDKShell):
     """Testpmd interactive shell.
 
@@ -645,7 +1016,7 @@  def start(self, verify: bool = True) -> None:
                         "Not all ports came up after starting packet forwarding in testpmd."
                     )
 
-    def stop(self, verify: bool = True) -> None:
+    def stop(self, verify: bool = True) -> str:
         """Stop packet forwarding.
 
         Args:
@@ -656,6 +1027,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.
         """
         stop_cmd_output = self.send_command("stop")
         if verify:
@@ -665,6 +1039,7 @@  def stop(self, verify: bool = True) -> None:
             ):
                 self._logger.debug(f"Failed to stop packet forwarding: \n{stop_cmd_output}")
                 raise InteractiveCommandExecutionError("Testpmd failed to stop packet forwarding.")
+        return stop_cmd_output
 
     def get_devices(self) -> list[TestPmdDevice]:
         """Get a list of device names that are known to testpmd.
@@ -806,6 +1181,32 @@  def show_port_stats(self, port_id: int) -> TestPmdPortStats:
 
         return TestPmdPortStats.parse(output)
 
+    @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*"
+            r"(?P<PACKET>src=[\w\s=:-]+?ol_flags: [\w ]+)",
+            output,
+        )
+        for match in iter:
+            if match.group("HEADER"):
+                prev_header = match.group("HEADER")
+            out.append(TestPmdVerbosePacket.parse(f"{prev_header}\n{match.group('PACKET')}"))
+        return out
+
     def _close(self) -> None:
         """Overrides :meth:`~.interactive_shell.close`."""
         self.stop()
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}"
 
 
 def expand_range(range_str: str) -> list[int]: