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

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

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.

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

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

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