Re: [PATCH v3 11/12] dts: add Rx offload capabilities

[Jeremy Spewock <jspewock@iol.unh.edu>]

On Wed, Sep 18, 2024 at 10:18 AM Juraj Linkeš
<juraj.linkes@pantheon.tech> wrote:
>
>
>
> On 26. 8. 2024 19:24, Jeremy Spewock wrote:
> > On Wed, Aug 21, 2024 at 10:53 AM Juraj Linkeš
> > <juraj.linkes@pantheon.tech> wrote:
> > <snip>
> >> diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py
> >> index 48c31124d1..f83569669e 100644
> >> --- a/dts/framework/remote_session/testpmd_shell.py
> >> +++ b/dts/framework/remote_session/testpmd_shell.py
> >> @@ -659,6 +659,103 @@ class TestPmdPortStats(TextParser):
> >>       tx_bps: int = field(metadata=TextParser.find_int(r"Tx-bps:\s+(\d+)"))
> >>
> >>
> >> +class RxOffloadCapability(Flag):
> >> +    """Rx offload capabilities of a device."""
> >> +
> >> +    #:
> >> +    RX_OFFLOAD_VLAN_STRIP = auto()
> >> +    #: Device supports L3 checksum offload.
> >> +    RX_OFFLOAD_IPV4_CKSUM = auto()
> >> +    #: Device supports L4 checksum offload.
> >> +    RX_OFFLOAD_UDP_CKSUM = auto()
> >> +    #: Device supports L4 checksum offload.
> >> +    RX_OFFLOAD_TCP_CKSUM = auto()
> >> +    #: Device supports Large Receive Offload.
> >> +    RX_OFFLOAD_TCP_LRO = auto()
> >> +    #: Device supports QinQ (queue in queue) offload.
> >> +    RX_OFFLOAD_QINQ_STRIP = auto()
> >> +    #: Device supports inner packet L3 checksum.
> >> +    RX_OFFLOAD_OUTER_IPV4_CKSUM = auto()
> >> +    #: Device supports MACsec.
> >> +    RX_OFFLOAD_MACSEC_STRIP = auto()
> >> +    #: Device supports filtering of a VLAN Tag identifier.
> >> +    RX_OFFLOAD_VLAN_FILTER = 1 << 9
> >> +    #: Device supports VLAN offload.
> >> +    RX_OFFLOAD_VLAN_EXTEND = auto()
> >> +    #: Device supports receiving segmented mbufs.
> >> +    RX_OFFLOAD_SCATTER = 1 << 13
> >
> > I know you mentioned in the commit message that the auto() can cause
> > problems with mypy/sphinx, is that why this one is a specific value
> > instead? Regardless, I think we should probably make it consistent so
> > that either all of them are bit-shifts or none of them are unless
> > there is a specific reason that the scatter offload is different.
> >
>
> Since both you and Dean asked, I'll add something to the docstring about
> this.
>
> There are actually two non-auto values (RX_OFFLOAD_VLAN_FILTER = 1 << 9
> is the first one). I used the actual values to mirror the flags in DPDK
> code.

Gotcha, that makes sense.

