@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.rybche...@oktetlabs.ru> >> Sent: Wednesday, July 7, 2021 9:11 PM >> To: Zhang, Qi Z <qi.z.zh...@intel.com>; Zhang, AlvinX >> <alvinx.zh...@intel.com>; ajit.khapa...@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.rybche...@oktetlabs.ru> >>>> Sent: Wednesday, July 7, 2021 5:49 PM >>>> To: Zhang, Qi Z <qi.z.zh...@intel.com>; Zhang, AlvinX >>>> <alvinx.zh...@intel.com>; ajit.khapa...@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.rybche...@oktetlabs.ru> >>>>>> Sent: Tuesday, July 6, 2021 4:05 PM >>>>>> To: Zhang, Qi Z <qi.z.zh...@intel.com>; Zhang, AlvinX >>>>>> <alvinx.zh...@intel.com>; ajit.khapa...@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.zh...@intel.com> >>>>>>>> Sent: Tuesday, July 6, 2021 3:06 PM >>>>>>>> To: Andrew Rybchenko <andrew.rybche...@oktetlabs.ru>; Zhang, Qi Z >>>>>>>> <qi.z.zh...@intel.com>; ajit.khapa...@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. >>>>> >>> >