Re: [dpdk-dev] [PATCH v3] ethdev: add IPv4 and L4 checksum RSS offload types

[Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>]

@Thomas, @Ferruh, @Ori I need your opinion on the discussion.

On 7/8/21 4:07 AM, Zhang, Qi Z wrote:
> 
> 
>> -----Original Message-----
>> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>> Sent: Wednesday, July 7, 2021 9:11 PM
>> To: Zhang, Qi Z <qi.z.zhang@intel.com>; Zhang, AlvinX
>> <alvinx.zhang@intel.com>; ajit.khaparde@broadcom.com
>> Cc: dev@dpdk.org
>> Subject: Re: [PATCH v3] ethdev: add IPv4 and L4 checksum RSS offload types
>>
>> On 7/7/21 4:00 PM, Zhang, Qi Z wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>>> Sent: Wednesday, July 7, 2021 5:49 PM
>>>> To: Zhang, Qi Z <qi.z.zhang@intel.com>; Zhang, AlvinX
>>>> <alvinx.zhang@intel.com>; ajit.khaparde@broadcom.com
>>>> Cc: dev@dpdk.org
>>>> Subject: Re: [PATCH v3] ethdev: add IPv4 and L4 checksum RSS offload
>>>> types
>>>>
>>>> On 7/7/21 6:23 AM, Zhang, Qi Z wrote:
>>>>>
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>>>>> Sent: Tuesday, July 6, 2021 4:05 PM
>>>>>> To: Zhang, Qi Z <qi.z.zhang@intel.com>; Zhang, AlvinX
>>>>>> <alvinx.zhang@intel.com>; ajit.khaparde@broadcom.com
>>>>>> Cc: dev@dpdk.org
>>>>>> Subject: Re: [PATCH v3] ethdev: add IPv4 and L4 checksum RSS
>>>>>> offload types
>>>>>>
>>>>>> On 7/6/21 10:18 AM, Zhang, Qi Z wrote:
>>>>>>>
>>>>>>>
>>>>>>>> -----Original Message-----
>>>>>>>> From: Zhang, AlvinX <alvinx.zhang@intel.com>
>>>>>>>> Sent: Tuesday, July 6, 2021 3:06 PM
>>>>>>>> To: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>; Zhang, Qi Z
>>>>>>>> <qi.z.zhang@intel.com>; ajit.khaparde@broadcom.com
>>>>>>>> Cc: dev@dpdk.org
>>>>>>>> Subject: RE: [PATCH v3] ethdev: add IPv4 and L4 checksum RSS
>>>>>>>> offload types
>>>>>>>>
>>>>>>>>>> @@ -537,6 +537,8 @@ struct rte_eth_rss_conf {
>>>>>>>>>>  #define ETH_RSS_PPPOE		   (1ULL << 31)
>>>>>>>>>>  #define ETH_RSS_ECPRI		   (1ULL << 32)
>>>>>>>>>>  #define ETH_RSS_MPLS		   (1ULL << 33)
>>>>>>>>>> +#define ETH_RSS_IPV4_CHKSUM	   (1ULL << 34)
>>>>>>>>>> +#define ETH_RSS_L4_CHKSUM	   (1ULL << 35)
>>>>>>>>>
>>>>>>>>> What does efine which L4 protocols are supported? How user will
>> know?
>>>>>>>>
>>>>>>>> I think if we want to support L4 checksum RSS by using below
>>>>>>>> command port config all rss (all|default|eth|vlan|...)
>>>>>>>>
>>>>>>>> We must define TCP/UDP/SCTP checksum RSS separately:
>>>>>>>> #define ETH_RSS_TCP_CHKSUM	(1ULL << 35)
>>>>>>>> #define ETH_RSS_UDP_CHKSUM	(1ULL << 36)
>>>>>>>> #deifne ETH_RSS_SCTP_CHKSUM	(1ULL << 37)
>>>>>>>>
>>>>>>>> Here 3 bits are occupied, this is not good for there are not many
>>>>>>>> bits
>>>>>> available.
>>>>>>>>
>>>>>>>> If we only want to using it in flows, we only need to define
>>>>>>>> ETH_RSS_L4_CHKSUM, because the flow pattern pointed out the L4
>>>>>>>> protocol type.
>>>>>>>> flow create 0 ingress pattern eth / ipv4 / tcp / end actions rss
>>>>>>>> types l4-chksum end queues end / end
>>>>>>>
>>>>>>> +1, the pattern already give the hint to avoid the ambiguity and I
>>>>>>> +think we
>>>>>> already have ETH_RSS_LEVEL to figure out inner or outer.
>>>>>>
>>>>>> The problem that it may be used in generic RSS flags which has no
>>>>>> the
>>>> context.
>>>>>> Also even in the case of flow API context could have no L4 protocol at all.
>>>>>
>>>>> For generic case, it can simply assume it cover all L4 checksum
>>>>> cases and I'm
>>>> not sure if any user intend to use it as generic RSS, pmd can simply
>>>> reject it if it's not necessary to support.
>>>>
>>>> Try to look at it from an application point of view which does not
>>>> know any specifics of the driver.
>>>>
>>>>  * Get dev_info and see ETH_RSS_L4_CHKSUM, good!, would like to
>>>>    use it.
>>>
>>>
>>> The PMD should not expose it if it don't want to (or not able to)
>>> support all l4 checksum from generic RSS configure
>>
>> Document what is "all L4".
>>
>>>
>>> And we should assume this is only apply for generic RSS configure but not for
>> flow API.
>>
>> I don't think so. IMHO, it should report all RSS capabilities regardless generic vs
>> flow API RSS action.
> 
> 
> The RSS action in flow API could cover lots of possibility.
> for example an ETH_RSS_IPV4 can be applied on a GTPU flow for inner but may not work for a VxLan flow's inner l3 at the same time.
> it's difficult to accurately describe all of these by a 64 bits capability, it's more practice to just rely on rte_flow_validation.
> Otherwise it will always leading the confusing you mentioned in previous mail.
> 
> It is more reasonable for me, the driver just expose some basic RSS bit that everybody can easiely understand,(e.g.: 5 tuple.), and left all the complexity capability probe to flow API.

May be it is OK to report subset in
dev_info->flow_type_rss_offloads, but I'm very
uncomfortable with the approach. Superset sounds
more logical to me, but has drawbacks as well.

> 
>>
>> It is just RSS capabilities reporting w/o any context.
> 
> 
> 
> 
>>
>>>
>>> Because the rte_flow_validate is the recommended method to check if a RSS
>> action is supported in flow API or not.
>>
>> It could restrict the subset. But superset should be reported in caps.
>>
>>>
>>>>
>>>>  * If I try to use it in default RSS config, but the request
>>>>    fail, it could be very confusing.
>>>>
>>>>  * Will it distribute TCP packets? UDP packets? SCTP packets?
>>>>    Or should I care about RSS for some of them based on other
>>>>    supported fields? E.g. if SCTP is not supported by the NIC,
>>>>    I need to install RSS flow rule for the IP protocol to do
>>>>    RSS based on IPv4/IPv6 addresses. But if SCTP is supported,
>>>>    I'm happy to use ETH_RSS_L4_CHKSUM for it as well.
>>>>
>>>>> In flow API, if no l4 protocol in pattern , the PMD should return
>>>>> failure (or maybe some default behavior), and I think this is not a
>>>>> new question as it happens all the cases
>>>>> e.g.:
>>>>> pattern eth / vlan / end action rss type ipv4 .
>>>>
>>>> IMHO, it would be pretty logical to apply RSS to IPv4 packets only
>>>> and send everything else to default queue.
>>>
>>> Yes, this also make sense to me, but I think PMD's flow parser still can have
>> more strict check, as it does not drop any feature that the NIC can support.
>>>
>>>>
>>>>>>
>>>>>> Is UDP checksum 0 treated as no checksum and go to default queue or
>>>>>> treated as a regular checksum with value equal to 0?
>>>>>
>>>>> I think we can treat it as value 0, as least our hardware behavior
>>>>> like this, is
>>>> this any issue?
>>>>
>>>> OK, no problem. Just document it.
>>>>
>>>>>>
>>>>>> I tend to agree that 3 flags is too much for the feature, but one
>>>>>> flag without properly defined meaning is not good as well.
>>>>>>
>>>>>> I just want rules to be defined and documented.'
>>>>>
>>>>> Agree, we need more document for this. if you agree above proposal.
>>>>>
>>>
>