>
> >> +    #: Device supports Timestamp.
> >> +    RX_OFFLOAD_TIMESTAMP = auto()
> >> +    #: Device supports crypto processing while packet is received in NIC.
> >> +    RX_OFFLOAD_SECURITY = auto()
> >> +    #: Device supports CRC stripping.
> >> +    RX_OFFLOAD_KEEP_CRC = auto()
> >> +    #: Device supports L4 checksum offload.
> >> +    RX_OFFLOAD_SCTP_CKSUM = auto()
> >> +    #: Device supports inner packet L4 checksum.
> >> +    RX_OFFLOAD_OUTER_UDP_CKSUM = auto()
> >> +    #: Device supports RSS hashing.
> >> +    RX_OFFLOAD_RSS_HASH = auto()
> >> +    #: Device supports
> >> +    RX_OFFLOAD_BUFFER_SPLIT = auto()
> >> +    #: Device supports all checksum capabilities.
> >> +    RX_OFFLOAD_CHECKSUM = RX_OFFLOAD_IPV4_CKSUM | RX_OFFLOAD_UDP_CKSUM | RX_OFFLOAD_TCP_CKSUM
> >> +    #: Device supports all VLAN capabilities.
> >> +    RX_OFFLOAD_VLAN = (
> >> +        RX_OFFLOAD_VLAN_STRIP
> >> +        | RX_OFFLOAD_VLAN_FILTER
> >> +        | RX_OFFLOAD_VLAN_EXTEND
> >> +        | RX_OFFLOAD_QINQ_STRIP
> >> +    )
> > <snip>
> >>
> >> @@ -1048,6 +1145,42 @@ def _close(self) -> None:
> >>       ====== Capability retrieval methods ======
> >>       """
> >>
> >> +    def get_capabilities_rx_offload(
> >> +        self,
> >> +        supported_capabilities: MutableSet["NicCapability"],
> >> +        unsupported_capabilities: MutableSet["NicCapability"],
> >> +    ) -> None:
> >> +        """Get all rx offload capabilities and divide them into supported and unsupported.
> >> +
> >> +        Args:
> >> +            supported_capabilities: Supported capabilities will be added to this set.
> >> +            unsupported_capabilities: Unsupported capabilities will be added to this set.
> >> +        """
> >> +        self._logger.debug("Getting rx offload capabilities.")
> >> +        command = f"show port {self.ports[0].id} rx_offload capabilities"
> >
> > Is it desirable to only get the capabilities of the first port? In the
> > current framework I suppose it doesn't matter all that much since you
> > can only use the first few ports in the list of ports anyway, but will
> > there ever be a case where a test run has 2 different devices included
> > in the list of ports? Of course it's possible that it will happen, but
> > is it practical? Because, if so, then we would want this to aggregate
> > what all the devices are capable of and have capabilities basically
> > say "at least one of the ports in the list of ports is capable of
> > these things."
> >
> > This consideration also applies to the rxq info capability gathering as well.
> >
>
> No parts of the framework are adjusted to use multiple NIC in a single
> test run (because we assume we're testing only one NIC at a time). If we
> add this support, it's going to be a broader change.
>
> I approached this with the above assumption in mind and in that case,
> testing just one port of the NIC seemed just fine.

That's a good point that making the adjustment to allow for multiple
devices is a bigger change that is definitely out of scope for this
series. Makes sense to put it off and go with the current assumptions,
I only asked in case it was something simple so it would be one less
thing to do in the future :). This is fine as is then I think.

>
> >> +        rx_offload_capabilities_out = self.send_command(command)
> >> +        rx_offload_capabilities = RxOffloadCapabilities.parse(rx_offload_capabilities_out)
> >> +        self._update_capabilities_from_flag(
> >> +            supported_capabilities,
> >> +            unsupported_capabilities,
> >> +            RxOffloadCapability,
> >> +            rx_offload_capabilities.per_port | rx_offload_capabilities.per_queue,
> >> +        )
> >> +
> > <snip>
> >>
> >>       def __call__(
> >>           self,
> >> diff --git a/dts/tests/TestSuite_pmd_buffer_scatter.py b/dts/tests/TestSuite_pmd_buffer_scatter.py
> >> index 89ece2ef56..64c48b0793 100644
> >> --- a/dts/tests/TestSuite_pmd_buffer_scatter.py
> >> +++ b/dts/tests/TestSuite_pmd_buffer_scatter.py
> >> @@ -28,6 +28,7 @@
> >>   from framework.testbed_model.capability import NicCapability, requires
> >>
> >>
> >> +@requires(NicCapability.RX_OFFLOAD_SCATTER)
> >
> > I know that we talked about this and how, in the environments we
> > looked at, it was true that the offload was supported in all cases
> > where the "native" or non-offloaded was supported, but thinking about
> > this more, I wonder if it is worth generalizing this assumption to all
> > NICs or if we can just decorate the second test case that I wrote
> > which uses the offloaded support. As long as the capabilities exposed
> > by testpmd are accurate, even if this assumption was true, the
> > capability for the non-offloaded one would show False when this
> > offload wasn't usable and it would skip the test case anyway, so I
> > don't think we lose anything by not including this test-suite-level
> > requirement and making it more narrow to the test cases that require
> > it.
> >
> > Let me know your thoughts on that though and I would be interested to
> > hear if anyone else has any.
> >
>
> I'm not sure I understand what your point is. Let's talk about it in the
> call.

Sure, sounds good to me.


>
> >>   class TestPmdBufferScatter(TestSuite):
> >>       """DPDK PMD packet scattering test suite.
> >>
> >> --
> >> 2.34.1
> >>
>