From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id F4159A32A2 for ; Thu, 24 Oct 2019 12:25:02 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 062031C1EB; Thu, 24 Oct 2019 12:25:01 +0200 (CEST) Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) by dpdk.org (Postfix) with ESMTP id 4F1761C1B6 for ; Thu, 24 Oct 2019 12:24:59 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by orsmga105.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 24 Oct 2019 03:24:58 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.68,224,1569308400"; d="scan'208";a="228461712" Received: from fyigit-mobl.ger.corp.intel.com (HELO [10.237.221.10]) ([10.237.221.10]) by fmsmga002.fm.intel.com with ESMTP; 24 Oct 2019 03:24:45 -0700 From: Ferruh Yigit To: Pavan Nikhilesh Bhagavatula , Andrew Rybchenko , Jerin Jacob Kollanukkaran Cc: "dev@dpdk.org" , Adrien Mazarguil , Thomas Monjalon , Xiaolong Ye , Bruce Richardson References: Openpgp: preference=signencrypt Autocrypt: addr=ferruh.yigit@intel.com; prefer-encrypt=mutual; keydata= mQINBFXZCFABEADCujshBOAaqPZpwShdkzkyGpJ15lmxiSr3jVMqOtQS/sB3FYLT0/d3+bvy qbL9YnlbPyRvZfnP3pXiKwkRoR1RJwEo2BOf6hxdzTmLRtGtwWzI9MwrUPj6n/ldiD58VAGQ +iR1I/z9UBUN/ZMksElA2D7Jgg7vZ78iKwNnd+vLBD6I61kVrZ45Vjo3r+pPOByUBXOUlxp9 GWEKKIrJ4eogqkVNSixN16VYK7xR+5OUkBYUO+sE6etSxCr7BahMPKxH+XPlZZjKrxciaWQb +dElz3Ab4Opl+ZT/bK2huX+W+NJBEBVzjTkhjSTjcyRdxvS1gwWRuXqAml/sh+KQjPV1PPHF YK5LcqLkle+OKTCa82OvUb7cr+ALxATIZXQkgmn+zFT8UzSS3aiBBohg3BtbTIWy51jNlYdy ezUZ4UxKSsFuUTPt+JjHQBvF7WKbmNGS3fCid5Iag4tWOfZoqiCNzxApkVugltxoc6rG2TyX CmI2rP0mQ0GOsGXA3+3c1MCdQFzdIn/5tLBZyKy4F54UFo35eOX8/g7OaE+xrgY/4bZjpxC1 1pd66AAtKb3aNXpHvIfkVV6NYloo52H+FUE5ZDPNCGD0/btFGPWmWRmkPybzColTy7fmPaGz cBcEEqHK4T0aY4UJmE7Ylvg255Kz7s6wGZe6IR3N0cKNv++O7QARAQABtCVGZXJydWggWWln aXQgPGZlcnJ1aC55aWdpdEBpbnRlbC5jb20+iQJUBBMBCgA+AhsDAh4BAheABQsJCAcDBRUK CQgLBRYCAwEAFiEE0jZTh0IuwoTjmYHH+TPrQ98TYR8FAl1meboFCQlupOoACgkQ+TPrQ98T YR9ACBAAv2tomhyxY0Tp9Up7mNGLfEdBu/7joB/vIdqMRv63ojkwr9orQq5V16V/25+JEAD0 60cKodBDM6HdUvqLHatS8fooWRueSXHKYwJ3vxyB2tWDyZrLzLI1jxEvunGodoIzUOtum0Ce gPynnfQCelXBja0BwLXJMplM6TY1wXX22ap0ZViC0m714U5U4LQpzjabtFtjT8qOUR6L7hfy YQ72PBuktGb00UR/N5UrR6GqB0x4W41aZBHXfUQnvWIMmmCrRUJX36hOTYBzh+x86ULgg7H2 1499tA4o6rvE13FiGccplBNWCAIroAe/G11rdoN5NBgYVXu++38gTa/MBmIt6zRi6ch15oLA Ln2vHOdqhrgDuxjhMpG2bpNE36DG/V9WWyWdIRlz3NYPCDM/S3anbHlhjStXHOz1uHOnerXM 1jEjcsvmj1vSyYoQMyRcRJmBZLrekvgZeh7nJzbPHxtth8M7AoqiZ/o/BpYU+0xZ+J5/szWZ aYxxmIRu5ejFf+Wn9s5eXNHmyqxBidpCWvcbKYDBnkw2+Y9E5YTpL0mS0dCCOlrO7gca27ux ybtbj84aaW1g0CfIlUnOtHgMCmz6zPXThb+A8H8j3O6qmPoVqT3qnq3Uhy6GOoH8Fdu2Vchh TWiF5yo+pvUagQP6LpslffufSnu+RKAagkj7/RSuZV25Ag0EV9ZMvgEQAKc0Db17xNqtSwEv mfp4tkddwW9XA0tWWKtY4KUdd/jijYqc3fDD54ESYpV8QWj0xK4YM0dLxnDU2IYxjEshSB1T qAatVWz9WtBYvzalsyTqMKP3w34FciuL7orXP4AibPtrHuIXWQOBECcVZTTOdZYGAzaYzxiA ONzF9eTiwIqe9/oaOjTwTLnOarHt16QApTYQSnxDUQljeNvKYt1lZE/gAUUxNLWsYyTT+22/ vU0GDUahsJxs1+f1yEr+OGrFiEAmqrzpF0lCS3f/3HVTU6rS9cK3glVUeaTF4+1SK5ZNO35p iVQCwphmxa+dwTG/DvvHYCtgOZorTJ+OHfvCnSVjsM4kcXGjJPy3JZmUtyL9UxEbYlrffGPQ I3gLXIGD5AN5XdAXFCjjaID/KR1c9RHd7Oaw0Pdcq9UtMLgM1vdX8RlDuMGPrj5sQrRVbgYH fVU/TQCk1C9KhzOwg4Ap2T3tE1umY/DqrXQgsgH71PXFucVjOyHMYXXugLT8YQ0gcBPHy9mZ qw5mgOI5lCl6d4uCcUT0l/OEtPG/rA1lxz8ctdFBVOQOxCvwRG2QCgcJ/UTn5vlivul+cThi 6ERPvjqjblLncQtRg8izj2qgmwQkvfj+h7Ex88bI8iWtu5+I3K3LmNz/UxHBSWEmUnkg4fJl Rr7oItHsZ0ia6wWQ8lQnABEBAAGJAjwEGAEKACYCGwwWIQTSNlOHQi7ChOOZgcf5M+tD3xNh HwUCXWZ5wAUJB3FgggAKCRD5M+tD3xNhH2O+D/9OEz62YuJQLuIuOfL67eFTIB5/1+0j8Tsu o2psca1PUQ61SZJZOMl6VwNxpdvEaolVdrpnSxUF31kPEvR0Igy8HysQ11pj8AcgH0a9FrvU /8k2Roccd2ZIdpNLkirGFZR7LtRw41Kt1Jg+lafI0efkiHKMT/6D/P1EUp1RxOBNtWGV2hrd 0Yg9ds+VMphHHU69fDH02SwgpvXwG8Qm14Zi5WQ66R4CtTkHuYtA63sS17vMl8fDuTCtvfPF HzvdJLIhDYN3Mm1oMjKLlq4PUdYh68Fiwm+boJoBUFGuregJFlO3hM7uHBDhSEnXQr5mqpPM 6R/7Q5BjAxrwVBisH0yQGjsWlnysRWNfExAE2sRePSl0or9q19ddkRYltl6X4FDUXy2DTXa9 a+Fw4e1EvmcF3PjmTYs9IE3Vc64CRQXkhujcN4ZZh5lvOpU8WgyDxFq7bavFnSS6kx7Tk29/ wNJBp+cf9qsQxLbqhW5kfORuZGecus0TLcmpZEFKKjTJBK9gELRBB/zoN3j41hlEl7uTUXTI JQFLhpsFlEdKLujyvT/aCwP3XWT+B2uZDKrMAElF6ltpTxI53JYi22WO7NH7MR16Fhi4R6vh FHNBOkiAhUpoXRZXaCR6+X4qwA8CwHGqHRBfYFSU/Ulq1ZLR+S3hNj2mbnSx0lBs1eEqe2vh cA== Message-ID: Date: Thu, 24 Oct 2019 11:24:44 +0100 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH v12 0/7] ethdev: add new Rx offload flags X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 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" On 10/24/2019 10:47 AM, Ferruh Yigit wrote: > On 10/22/2019 4:20 PM, Pavan Nikhilesh Bhagavatula wrote: >>> -----Original Message----- >>> From: Ferruh Yigit >>> Sent: Tuesday, October 22, 2019 7:51 PM >>> To: Andrew Rybchenko ; Pavan Nikhilesh >>> Bhagavatula ; Jerin Jacob Kollanukkaran >>> >>> Cc: dev@dpdk.org; Adrien Mazarguil ; >>> Thomas Monjalon ; Xiaolong Ye >>> ; Bruce Richardson >>> >>> Subject: Re: [dpdk-dev] [PATCH v12 0/7] ethdev: add new Rx >>> offload flags >>> On 10/22/2019 11:16 AM, Andrew Rybchenko wrote: >>>> Hi, >>>> >>>> @Pavan, see question below. >>>> >>>> On 10/21/19 6:34 PM, Ferruh Yigit wrote: >>>>> On 10/21/2019 4:19 PM, Pavan Nikhilesh Bhagavatula wrote: >>>>>> Hi Ferruh, >>>>>> >>>>>>> -----Original Message----- >>>>>>> From: Ferruh Yigit >>>>>>> Sent: Monday, October 21, 2019 8:37 PM >>>>>>> To: Andrew Rybchenko ; Pavan >>> Nikhilesh >>>>>>> Bhagavatula ; Jerin Jacob >>> Kollanukkaran >>>>>>> >>>>>>> Cc: dev@dpdk.org; Adrien Mazarguil >>> ; >>>>>>> Thomas Monjalon ; Xiaolong Ye >>>>>>> ; Bruce Richardson >>>>>>> >>>>>>> Subject: Re: [dpdk-dev] [PATCH v12 0/7] ethdev: add new Rx >>>>>>> offload flags >>>>>>> On 10/18/2019 11:31 AM, Andrew Rybchenko wrote: >>>>>>>> On 10/18/19 12:42 PM, Ferruh Yigit wrote: >>>>>>>>> On 10/18/2019 8:32 AM, Andrew Rybchenko wrote: >>>>>>>>>> Hi Ferruh, >>>>>>>>>> >>>>>>>>>> since I've reviewed I'll reply as I understand it. >>>>>>>>>> >>>>>>>>>> On 10/17/19 8:43 PM, Ferruh Yigit wrote: >>>>>>>>>>> On 10/17/2019 1:02 PM, pbhagavatula@marvell.com wrote: >>>>>>>>>>>> From: Pavan Nikhilesh >>>>>>>>>>>> >>>>>>>>>>>> Add new Rx offload flags `DEV_RX_OFFLOAD_RSS_HASH` >>> and >>>>>>>>>>>> `DEV_RX_OFFLOAD_FLOW_MARK`. These flags can be >>> used to >>>>>>>>>>>> enable/disable PMD writes to rte_mbuf fields `hash.rss` >>> and >>>>>>> `hash.fdir.hi` >>>>>>>>>>>> and also `ol_flags:PKT_RX_RSS` and >>> `ol_flags:PKT_RX_FDIR`. >>>>>>>>>>> Hi Pavan, >>>>>>>>>>> >>>>>>>>>>> Initially sorry for involving late, >>>>>>>>>>> >>>>>>>>>>> When we expose an interface to the applications, they will >>> expect >>>>>>> those will be >>>>>>>>>>> respected by underlying PMDs. >>>>>>>>>>> As far as I can see drivers are updated to report new added >>> Rx >>>>>>> offload flags as >>>>>>>>>>> supported capabilities but drivers are not using those flags at >>> all, >>>>>>> so >>>>>>>>>>> application providing that flag won't really enable/disable >>>>>>> anything, I think >>>>>>>>>>> this is a problem and it is wrong to lie even for the PMDs J >>>>>>>>>> It is required to let applications know that the offload is >>> supported. >>>>>>>>>> There are a number of cases when an offload cannot be >>> disabled, >>>>>>>>>> but it does not mean that the offload must not be advertised. >>>>>>>>> Can't disable is something else, although I believe that is rare >>> case, in >>>>>>> this >>>>>>>>> case driver can enable/disable the RSS and representing this as >>> an >>>>>>> offload >>>>>>>>> capability. >>>>>>>> It is not enabling/disabling the RSS. It is enabling/disabling RSS >>> hash >>>>>>>> delivery >>>>>>>> together with an mbuf. >>>>>>> >>>>>>> Got it, it is related to the RSS hash delivery. >>>>>>> >>>>>>>>> But when user want to configure this offload by setting or >>> unsetting >>>>>>> in offload >>>>>>>>> config, driver just ignores it. >>>>>>>> When application enables offload, it says that it needs it and >>> going to >>>>>>> use >>>>>>>> (required). When the offload is not enabled, application simply >>> don't >>>>>>> care. >>>>>>>> So, if the information is still provided it does not harm. >>>>>>> >>>>>>> Not sure if there is no harm, a config option not respected by >>>>>>> underlying PMDs >>>>>>> silently is a problem I think. >>>> >>>> Some time ago the idea of offloads which cannot be disabled >>>> by PMD was discussed and, if I remember decision correctly, >>>> it was decided that it will overcomplicate it. >>>> It was discussed when new offload API is introduced. >>>> Logging is helpful sometimes, but it is not a panacea. >>> >>> I remember it has been discussed in the context of CRC, because some >>> HW were not >>> able to disable CRC STRIP, it has been implemented as I suggested for >>> this case, >>> when application requests to disable CRC STRIP offload, the PMD print a >>> log >>> saying it can't be disabled and keep the internal state as offload >>> enabled. >>> >>> Other than this I think all offloads announced as supported by PMD can >>> be >>> enabled and disabled. >>> >>>> >>>>>>>>>> If driver see benefits from disabling the offload (e.g. avoid >>> delivery >>>>>>>>>> of RSS hash from NIC to host), it can do it after the patchset. >>>>>>>>> Yes but I think this patchset shouldn't ignore that disabling the >>>>>>> feature is not >>>>>>>>> implemented yet. If those PMDs that has been updated to >>> report >>>>>>> the HASH >>>>>>>>> capability has RSS enabled by default, I suggest adding a check >>> for >>>>>>> this offload >>>>>>>>> in PMD, >>>>>>>>> if it is requested to disable (which means not requested for >>> enable), >>>>>>> print a >>>>>>>>> log saying disabling HASH is not supported and set this flag in >>> the >>>>>>> offload >>>>>>>>> configuration to say PMD is configured to calculate the HASH. >>>>>>>>> Later PMD maintainers may prefer to replace that error log with >>>>>>> actual disable code. >>>>>>>> It is possible to do. Of course, it is better to provide real offload >>>>>>>> values on get, but >>>>>>>> eth_conf is const in rte_eth_dev_configure(), so, we can't >>> change it >>>>>>> and >>>>>>>> it is good. >>>>>>>> So, the only way is rte_eth_rx_queue_info_get(). >>>>>>>> I guess there is a lot of space for the same improvement for >>> other Rx >>>>>>>> offloads >>>>>>>> in various PMDs. >>>>>>> >>>>>>> We don't need the update 'eth_conf' parameter of the >>>>>>> 'rte_eth_dev_configure()', >>>>>>> that is what user requested, but config stored in 'dev->data- >>>> dev_conf' >>>>>>> which >>>>>>> can be updated. >>>> >>>> What for? As I understand data->dev_conf should not be used >>> outside >>>> librte_ethdev and drivers. Basically it means that all patched drivers >>>> should be updated to log information message if the offload is not >>>> requested, but will be enabled anyway and updated >>>> data->dev_conf.rxmode.offloads. Please, confirm. >>> >>> To keep PMD internal state correct. >>> >>>> >>>>>>>> Also I worry that it could be not that trivial to do in all effected >>> PMDs. >>>>>>> >>>>>>> Yes it can be some work, and if this patchset doesn't do it, who >>> will do >>>>>>> the work? >>>> >>>> Pavan, will you do or should I care about it if confirmed? >> >> I will send a v13. >> >>>> >>>>>>>>>>> Specific to `DEV_RX_OFFLOAD_RSS_HASH`, we have already >>>>>>> some RSS config >>>>>>>>>>> structures and it is part of the 'rte_eth_dev_configure()' API, >>>>>>> won't it create >>>>>>>>>>> multiple way to do same thing? >>>>>>>>>> No, a new offload is responsible for RSS hash delivery from >>> NIC to >>>>>>> host >>>>>>>>>> and fill in in mbuf returned to application on Rx. >>>>>>>>> What you have described is already happening without the >>> new >>>>>>> offload flag and >>>>>>>>> this is my concern that we are duplicating it. >>>>>>>>> >>>>>>>>> >>>>>>>>> There is a 'struct rte_eth_rxmode' (under 'struct rte_eth_conf') >>>>>>>>> which has 'enum rte_eth_rx_mq_mode mq_mode;' >>>>>>>>> >>>>>>>>> If "mq_mode == ETH_MQ_RX_NONE" hash calculation is >>> disabled, >>>>>>> and >>>>>>>>> 'mbuf::hash::rss' is not updated. >>>>>>>> No-no. It binds RSS distribution and hash delivery. What the new >>>>>>>> offload allows to achieve: I want Rx to spread traffic over many >>> Rx >>>>>>>> queues, but I don't need RSS hash. >>>>>>> >>>>>>> I see, so RSS configuration will stay same, but driver needs to take >>> care >>>>>>> the >>>>>>> new flags to decide to update or not the mbuf::rss::hash field. >>>>>>> >>>>>>> I don't know if disabling RSS but calculating hash is supported, if >>> not >>>>>>> supported that case also should be checked by driver. >>>> >>>> Yes, it is interesting question what should happen if >>> ETH_MQ_RX_NONE with >>>> DEV_RX_OFFLOAD_RSS_HASH. It sounds like rte_ethdev should >>> check >>>> dev_conf.rxmode.mq_mode & ETH_MQ_RX_RSS_FLAG is set when >>>> DEV_RX_OFFLOAD_RSS_HASH is requested. Otherwise the request is >>> invalid. >>> >>> +1 >>> >>>> May be it could be relaxed in the future, but not now. >>>> >>>>>>>>> (Thanks Bruce to helping finding it out) >>>>>>>>> >>>>>>>>> >>>>>>>>>>> And for the `ol_flags:PKT_RX_RSS` flag, it was already used >>> to >>>>>>> mark that >>>>>>>>>>> 'mbuf::hash::rss' is valid, right? Is there anything new related >>> that >>>>>>> in the set? >>>>>>>>>> As I understand you mean, ol_flags::PKT_RX_RSS_HASH. >>>>>>>>>> Yes, the new offload allows say if application needs it or now. >>>>>>>>>> Basically it decouples RSS distribution and hash delivery. >>>>>>>>> Setting 'ol_flags::PKT_RX_RSS_HASH' and 'mbuf::hash::rss' >>> already >>>>>>> there and not >>>>>>>>> changing. I just want to clarify since this is not clear in the >>> commit log. >>>>>>>>> >>>>>>>>> Only addition is to add a new flag to control PMD to >>> enable/disable >>>>>>> hash >>>>>>>>> calculation (which PMDs ignore in the patch ???) >>>>>>>> It is not calculation, but delivery of the value from HW to >>> applications. >>>>>>> >>>>>>> OK >>>>>>> >>>>>>>> Yes, commit log may/should be improved.> >>>>>>>>>>> Specific to the `DEV_RX_OFFLOAD_FLOW_MARK` and >>>>>>> `RTE_FLOW_ACTION_FLAG`, they are >>>>>>>>>>> rte_flow actions, application can verify and later request >>> these >>>>>>> actions via >>>>>>>>>>> rte_flow APIs. Why we are adding an additional >>> RX_OFFLOAD flag >>>>>>> for them? >>>>>>>>>> The reason is basically the same as above. HW needs to know >>> in >>>>>>> advance, >>>>>>>>>> if application is going to use flow marks and configure Rx >>> queue to >>>>>>> enable >>>>>>>>>> the information delivery. >>>>>>>>> What you described is done via 'rte_flow_create()' API, >>> application >>>>>>> will request >>>>>>>>> those actions via API and Rx queue will be configured >>> accordingly, >>>>>>> this is more >>>>>>>>> dynamic approach. Why application need to set this additional >>>>>>> configuration flag? >>>>>>>> More dynamic approach is definitely better, but it is not always >>>>>>> possible. >>>>>>>> Some PMDs can't even change MTU dynamically or MTU >>> changing >>>>>>> requires >>>>>>>> restart which is hardly really a dynamic change. Of course, it is >>>>>>>> unlikely that >>>>>>>> MTU is changed when traffic is running etc, but still possible. >>>>>>>> The information about necessity to support flow marks delivery >>> may >>>>>>>> be required on Rx queue setup and cannot be changed >>> dynamically >>>>>>> when >>>>>>>> Rx queue is running and application would like to add flow rule >>> with >>>>>>> mark >>>>>>>> action. >>>>>>> It doesn't need to be changed dynamically, application can call >>>>>>> 'rte_flow_validate()' and learn if it can set this action or not. >>> Perhaps I >>>>>>> am >>>>>>> missing something, when it is required to have this as >>> configuration >>>>>>> option? >>>>>>> >>>>>>>>> And as above the new RX offload flags ignored by PMDs, hard >>> to >>>>>>> understand what >>>>>>>>> is the intention here. >>>>>>>>> >>>>>>>>> >>>>>>>>> Above usage of flags feels like the intention is adding some >>> capability >>>>>>>>> information for the PMDs more that adding new offload >>>>>>> configuration. >>>>>>>>> If so this is bigger/older problem, and instead of abusing the >>> offload >>>>>>> flags we >>>>>>>>> can think of an API to present device capabilities, and move >>>>>>> features.ini >>>>>>>>> content to the API in long term. >>>>>>>> What I really like with these new offload flags for Rx hash and >>> flow >>>>>>> mark is >>>>>>>> that it makes features which provide information in mbuf on Rx >>>>>>> consistent: >>>>>>>>  - want timestamp? => DEV_RX_OFFLOAD_TIMESTAMP >>>>>>>>  - want Rx checksum flags => DEV_RX_OFFLOAD_CHECKSUM >>>>>>>>  - want to strip VLAN? => DEV_RX_OFFLOAD_VLAN_STRIP >>>>>>>>  - want RSS hash? => DEV_RX_OFFLOAD_RSS_HASH >>>>>>>>  - want flow mark support? => >>> DEV_RX_OFFLOAD_FLOW_MARK >>>>>>>> >>>>>>>> Also it perfectly fits dynamic mbuf fields and allows to make RSS >>> hash >>>>>>>> and flow mark fields dynamic with the new offloads as controls >>>>>>> Agree RSS_HASH fits well, my main concern with the patchset is >>> driver >>>>>>> implementations are missing and just ignored. >>>>>>> >>>>>> Ignoring driver implementation is intentional as it involves adding a >>> branch >>>>>> in Rx fastpath function for all drivers and might have -ve effects on >>> performance. >>>>> Yes it may affect performance. Also it may be too much driver >>> specific >>>>> implementation. >>>>> >>>>> That is why I suggest, following: >>>>> For the drivers that claim this capability, >>>>> - For the case driver updates the mbuf::rss:hash >>>>> Check if this offload requested or not, if not print an error and set >>> internal >>>>> config as this offload enabled >>>> >>>> I would not say it is an error. At maximum it is warning, but I would >>> use >>>> just info log level. I think there is nothing critical there. >>> >>> Any log is OK, only info level my be disabled by default by many drivers, >>> to be >>> sure this is printed, warning can be better. >>> >>>> >>>>> - For the case driver not updates the mbuf::rss:hash >>>>> Check if this offload requested or not, if requested print an error >>> and set >>>>> internal config as this offload disabled >>>> >>>> If so, the offload is not advertised and generic checks in rte_ethdev >>>> catch it and return error. >>> >>> You are right, for this case it shouldn't be advertised. >>> >>> >>> Just to double check the understanding of this discussion, is following >>> summary >>> correct? >>> >>> A) When RSS enabled >>> 1) if RSS_HASH set, put hash value to "mbuf::rss:hash" and set >>> PKT_RX_RSS_HASH >>> 2) if RSS_HASH unset, don't updated >>> "mbuf::rss:hash"/PKT_RX_RSS_HASH >>> >>> B) When RSS disabled >>> 3) if RSS_HASH set, >>> i) if HW supports HASH calculation offload independently, enable it >>> and put hash value to "mbuf::rss:hash" and set PKT_RX_RSS_HASH >>> ii) if HW doesn't support it return error >>> 4) if RSS_HASH unset, don't updated >>> "mbuf::rss:hash"/PKT_RX_RSS_HASH >>> >>> >>> This may be good for (3) to enable HASH calculation offload >>> independent from >>> RSS. Not sure if this is supported by HW or this is the motivation of the >>> patch. >>> >>> But for (1), since the hash value is already part of descriptor, this will >>> bring >>> an additional check, as Pavan mentioned, which is not good. >>> >>> For (3) is there any intention to implicitly enable RSS, this was my >>> understanding and concern to have two different methods to enable >>> RSS, >>> I guess my understanding was wrong but I want to confirm this. >>> >>> Currently RSS hash delivery is implied when RSS is enabled. >>> >> >> Correct me if I'm wrong but current agreement is to print a log message when application >> tries to disable RSS and it's not supported by underlying PMD. > > Yes. > >> So all the PMD which we aren’t currently modifying need to be have a log message. > > No. If you mean all PMD that is announcing the capability, yes, as you said. I thought you are referring to other PMDs too. > If a PMD not announced this capability, it doesn't need to check this flag. From > the PMDs that announce this capability, the ones that doesn't support disabling > the offload should log this and update the PMD config to represent the actual > status. > >> Also, it applies to FLOW_FLAG and FLOW_MARK. > > I am not really sure about them, I think we are duplicating the rte_flow API and > we shouldn't have them as Rx offload, I would like to hear more comment about this. > >> >> If we are at an agreement I will send v13. >> >> Thanks, >> Pavan. >> >>>> >>>>> Later PMD maintainers may prefer to replace those errors with >>> actual >>>>> implementation if they want. >>>> >>>> [snip] >>>> >> >