From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: 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 ; 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: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvtddrtdekgddutdcutefuodetggdotefrodftvf curfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfghnecu uegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmdenuc fjughrpefhvffufffkjghfggfgtgesthfuredttddtvdenucfhrhhomhepvfhhohhmrghs ucfoohhnjhgrlhhonhcuoehthhhomhgrshesmhhonhhjrghlohhnrdhnvghtqeenucggtf frrghtthgvrhhnpedugefgvdefudfftdefgeelgffhueekgfffhfeujedtteeutdejueei iedvffegheenucevlhhushhtvghrufhiiigvpedtnecurfgrrhgrmhepmhgrihhlfhhroh hmpehthhhomhgrshesmhhonhhjrghlohhnrdhnvght X-ME-Proxy: Received: by mail.messagingengine.com (Postfix) with ESMTPA; Sat, 10 Jul 2021 04:48:55 -0400 (EDT) From: Thomas Monjalon To: "Zhang, Qi Z" , "Zhang, AlvinX" , Andrew Rybchenko Cc: "ajit.khaparde@broadcom.com" , dev@dpdk.org, Ferruh Yigit , Ori Kam , 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 > >>> From: Andrew Rybchenko > >>>> On 7/7/21 6:23 AM, Zhang, Qi Z wrote: > >>>>> From: Andrew Rybchenko > >>>>>> On 7/6/21 10:18 AM, Zhang, Qi Z wrote: > >>>>>>> From: Zhang, AlvinX > >>>>>>>> > >>>>>>>>>> @@ -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.