From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <dev-bounces@dpdk.org>
Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124])
	by inbox.dpdk.org (Postfix) with ESMTP id C656CA0A0C;
	Sat, 10 Jul 2021 10:49:02 +0200 (CEST)
Received: from [217.70.189.124] (localhost [127.0.0.1])
	by mails.dpdk.org (Postfix) with ESMTP id 8820040DDB;
	Sat, 10 Jul 2021 10:49:02 +0200 (CEST)
Received: from out5-smtp.messagingengine.com (out5-smtp.messagingengine.com
 [66.111.4.29]) by mails.dpdk.org (Postfix) with ESMTP id 11081407FF
 for <dev@dpdk.org>; Sat, 10 Jul 2021 10:49:01 +0200 (CEST)
Received: from compute4.internal (compute4.nyi.internal [10.202.2.44])
 by mailout.nyi.internal (Postfix) with ESMTP id 0B3F65C00B6;
 Sat, 10 Jul 2021 04:48:58 -0400 (EDT)
Received: from mailfrontend1 ([10.202.2.162])
 by compute4.internal (MEProxy); Sat, 10 Jul 2021 04:48:58 -0400
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=monjalon.net; h=
 from:to:cc:subject:date:message-id:in-reply-to:references
 :mime-version:content-transfer-encoding:content-type; s=fm1; bh=
 Uii56NW3afC0Uy4/g49aBc7PSqkn3be57MWlRS7cpxk=; b=vRP2i/LFYWCjw3jm
 yewVRvuIhLxdhJFLL4+Y8uYzaKGYPen0NZkJKi4iX0g1S2wIDu9pj5hdm/hQs45O
 Kk5nOd7wPa0UwZ6BSP75KqQXTOXvDpu2nBA9LZ27m9jcpbw35ww/tWu4SB1gj8DQ
 LwRJUNwx0klRCP4pveTnoseuJHEECIIXhQ1/qIY0N5haj0ord2QuK/VaL1B6lTJx
 E8zQtlvw09xaZhce+nrhdNyWPTXmRXsBM5aWkyH2eamlQ2lWUGg4eI1KJiQYWO6y
 LjNHTjQyh6znYJ64rwAOxgHGgxvmYPLKhTOgFrkn47hGrUbBXuQYH3bk9vvAG04J
 HQ5X1g==
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=
 messagingengine.com; h=cc:content-transfer-encoding:content-type
 :date:from:in-reply-to:message-id:mime-version:references
 :subject:to:x-me-proxy:x-me-proxy:x-me-sender:x-me-sender
 :x-sasl-enc; s=fm3; bh=Uii56NW3afC0Uy4/g49aBc7PSqkn3be57MWlRS7cp
 xk=; b=iQ6UFh1rRQNXI0GMCwLFPl13mrW0tlp8+mXpEW/cVNnkkVw87syobcQ4O
 VEuuavY24aUs4nQi4cDvLYyYUPiJa0x5o1QtYm/7jj34uYCI1Rn3hNs9Amp15P6n
 Zlw9zRTHLhvu/y9jNPyxpSvYyvmyU6ywk6PHj7gFAb7OXeF9VXxIiqn+i5FwJwei
 hdDrOivvh8OsqSUrQsP1M5V7nw9oR+bK4IgT37R76bTq/6PgJfLB4wKxcQ/nVbcD
 cwbKlGxRkKLS6iPDYzCeD6S3sFMmDP42iT9Rydug0vNc5UkPQ8907+4pnetbSXag
 hYTmWV6gcfN/RK/stwWcd9Lb8OMcg==
X-ME-Sender: <xms:-V7pYOKx_uAETN8gF8M4G54W3cgJW3TDVdEYFDzvz7ckiZs50Uc9Vg>
 <xme:-V7pYGLlkODUlG7hmBO7-_V2Mwi4sI_u_Q0YVqznqoV8ldsD1T7P-gJ2pbs6Mw3fj
 lcx-3pqFW14-PaUgw>
X-ME-Received: <xmr:-V7pYOsOgDXNu6lKHgqXhDm3dSIIhwxgWeQ-y-Vb4qR288yaHmNVAeb_l0jPsQCdZygZZn1E4MoTnXB0gqJhLHw>
X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvtddrtdekgddutdcutefuodetggdotefrodftvf
 curfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfghnecu
 uegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmdenuc
 fjughrpefhvffufffkjghfggfgtgesthfuredttddtvdenucfhrhhomhepvfhhohhmrghs
 ucfoohhnjhgrlhhonhcuoehthhhomhgrshesmhhonhhjrghlohhnrdhnvghtqeenucggtf
 frrghtthgvrhhnpedugefgvdefudfftdefgeelgffhueekgfffhfeujedtteeutdejueei
 iedvffegheenucevlhhushhtvghrufhiiigvpedtnecurfgrrhgrmhepmhgrihhlfhhroh
 hmpehthhhomhgrshesmhhonhhjrghlohhnrdhnvght
X-ME-Proxy: <xmx:-V7pYDbodKRUG7ttfgrVEv0Hn3a6lDbB-o-cAd5XjGJ_HOTLQ_rCKg>
 <xmx:-V7pYFYnczDNzCQPiXMX9mVyUncZ1Xar2VvbXUYE8JSunn7oW4eGfw>
 <xmx:-V7pYPCfjW5od-PphVGA_b77aZiNGSh-qRR_-GMuQ6VI1kNDLEt89g>
 <xmx:-l7pYKNT5CMlqt2UW_b9jbzbmJ0u2iduqoYKEqBLJEXXloH_cu4M9Q>
Received: by mail.messagingengine.com (Postfix) with ESMTPA; Sat,
 10 Jul 2021 04:48:55 -0400 (EDT)
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
Date: Sat, 10 Jul 2021 10:38:07 +0200
Message-ID: <18478202.kP3E1pVba9@thomas>
In-Reply-To: <4c74f418-bbc6-93e3-c555-b513bd01be19@oktetlabs.ru>
References: <20210603080352.10924-1-alvinx.zhang@intel.com>
 <83f658dea5de4ab9a94c6078460edff7@intel.com>
 <4c74f418-bbc6-93e3-c555-b513bd01be19@oktetlabs.ru>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="us-ascii"
Subject: Re: [dpdk-dev] [PATCH v3] ethdev: add IPv4 and L4 checksum RSS
 offload types
X-BeenThere: dev@dpdk.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>

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.