From: Thomas Monjalon <thomas@monjalon.net>
To: "Zhang, Qi Z" <qi.z.zhang@intel.com>,
"Zhang, AlvinX" <alvinx.zhang@intel.com>,
Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
Cc: "ajit.khaparde@broadcom.com" <ajit.khaparde@broadcom.com>,
dev@dpdk.org, Ferruh Yigit <ferruh.yigit@intel.com>,
Ori Kam <orika@nvidia.com>,
olivier.matz@6wind.com
Subject: Re: [dpdk-dev] [PATCH v3] ethdev: add IPv4 and L4 checksum RSS offload types
Date: Sat, 10 Jul 2021 10:38:07 +0200 [thread overview]
Message-ID: <18478202.kP3E1pVba9@thomas> (raw)
In-Reply-To: <4c74f418-bbc6-93e3-c555-b513bd01be19@oktetlabs.ru>
08/07/2021 09:45, Andrew Rybchenko:
> @Thomas, @Ferruh, @Ori I need your opinion on the discussion.
>
> On 7/8/21 4:07 AM, Zhang, Qi Z wrote:
> > From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> >>> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> >>>> On 7/7/21 6:23 AM, Zhang, Qi Z wrote:
> >>>>> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> >>>>>> On 7/6/21 10:18 AM, Zhang, Qi Z wrote:
> >>>>>>> From: Zhang, AlvinX <alvinx.zhang@intel.com>
> >>>>>>>>
> >>>>>>>>>> @@ -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
That's restrictive to allow only full-support,
but I'm fine with the trade-off to avoid wasting bits.
> >>
> >> Document what is "all L4".
List of L4 protocols should be explicit.
> >>> 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.
Yes superset should be reported, this is the meaning of capabilities:
the driver is capable but there are some limitations
which cannot be advertised, so rte_flow_validate checks the limitations
in the dynamic context.
> >> 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.
Yes please do not add a new flag in rte_ethdev.h without doc.
next prev parent reply other threads:[~2021-07-10 8:49 UTC|newest]
Thread overview: 39+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-03 3:12 [dpdk-dev] [PATCH] ethdev: add RSS offload type for L3 checksum Alvin Zhang
2021-06-03 7:14 ` Andrew Rybchenko
2021-06-03 7:28 ` Zhang, AlvinX
2021-06-03 8:03 ` [dpdk-dev] [PATCH v2] ethdev: add IPv4 checksum RSS offload type Alvin Zhang
2021-06-03 8:17 ` Andrew Rybchenko
2021-06-04 2:23 ` Zhang, AlvinX
2021-06-07 18:31 ` Ajit Khaparde
2021-06-15 8:19 ` [dpdk-dev] [PATCH v3] ethdev: add IPv4 and L4 checksum RSS offload types Alvin Zhang
2021-06-15 8:26 ` Jerin Jacob
2021-06-16 15:18 ` Zhang, Qi Z
2021-06-22 8:20 ` Singh, Aman Deep
2021-07-01 14:26 ` Andrew Rybchenko
2021-07-06 6:14 ` Zhang, AlvinX
2021-07-06 7:05 ` Zhang, AlvinX
2021-07-06 7:18 ` Zhang, Qi Z
2021-07-06 8:04 ` Andrew Rybchenko
2021-07-07 3:23 ` Zhang, Qi Z
2021-07-07 9:49 ` Andrew Rybchenko
2021-07-07 13:00 ` Zhang, Qi Z
2021-07-07 13:10 ` Andrew Rybchenko
2021-07-08 1:07 ` Zhang, Qi Z
2021-07-08 7:45 ` Andrew Rybchenko
2021-07-10 8:38 ` Thomas Monjalon [this message]
2021-07-13 1:13 ` [dpdk-dev] [PATCH v4] " Alvin Zhang
2021-07-13 7:54 ` Andrew Rybchenko
2021-07-13 9:38 ` Zhang, AlvinX
2021-07-13 10:24 ` Andrew Rybchenko
2021-07-14 2:38 ` Zhang, AlvinX
2021-08-18 2:32 ` [dpdk-dev] [PATCH v5] " Alvin Zhang
2021-08-29 12:07 ` Zhang, Qi Z
2021-08-31 9:44 ` [dpdk-dev] [PATCH v6] " Alvin Zhang
2021-08-31 9:52 ` [dpdk-dev] [PATCH v7] " Alvin Zhang
2021-09-06 0:28 ` Zhang, Qi Z
2021-09-14 14:00 ` Ferruh Yigit
2021-09-15 1:36 ` Zhang, AlvinX
2021-09-15 5:47 ` [dpdk-dev] [PATCH v8] " Alvin Zhang
2021-09-21 8:26 ` Ferruh Yigit
2021-09-28 13:09 ` Ferruh Yigit
2021-09-29 2:27 ` Zhang, AlvinX
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=18478202.kP3E1pVba9@thomas \
--to=thomas@monjalon.net \
--cc=ajit.khaparde@broadcom.com \
--cc=alvinx.zhang@intel.com \
--cc=andrew.rybchenko@oktetlabs.ru \
--cc=dev@dpdk.org \
--cc=ferruh.yigit@intel.com \
--cc=olivier.matz@6wind.com \
--cc=orika@nvidia.com \
--cc=qi.z.zhang@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).