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

["Juraj Linkeš" <juraj.linkes@pantheon.tech>]

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.

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

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

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