Re: [dpdk-dev] [PATCH v12 0/7] ethdev: add new Rx offload flags

Re: [dpdk-dev] [PATCH v12 0/7] ethdev: add new Rx offload flags

[Pavan Nikhilesh Bhagavatula]

>-----Original Message-----
>From: Ferruh Yigit <ferruh.yigit@intel.com>
>Sent: Tuesday, October 22, 2019 7:51 PM
>To: Andrew Rybchenko <arybchenko@solarflare.com>; Pavan Nikhilesh
>Bhagavatula <pbhagavatula@marvell.com>; Jerin Jacob Kollanukkaran
><jerinj@marvell.com>
>Cc: dev@dpdk.org; Adrien Mazarguil <adrien.mazarguil@6wind.com>;
>Thomas Monjalon <thomas@monjalon.net>; Xiaolong Ye
><xiaolong.ye@intel.com>; Bruce Richardson
><bruce.richardson@intel.com>
>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 <ferruh.yigit@intel.com>
>>>>> Sent: Monday, October 21, 2019 8:37 PM
>>>>> To: Andrew Rybchenko <arybchenko@solarflare.com>; Pavan
>Nikhilesh
>>>>> Bhagavatula <pbhagavatula@marvell.com>; Jerin Jacob
>Kollanukkaran
>>>>> <jerinj@marvell.com>
>>>>> Cc: dev@dpdk.org; Adrien Mazarguil
><adrien.mazarguil@6wind.com>;
>>>>> Thomas Monjalon <thomas@monjalon.net>; Xiaolong Ye
>>>>> <xiaolong.ye@intel.com>; Bruce Richardson
>>>>> <bruce.richardson@intel.com>
>>>>> 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 <pbhagavatula@marvell.com>
>>>>>>>>>>
>>>>>>>>>>     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.
So all the PMD which we aren’t currently modifying need to be have a log message.
Also, it applies to FLOW_FLAG and FLOW_MARK.

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]
>>

Re: [dpdk-dev] [PATCH v12 0/7] ethdev: add new Rx offload flags

[Ferruh Yigit]

On 10/22/2019 4:20 PM, Pavan Nikhilesh Bhagavatula wrote:
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>> Sent: Tuesday, October 22, 2019 7:51 PM
>> To: Andrew Rybchenko <arybchenko@solarflare.com>; Pavan Nikhilesh
>> Bhagavatula <pbhagavatula@marvell.com>; Jerin Jacob Kollanukkaran
>> <jerinj@marvell.com>
>> Cc: dev@dpdk.org; Adrien Mazarguil <adrien.mazarguil@6wind.com>;
>> Thomas Monjalon <thomas@monjalon.net>; Xiaolong Ye
>> <xiaolong.ye@intel.com>; Bruce Richardson
>> <bruce.richardson@intel.com>
>> 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 <ferruh.yigit@intel.com>
>>>>>> Sent: Monday, October 21, 2019 8:37 PM
>>>>>> To: Andrew Rybchenko <arybchenko@solarflare.com>; Pavan
>> Nikhilesh
>>>>>> Bhagavatula <pbhagavatula@marvell.com>; Jerin Jacob
>> Kollanukkaran
>>>>>> <jerinj@marvell.com>
>>>>>> Cc: dev@dpdk.org; Adrien Mazarguil
>> <adrien.mazarguil@6wind.com>;
>>>>>> Thomas Monjalon <thomas@monjalon.net>; Xiaolong Ye
>>>>>> <xiaolong.ye@intel.com>; Bruce Richardson
>>>>>> <bruce.richardson@intel.com>
>>>>>> 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 <pbhagavatula@marvell.com>
>>>>>>>>>>>
>>>>>>>>>>>     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 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]
>>>
> 

Re: [dpdk-dev] [PATCH v12 0/7] ethdev: add new Rx offload flags

[Ferruh Yigit]

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 <ferruh.yigit@intel.com>
>>> Sent: Tuesday, October 22, 2019 7:51 PM
>>> To: Andrew Rybchenko <arybchenko@solarflare.com>; Pavan Nikhilesh
>>> Bhagavatula <pbhagavatula@marvell.com>; Jerin Jacob Kollanukkaran
>>> <jerinj@marvell.com>
>>> Cc: dev@dpdk.org; Adrien Mazarguil <adrien.mazarguil@6wind.com>;
>>> Thomas Monjalon <thomas@monjalon.net>; Xiaolong Ye
>>> <xiaolong.ye@intel.com>; Bruce Richardson
>>> <bruce.richardson@intel.com>
>>> 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 <ferruh.yigit@intel.com>
>>>>>>> Sent: Monday, October 21, 2019 8:37 PM
>>>>>>> To: Andrew Rybchenko <arybchenko@solarflare.com>; Pavan
>>> Nikhilesh
>>>>>>> Bhagavatula <pbhagavatula@marvell.com>; Jerin Jacob
>>> Kollanukkaran
>>>>>>> <jerinj@marvell.com>
>>>>>>> Cc: dev@dpdk.org; Adrien Mazarguil
>>> <adrien.mazarguil@6wind.com>;
>>>>>>> Thomas Monjalon <thomas@monjalon.net>; Xiaolong Ye
>>>>>>> <xiaolong.ye@intel.com>; Bruce Richardson
>>>>>>> <bruce.richardson@intel.com>
>>>>>>> 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 <pbhagavatula@marvell.com>
>>>>>>>>>>>>
>>>>>>>>>>>>     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]
>>>>
>>
> 

Re: [dpdk-dev] [PATCH v12 0/7] ethdev: add new Rx offload flags

[Pavan Nikhilesh Bhagavatula]

<snip>
>>> 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.
>

I was referring to the PMDs currently announcing the capability.
I will send the v13.

>> 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.
>>
>>>

Re: [dpdk-dev] [PATCH v12 0/7] ethdev: add new Rx offload flags

[Ferruh Yigit]

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 <ferruh.yigit@intel.com>
>>>> Sent: Monday, October 21, 2019 8:37 PM
>>>> To: Andrew Rybchenko <arybchenko@solarflare.com>; Pavan Nikhilesh
>>>> Bhagavatula <pbhagavatula@marvell.com>; Jerin Jacob Kollanukkaran
>>>> <jerinj@marvell.com>
>>>> Cc: dev@dpdk.org; Adrien Mazarguil <adrien.mazarguil@6wind.com>;
>>>> Thomas Monjalon <thomas@monjalon.net>; Xiaolong Ye
>>>> <xiaolong.ye@intel.com>; Bruce Richardson
>>>> <bruce.richardson@intel.com>
>>>> 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 <pbhagavatula@marvell.com>
>>>>>>>>>
>>>>>>>>>     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?
> 
>>>>>>>> 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.

> 
>> Later PMD maintainers may prefer to replace those errors with actual
>> implementation if they want.
> 
> [snip]
> 

Re: [dpdk-dev] [PATCH v12 0/7] ethdev: add new Rx offload flags

[Andrew Rybchenko]

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 <ferruh.yigit@intel.com>
>>> Sent: Monday, October 21, 2019 8:37 PM
>>> To: Andrew Rybchenko <arybchenko@solarflare.com>; Pavan Nikhilesh
>>> Bhagavatula <pbhagavatula@marvell.com>; Jerin Jacob Kollanukkaran
>>> <jerinj@marvell.com>
>>> Cc: dev@dpdk.org; Adrien Mazarguil <adrien.mazarguil@6wind.com>;
>>> Thomas Monjalon <thomas@monjalon.net>; Xiaolong Ye
>>> <xiaolong.ye@intel.com>; Bruce Richardson
>>> <bruce.richardson@intel.com>
>>> 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 <pbhagavatula@marvell.com>
>>>>>>>>
>>>>>>>>     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.

>>>>>> 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.

>>>> 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?

>>>>>>> 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.
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.

> - 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.

> Later PMD maintainers may prefer to replace those errors with actual
> implementation if they want.

[snip]

Re: [dpdk-dev] [PATCH v12 0/7] ethdev: add new Rx offload flags

[Ferruh Yigit]

On 10/21/2019 4:19 PM, Pavan Nikhilesh Bhagavatula wrote:
> Hi Ferruh,
> 
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>> Sent: Monday, October 21, 2019 8:37 PM
>> To: Andrew Rybchenko <arybchenko@solarflare.com>; Pavan Nikhilesh
>> Bhagavatula <pbhagavatula@marvell.com>; Jerin Jacob Kollanukkaran
>> <jerinj@marvell.com>
>> Cc: dev@dpdk.org; Adrien Mazarguil <adrien.mazarguil@6wind.com>;
>> Thomas Monjalon <thomas@monjalon.net>; Xiaolong Ye
>> <xiaolong.ye@intel.com>; Bruce Richardson
>> <bruce.richardson@intel.com>
>> 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 <pbhagavatula@marvell.com>
>>>>>>>
>>>>>>>    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.
>>
>>>
>>>>> 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.
>>
>>> 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?
>>
>>>
>>>>>> 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.
>>
>>>
>>>> (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

- 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

Later PMD maintainers may prefer to replace those errors with actual
implementation if they want.


> 
>> I am not so sure about FLOW_MARK one, it looks like it is duplicating the
>> rte_flow API.
>>
>>>
>>>>>>>    Add new packet type set function
>> `rte_eth_dev_set_supported_ptypes`,
>>>>>>>    allows application to inform PMDs about the packet types it is
>> interested
>>>>>>>    in. Based on ptypes requested by application PMDs can
>> optimize the Rx path.
>>>>>> OK to the API, but why "Packet type parsing" feature updated to
>> say it should
>>>>>> implement this API?
>>>>>> Is this API really required to say "Packet type parsing" supported
>> by PMD?
>>>>> As I understand it is not strictly required, but related to the feature.
>>>> I am OK with "related", but it is documented as "implements", so doc
>> says it is
>>>> required.
>>>
>>> Agreed.
>>>
>>>>>>>    For example, if a given PMD doesn't support any packet types
>> that the
>>>>>>>    application is interested in then the application can disable[1]
>> writes to
>>>>>>>    `mbuf.packet_type` done by the PMD and use a software
>> ptype parser.
>>>>>>>         [1] rte_eth_dev_set_supported_ptypes(*port_id*,
>> RTE_PTYPE_UNKNOWN,
>>>>>>> 					  NULL, 0);
>>>>>> And for the 7/7 patch, why we are updating all examples, is the
>> API something do
>>>>>> we really need to call for any DPDK application? I am for leaving
>> the default
>>>>>> behavior unless there is a very specific case for set or disable
>> packet typing.
>>>>>> Instead implement a command in testpmd to test this feature.
>>>>> If an application does not use packet types provided in mbuf, it is
>>>>> better to inform PMD that the information is not required to allow
>> PMD
>>>>> to do optimizations.
>>>>>
>>>> I can see disabling packet type detection may increase the
>> performance but
>>>> sample applications are to demonstrate a specific feature, adding
>> these kind of
>>>> APIs will pollute them.
>>>> 'skeleton' app that shows the most basic code for forwarding
>> sample, why it is
>>>> now having "experimental" 'set_supported_ptypes()' API? Same for
>> other. As said
>>>> before I think a testpmd command suits better here.
>>>
>>> May be you're right and we should reconsider which applications
>>> are updated and which are ignored. I guess before the criteria
>>> was simple: don't use packet type information, say so to take
>>> benefits from all possible optimizations.
>>>
>>>>> Yes, may be it would be better to have it as the
>>>>> default behaviour, but it would be behaviour change in comparison
>>>>> to previous DPDK releases and it is better to avoid it.
>>>> Sorry I missed why not calling this function cause behavior change? I
>> think it
>>>> is other way around, no?
>>>
>>> Just misunderstanding. What I was trying to say is that it could
>>> be more logical to have packet type parsing and delivery
>>> disabled by default  (as we have for all other offloads), but
>>> it would be behaviour change from application point of view.
>>> That's why it is necessary to disable explicitly.
>>>
>>> Thanks,
>>> Andrew.
>>>
>>>>> Thanks,
>>>>> Andrew.
>>>>>
>>>>>>> v12 Changes:
>>>>>>> -----------
>>>>>>> - Rebase onto next-net.
>>>>>>>
>>>>>>> v11 Changes:
>>>>>>> -----------
>>>>>>> - Use RTE_DIM to get array size.
>>>>>>> - Since we are using a list of MASKs to validate ptype_mask
>> return -EINVAL
>>>>>>>     if any unknown mask is set.
>>>>>>> - Rebase to TOT.
>>>>>>>
>>>>>>> v10 Changes:
>>>>>>> -----------
>>>>>>> - Modify ptype_mask validation in
>> set_supported_ptypes.(Andrew)
>>>>>>>
>>>>>>> v9 Changes:
>>>>>>> ----------
>>>>>>> - Add ptype_mask validation in set_supported_ptypes.(Andrew)
>>>>>>> - Make description more verbose.
>>>>>>>
>>>>>>> v8 Changes:
>>>>>>> ----------
>>>>>>> - Make description more verbose.
>>>>>>> - Set RTE_PTYPE_UNKNOWN in set_ptypes array when either
>> get ot set ptypes
>>>>>>>     is not supported by ethernet device.
>>>>>>>
>>>>>>> v7 Changes:
>>>>>>> ----------
>>>>>>> - Fix unused variable in net/octeontx2
>>>>>>>
>>>>>>> v6 Changes:
>>>>>>> ----------
>>>>>>> - Add additional checks for set supported ptypes.(Andrew)
>>>>>>> - Clarify `rte_eth_dev_set_supported_ptypes` documentation.
>>>>>>> - Remove DEV_RX_OFFLOAD_FLOW_MARK emulation from
>> net/octeontx2.
>>>>>>>
>>>>>>> v5 Changes:
>>>>>>> ----------
>>>>>>> - Fix typos.
>>>>>>>
>>>>>>> v4 Changes:
>>>>>>> ----------
>>>>>>> - Set the last element in set_ptype array as
>> RTE_PTYPE_UNKNOWN to mark the end
>>>>>>>     of array.
>>>>>>> - Fix invalid set ptype function call in examples.
>>>>>>> - Remove setting rte_eth_dev_set_supported_ptypes to
>> UNKNOWN in l3fwd-power.
>>>>>>>
>>>>>>> v3 Changes:
>>>>>>> ----------
>>>>>>> - Add missing release notes. (Andrew)
>>>>>>> - Re-word various descriptions.
>>>>>>> - Fix ptype set logic.
>>>>>>>
>>>>>>> v2 Changes:
>>>>>>> ----------
>>>>>>> - Update release notes. (Andrew)
>>>>>>> - Redo commit logs. (Andrew)
>>>>>>> - Disable ptype parsing for unsupported examples. (Jerin)
>>>>>>> - Disable RSS write only in generic mode eventdev_pipeline.
>> (Jerin)
>>>>>>> - Modify set_supported_ptypes function to return successfuly
>> set mask
>>>>>>>     instead of failure.
>>>>>>> - Dropped set_supported_ptypes to drivers by handling in library
>>>>>>>     layer, interested PMD can add it in.
>>>>>>>
>>>>>>> Pavan Nikhilesh (7):
>>>>>>>     ethdev: add set ptype function
>>>>>>>     ethdev: add mbuf RSS update as an offload
>>>>>>>     ethdev: add flow action type update as an offload
>>>>>>>     drivers/net: update Rx RSS hash offload capabilities
>>>>>>>     drivers/net: update Rx flow flag and mark capabilities
>>>>>>>     examples/eventdev_pipeline: add new Rx RSS hash offload
>>>>>>>     examples: disable Rx packet type parsing
>>>>>> <...>
>>>
> 

Re: [dpdk-dev] [PATCH v12 0/7] ethdev: add new Rx offload flags

[Pavan Nikhilesh Bhagavatula]

Hi Ferruh,

>-----Original Message-----
>From: Ferruh Yigit <ferruh.yigit@intel.com>
>Sent: Monday, October 21, 2019 8:37 PM
>To: Andrew Rybchenko <arybchenko@solarflare.com>; Pavan Nikhilesh
>Bhagavatula <pbhagavatula@marvell.com>; Jerin Jacob Kollanukkaran
><jerinj@marvell.com>
>Cc: dev@dpdk.org; Adrien Mazarguil <adrien.mazarguil@6wind.com>;
>Thomas Monjalon <thomas@monjalon.net>; Xiaolong Ye
><xiaolong.ye@intel.com>; Bruce Richardson
><bruce.richardson@intel.com>
>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 <pbhagavatula@marvell.com>
>>>>>>
>>>>>>    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.
>
>>
>>>> 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.
>
>> 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?
>
>>
>>>>> 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.
>
>>
>>> (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.

>I am not so sure about FLOW_MARK one, it looks like it is duplicating the
>rte_flow API.
>
>>
>>>>>>    Add new packet type set function
>`rte_eth_dev_set_supported_ptypes`,
>>>>>>    allows application to inform PMDs about the packet types it is
>interested
>>>>>>    in. Based on ptypes requested by application PMDs can
>optimize the Rx path.
>>>>> OK to the API, but why "Packet type parsing" feature updated to
>say it should
>>>>> implement this API?
>>>>> Is this API really required to say "Packet type parsing" supported
>by PMD?
>>>> As I understand it is not strictly required, but related to the feature.
>>> I am OK with "related", but it is documented as "implements", so doc
>says it is
>>> required.
>>
>> Agreed.
>>
>>>>>>    For example, if a given PMD doesn't support any packet types
>that the
>>>>>>    application is interested in then the application can disable[1]
>writes to
>>>>>>    `mbuf.packet_type` done by the PMD and use a software
>ptype parser.
>>>>>>         [1] rte_eth_dev_set_supported_ptypes(*port_id*,
>RTE_PTYPE_UNKNOWN,
>>>>>> 					  NULL, 0);
>>>>> And for the 7/7 patch, why we are updating all examples, is the
>API something do
>>>>> we really need to call for any DPDK application? I am for leaving
>the default
>>>>> behavior unless there is a very specific case for set or disable
>packet typing.
>>>>> Instead implement a command in testpmd to test this feature.
>>>> If an application does not use packet types provided in mbuf, it is
>>>> better to inform PMD that the information is not required to allow
>PMD
>>>> to do optimizations.
>>>>
>>> I can see disabling packet type detection may increase the
>performance but
>>> sample applications are to demonstrate a specific feature, adding
>these kind of
>>> APIs will pollute them.
>>> 'skeleton' app that shows the most basic code for forwarding
>sample, why it is
>>> now having "experimental" 'set_supported_ptypes()' API? Same for
>other. As said
>>> before I think a testpmd command suits better here.
>>
>> May be you're right and we should reconsider which applications
>> are updated and which are ignored. I guess before the criteria
>> was simple: don't use packet type information, say so to take
>> benefits from all possible optimizations.
>>
>>>> Yes, may be it would be better to have it as the
>>>> default behaviour, but it would be behaviour change in comparison
>>>> to previous DPDK releases and it is better to avoid it.
>>> Sorry I missed why not calling this function cause behavior change? I
>think it
>>> is other way around, no?
>>
>> Just misunderstanding. What I was trying to say is that it could
>> be more logical to have packet type parsing and delivery
>> disabled by default  (as we have for all other offloads), but
>> it would be behaviour change from application point of view.
>> That's why it is necessary to disable explicitly.
>>
>> Thanks,
>> Andrew.
>>
>>>> Thanks,
>>>> Andrew.
>>>>
>>>>>> v12 Changes:
>>>>>> -----------
>>>>>> - Rebase onto next-net.
>>>>>>
>>>>>> v11 Changes:
>>>>>> -----------
>>>>>> - Use RTE_DIM to get array size.
>>>>>> - Since we are using a list of MASKs to validate ptype_mask
>return -EINVAL
>>>>>>     if any unknown mask is set.
>>>>>> - Rebase to TOT.
>>>>>>
>>>>>> v10 Changes:
>>>>>> -----------
>>>>>> - Modify ptype_mask validation in
>set_supported_ptypes.(Andrew)
>>>>>>
>>>>>> v9 Changes:
>>>>>> ----------
>>>>>> - Add ptype_mask validation in set_supported_ptypes.(Andrew)
>>>>>> - Make description more verbose.
>>>>>>
>>>>>> v8 Changes:
>>>>>> ----------
>>>>>> - Make description more verbose.
>>>>>> - Set RTE_PTYPE_UNKNOWN in set_ptypes array when either
>get ot set ptypes
>>>>>>     is not supported by ethernet device.
>>>>>>
>>>>>> v7 Changes:
>>>>>> ----------
>>>>>> - Fix unused variable in net/octeontx2
>>>>>>
>>>>>> v6 Changes:
>>>>>> ----------
>>>>>> - Add additional checks for set supported ptypes.(Andrew)
>>>>>> - Clarify `rte_eth_dev_set_supported_ptypes` documentation.
>>>>>> - Remove DEV_RX_OFFLOAD_FLOW_MARK emulation from
>net/octeontx2.
>>>>>>
>>>>>> v5 Changes:
>>>>>> ----------
>>>>>> - Fix typos.
>>>>>>
>>>>>> v4 Changes:
>>>>>> ----------
>>>>>> - Set the last element in set_ptype array as
>RTE_PTYPE_UNKNOWN to mark the end
>>>>>>     of array.
>>>>>> - Fix invalid set ptype function call in examples.
>>>>>> - Remove setting rte_eth_dev_set_supported_ptypes to
>UNKNOWN in l3fwd-power.
>>>>>>
>>>>>> v3 Changes:
>>>>>> ----------
>>>>>> - Add missing release notes. (Andrew)
>>>>>> - Re-word various descriptions.
>>>>>> - Fix ptype set logic.
>>>>>>
>>>>>> v2 Changes:
>>>>>> ----------
>>>>>> - Update release notes. (Andrew)
>>>>>> - Redo commit logs. (Andrew)
>>>>>> - Disable ptype parsing for unsupported examples. (Jerin)
>>>>>> - Disable RSS write only in generic mode eventdev_pipeline.
>(Jerin)
>>>>>> - Modify set_supported_ptypes function to return successfuly
>set mask
>>>>>>     instead of failure.
>>>>>> - Dropped set_supported_ptypes to drivers by handling in library
>>>>>>     layer, interested PMD can add it in.
>>>>>>
>>>>>> Pavan Nikhilesh (7):
>>>>>>     ethdev: add set ptype function
>>>>>>     ethdev: add mbuf RSS update as an offload
>>>>>>     ethdev: add flow action type update as an offload
>>>>>>     drivers/net: update Rx RSS hash offload capabilities
>>>>>>     drivers/net: update Rx flow flag and mark capabilities
>>>>>>     examples/eventdev_pipeline: add new Rx RSS hash offload
>>>>>>     examples: disable Rx packet type parsing
>>>>> <...>
>>

Re: [dpdk-dev] [PATCH v12 0/7] ethdev: add new Rx offload flags

[Ferruh Yigit]

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 <pbhagavatula@marvell.com>
>>>>>
>>>>>    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.

> 
>>> 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.

> 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?

> 
>>>> 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.

> 
>> (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.

I am not so sure about FLOW_MARK one, it looks like it is duplicating the
rte_flow API.

> 
>>>>>    Add new packet type set function `rte_eth_dev_set_supported_ptypes`,
>>>>>    allows application to inform PMDs about the packet types it is interested
>>>>>    in. Based on ptypes requested by application PMDs can optimize the Rx path.
>>>> OK to the API, but why "Packet type parsing" feature updated to say it should
>>>> implement this API?
>>>> Is this API really required to say "Packet type parsing" supported by PMD?
>>> As I understand it is not strictly required, but related to the feature.
>> I am OK with "related", but it is documented as "implements", so doc says it is
>> required.
> 
> Agreed.
> 
>>>>>    For example, if a given PMD doesn't support any packet types that the
>>>>>    application is interested in then the application can disable[1] writes to
>>>>>    `mbuf.packet_type` done by the PMD and use a software ptype parser.
>>>>>         [1] rte_eth_dev_set_supported_ptypes(*port_id*, RTE_PTYPE_UNKNOWN,
>>>>> 					  NULL, 0);
>>>> And for the 7/7 patch, why we are updating all examples, is the API something do
>>>> we really need to call for any DPDK application? I am for leaving the default
>>>> behavior unless there is a very specific case for set or disable packet typing.
>>>> Instead implement a command in testpmd to test this feature.
>>> If an application does not use packet types provided in mbuf, it is
>>> better to inform PMD that the information is not required to allow PMD
>>> to do optimizations.
>>>
>> I can see disabling packet type detection may increase the performance but
>> sample applications are to demonstrate a specific feature, adding these kind of
>> APIs will pollute them.
>> 'skeleton' app that shows the most basic code for forwarding sample, why it is
>> now having "experimental" 'set_supported_ptypes()' API? Same for other. As said
>> before I think a testpmd command suits better here.
> 
> May be you're right and we should reconsider which applications
> are updated and which are ignored. I guess before the criteria
> was simple: don't use packet type information, say so to take
> benefits from all possible optimizations.
> 
>>> Yes, may be it would be better to have it as the
>>> default behaviour, but it would be behaviour change in comparison
>>> to previous DPDK releases and it is better to avoid it.
>> Sorry I missed why not calling this function cause behavior change? I think it
>> is other way around, no?
> 
> Just misunderstanding. What I was trying to say is that it could
> be more logical to have packet type parsing and delivery
> disabled by default  (as we have for all other offloads), but
> it would be behaviour change from application point of view.
> That's why it is necessary to disable explicitly.
> 
> Thanks,
> Andrew.
> 
>>> Thanks,
>>> Andrew.
>>>
>>>>> v12 Changes:
>>>>> -----------
>>>>> - Rebase onto next-net.
>>>>>
>>>>> v11 Changes:
>>>>> -----------
>>>>> - Use RTE_DIM to get array size.
>>>>> - Since we are using a list of MASKs to validate ptype_mask return -EINVAL
>>>>>     if any unknown mask is set.
>>>>> - Rebase to TOT.
>>>>>
>>>>> v10 Changes:
>>>>> -----------
>>>>> - Modify ptype_mask validation in set_supported_ptypes.(Andrew)
>>>>>
>>>>> v9 Changes:
>>>>> ----------
>>>>> - Add ptype_mask validation in set_supported_ptypes.(Andrew)
>>>>> - Make description more verbose.
>>>>>
>>>>> v8 Changes:
>>>>> ----------
>>>>> - Make description more verbose.
>>>>> - Set RTE_PTYPE_UNKNOWN in set_ptypes array when either get ot set ptypes
>>>>>     is not supported by ethernet device.
>>>>>
>>>>> v7 Changes:
>>>>> ----------
>>>>> - Fix unused variable in net/octeontx2
>>>>>
>>>>> v6 Changes:
>>>>> ----------
>>>>> - Add additional checks for set supported ptypes.(Andrew)
>>>>> - Clarify `rte_eth_dev_set_supported_ptypes` documentation.
>>>>> - Remove DEV_RX_OFFLOAD_FLOW_MARK emulation from net/octeontx2.
>>>>>
>>>>> v5 Changes:
>>>>> ----------
>>>>> - Fix typos.
>>>>>
>>>>> v4 Changes:
>>>>> ----------
>>>>> - Set the last element in set_ptype array as RTE_PTYPE_UNKNOWN to mark the end
>>>>>     of array.
>>>>> - Fix invalid set ptype function call in examples.
>>>>> - Remove setting rte_eth_dev_set_supported_ptypes to UNKNOWN in l3fwd-power.
>>>>>
>>>>> v3 Changes:
>>>>> ----------
>>>>> - Add missing release notes. (Andrew)
>>>>> - Re-word various descriptions.
>>>>> - Fix ptype set logic.
>>>>>
>>>>> v2 Changes:
>>>>> ----------
>>>>> - Update release notes. (Andrew)
>>>>> - Redo commit logs. (Andrew)
>>>>> - Disable ptype parsing for unsupported examples. (Jerin)
>>>>> - Disable RSS write only in generic mode eventdev_pipeline. (Jerin)
>>>>> - Modify set_supported_ptypes function to return successfuly set mask
>>>>>     instead of failure.
>>>>> - Dropped set_supported_ptypes to drivers by handling in library
>>>>>     layer, interested PMD can add it in.
>>>>>
>>>>> Pavan Nikhilesh (7):
>>>>>     ethdev: add set ptype function
>>>>>     ethdev: add mbuf RSS update as an offload
>>>>>     ethdev: add flow action type update as an offload
>>>>>     drivers/net: update Rx RSS hash offload capabilities
>>>>>     drivers/net: update Rx flow flag and mark capabilities
>>>>>     examples/eventdev_pipeline: add new Rx RSS hash offload
>>>>>     examples: disable Rx packet type parsing
>>>> <...>
> 

Re: [dpdk-dev] [PATCH v12 0/7] ethdev: add new Rx offload flags

[Andrew Rybchenko]

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 <pbhagavatula@marvell.com>
>>>>
>>>>    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.

> 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.

>> 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.
Also I worry that it could be not that trivial to do in all effected PMDs.

>>> 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.

> (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.
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.

> 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.

>>>>    Add new packet type set function `rte_eth_dev_set_supported_ptypes`,
>>>>    allows application to inform PMDs about the packet types it is interested
>>>>    in. Based on ptypes requested by application PMDs can optimize the Rx path.
>>> OK to the API, but why "Packet type parsing" feature updated to say it should
>>> implement this API?
>>> Is this API really required to say "Packet type parsing" supported by PMD?
>> As I understand it is not strictly required, but related to the feature.
> I am OK with "related", but it is documented as "implements", so doc says it is
> required.

Agreed.

>>>>    For example, if a given PMD doesn't support any packet types that the
>>>>    application is interested in then the application can disable[1] writes to
>>>>    `mbuf.packet_type` done by the PMD and use a software ptype parser.
>>>>         [1] rte_eth_dev_set_supported_ptypes(*port_id*, RTE_PTYPE_UNKNOWN,
>>>> 					  NULL, 0);
>>> And for the 7/7 patch, why we are updating all examples, is the API something do
>>> we really need to call for any DPDK application? I am for leaving the default
>>> behavior unless there is a very specific case for set or disable packet typing.
>>> Instead implement a command in testpmd to test this feature.
>> If an application does not use packet types provided in mbuf, it is
>> better to inform PMD that the information is not required to allow PMD
>> to do optimizations.
>>
> I can see disabling packet type detection may increase the performance but
> sample applications are to demonstrate a specific feature, adding these kind of
> APIs will pollute them.
> 'skeleton' app that shows the most basic code for forwarding sample, why it is
> now having "experimental" 'set_supported_ptypes()' API? Same for other. As said
> before I think a testpmd command suits better here.

May be you're right and we should reconsider which applications
are updated and which are ignored. I guess before the criteria
was simple: don't use packet type information, say so to take
benefits from all possible optimizations.

>> Yes, may be it would be better to have it as the
>> default behaviour, but it would be behaviour change in comparison
>> to previous DPDK releases and it is better to avoid it.
> Sorry I missed why not calling this function cause behavior change? I think it
> is other way around, no?

Just misunderstanding. What I was trying to say is that it could
be more logical to have packet type parsing and delivery
disabled by default  (as we have for all other offloads), but
it would be behaviour change from application point of view.
That's why it is necessary to disable explicitly.

Thanks,
Andrew.

>> Thanks,
>> Andrew.
>>
>>>> v12 Changes:
>>>> -----------
>>>> - Rebase onto next-net.
>>>>
>>>> v11 Changes:
>>>> -----------
>>>> - Use RTE_DIM to get array size.
>>>> - Since we are using a list of MASKs to validate ptype_mask return -EINVAL
>>>>     if any unknown mask is set.
>>>> - Rebase to TOT.
>>>>
>>>> v10 Changes:
>>>> -----------
>>>> - Modify ptype_mask validation in set_supported_ptypes.(Andrew)
>>>>
>>>> v9 Changes:
>>>> ----------
>>>> - Add ptype_mask validation in set_supported_ptypes.(Andrew)
>>>> - Make description more verbose.
>>>>
>>>> v8 Changes:
>>>> ----------
>>>> - Make description more verbose.
>>>> - Set RTE_PTYPE_UNKNOWN in set_ptypes array when either get ot set ptypes
>>>>     is not supported by ethernet device.
>>>>
>>>> v7 Changes:
>>>> ----------
>>>> - Fix unused variable in net/octeontx2
>>>>
>>>> v6 Changes:
>>>> ----------
>>>> - Add additional checks for set supported ptypes.(Andrew)
>>>> - Clarify `rte_eth_dev_set_supported_ptypes` documentation.
>>>> - Remove DEV_RX_OFFLOAD_FLOW_MARK emulation from net/octeontx2.
>>>>
>>>> v5 Changes:
>>>> ----------
>>>> - Fix typos.
>>>>
>>>> v4 Changes:
>>>> ----------
>>>> - Set the last element in set_ptype array as RTE_PTYPE_UNKNOWN to mark the end
>>>>     of array.
>>>> - Fix invalid set ptype function call in examples.
>>>> - Remove setting rte_eth_dev_set_supported_ptypes to UNKNOWN in l3fwd-power.
>>>>
>>>> v3 Changes:
>>>> ----------
>>>> - Add missing release notes. (Andrew)
>>>> - Re-word various descriptions.
>>>> - Fix ptype set logic.
>>>>
>>>> v2 Changes:
>>>> ----------
>>>> - Update release notes. (Andrew)
>>>> - Redo commit logs. (Andrew)
>>>> - Disable ptype parsing for unsupported examples. (Jerin)
>>>> - Disable RSS write only in generic mode eventdev_pipeline. (Jerin)
>>>> - Modify set_supported_ptypes function to return successfuly set mask
>>>>     instead of failure.
>>>> - Dropped set_supported_ptypes to drivers by handling in library
>>>>     layer, interested PMD can add it in.
>>>>
>>>> Pavan Nikhilesh (7):
>>>>     ethdev: add set ptype function
>>>>     ethdev: add mbuf RSS update as an offload
>>>>     ethdev: add flow action type update as an offload
>>>>     drivers/net: update Rx RSS hash offload capabilities
>>>>     drivers/net: update Rx flow flag and mark capabilities
>>>>     examples/eventdev_pipeline: add new Rx RSS hash offload
>>>>     examples: disable Rx packet type parsing
>>> <...>

Re: [dpdk-dev] [PATCH v12 0/7] ethdev: add new Rx offload flags

[Ferruh Yigit]

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 <pbhagavatula@marvell.com>
>>>
>>>   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.
But when user want to configure this offload by setting or unsetting in offload
config, driver just ignores it.

> 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.

> 
>> 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.

(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 ???)

> 
>> 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?

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.

> 
>>>   Add new packet type set function `rte_eth_dev_set_supported_ptypes`,
>>>   allows application to inform PMDs about the packet types it is interested
>>>   in. Based on ptypes requested by application PMDs can optimize the Rx path.
>> OK to the API, but why "Packet type parsing" feature updated to say it should
>> implement this API?
>> Is this API really required to say "Packet type parsing" supported by PMD?
> 
> As I understand it is not strictly required, but related to the feature.

I am OK with "related", but it is documented as "implements", so doc says it is
required.

> 
>>>   For example, if a given PMD doesn't support any packet types that the
>>>   application is interested in then the application can disable[1] writes to
>>>   `mbuf.packet_type` done by the PMD and use a software ptype parser.
>>>        [1] rte_eth_dev_set_supported_ptypes(*port_id*, RTE_PTYPE_UNKNOWN,
>>> 					  NULL, 0);
>> And for the 7/7 patch, why we are updating all examples, is the API something do
>> we really need to call for any DPDK application? I am for leaving the default
>> behavior unless there is a very specific case for set or disable packet typing.
>> Instead implement a command in testpmd to test this feature.
> 
> If an application does not use packet types provided in mbuf, it is
> better to inform PMD that the information is not required to allow PMD
> to do optimizations. 
> 

I can see disabling packet type detection may increase the performance but
sample applications are to demonstrate a specific feature, adding these kind of
APIs will pollute them.
'skeleton' app that shows the most basic code for forwarding sample, why it is
now having "experimental" 'set_supported_ptypes()' API? Same for other. As said
before I think a testpmd command suits better here.


> Yes, may be it would be better to have it as the
> default behaviour, but it would be behaviour change in comparison
> to previous DPDK releases and it is better to avoid it.

Sorry I missed why not calling this function cause behavior change? I think it
is other way around, no?


> Thanks,
> Andrew.
> 
>>> v12 Changes:
>>> -----------
>>> - Rebase onto next-net.
>>>
>>> v11 Changes:
>>> -----------
>>> - Use RTE_DIM to get array size.
>>> - Since we are using a list of MASKs to validate ptype_mask return -EINVAL
>>>    if any unknown mask is set.
>>> - Rebase to TOT.
>>>
>>> v10 Changes:
>>> -----------
>>> - Modify ptype_mask validation in set_supported_ptypes.(Andrew)
>>>
>>> v9 Changes:
>>> ----------
>>> - Add ptype_mask validation in set_supported_ptypes.(Andrew)
>>> - Make description more verbose.
>>>
>>> v8 Changes:
>>> ----------
>>> - Make description more verbose.
>>> - Set RTE_PTYPE_UNKNOWN in set_ptypes array when either get ot set ptypes
>>>    is not supported by ethernet device.
>>>
>>> v7 Changes:
>>> ----------
>>> - Fix unused variable in net/octeontx2
>>>
>>> v6 Changes:
>>> ----------
>>> - Add additional checks for set supported ptypes.(Andrew)
>>> - Clarify `rte_eth_dev_set_supported_ptypes` documentation.
>>> - Remove DEV_RX_OFFLOAD_FLOW_MARK emulation from net/octeontx2.
>>>
>>> v5 Changes:
>>> ----------
>>> - Fix typos.
>>>
>>> v4 Changes:
>>> ----------
>>> - Set the last element in set_ptype array as RTE_PTYPE_UNKNOWN to mark the end
>>>    of array.
>>> - Fix invalid set ptype function call in examples.
>>> - Remove setting rte_eth_dev_set_supported_ptypes to UNKNOWN in l3fwd-power.
>>>
>>> v3 Changes:
>>> ----------
>>> - Add missing release notes. (Andrew)
>>> - Re-word various descriptions.
>>> - Fix ptype set logic.
>>>
>>> v2 Changes:
>>> ----------
>>> - Update release notes. (Andrew)
>>> - Redo commit logs. (Andrew)
>>> - Disable ptype parsing for unsupported examples. (Jerin)
>>> - Disable RSS write only in generic mode eventdev_pipeline. (Jerin)
>>> - Modify set_supported_ptypes function to return successfuly set mask
>>>    instead of failure.
>>> - Dropped set_supported_ptypes to drivers by handling in library
>>>    layer, interested PMD can add it in.
>>>
>>> Pavan Nikhilesh (7):
>>>    ethdev: add set ptype function
>>>    ethdev: add mbuf RSS update as an offload
>>>    ethdev: add flow action type update as an offload
>>>    drivers/net: update Rx RSS hash offload capabilities
>>>    drivers/net: update Rx flow flag and mark capabilities
>>>    examples/eventdev_pipeline: add new Rx RSS hash offload
>>>    examples: disable Rx packet type parsing
>> <...>
> 

Re: [dpdk-dev] [PATCH v12 0/7] ethdev: add new Rx offload flags

[Andrew Rybchenko]

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 <pbhagavatula@marvell.com>
>>
>>   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.
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.

> 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.

> 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.

> 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.

>>   Add new packet type set function `rte_eth_dev_set_supported_ptypes`,
>>   allows application to inform PMDs about the packet types it is interested
>>   in. Based on ptypes requested by application PMDs can optimize the Rx path.
> OK to the API, but why "Packet type parsing" feature updated to say it should
> implement this API?
> Is this API really required to say "Packet type parsing" supported by PMD?

As I understand it is not strictly required, but related to the feature.

>>   For example, if a given PMD doesn't support any packet types that the
>>   application is interested in then the application can disable[1] writes to
>>   `mbuf.packet_type` done by the PMD and use a software ptype parser.
>>        [1] rte_eth_dev_set_supported_ptypes(*port_id*, RTE_PTYPE_UNKNOWN,
>> 					  NULL, 0);
> And for the 7/7 patch, why we are updating all examples, is the API something do
> we really need to call for any DPDK application? I am for leaving the default
> behavior unless there is a very specific case for set or disable packet typing.
> Instead implement a command in testpmd to test this feature.

If an application does not use packet types provided in mbuf, it is
better to inform PMD that the information is not required to allow PMD
to do optimizations. Yes, may be it would be better to have it as the
default behaviour, but it would be behaviour change in comparison
to previous DPDK releases and it is better to avoid it.

Thanks,
Andrew.

>> v12 Changes:
>> -----------
>> - Rebase onto next-net.
>>
>> v11 Changes:
>> -----------
>> - Use RTE_DIM to get array size.
>> - Since we are using a list of MASKs to validate ptype_mask return -EINVAL
>>    if any unknown mask is set.
>> - Rebase to TOT.
>>
>> v10 Changes:
>> -----------
>> - Modify ptype_mask validation in set_supported_ptypes.(Andrew)
>>
>> v9 Changes:
>> ----------
>> - Add ptype_mask validation in set_supported_ptypes.(Andrew)
>> - Make description more verbose.
>>
>> v8 Changes:
>> ----------
>> - Make description more verbose.
>> - Set RTE_PTYPE_UNKNOWN in set_ptypes array when either get ot set ptypes
>>    is not supported by ethernet device.
>>
>> v7 Changes:
>> ----------
>> - Fix unused variable in net/octeontx2
>>
>> v6 Changes:
>> ----------
>> - Add additional checks for set supported ptypes.(Andrew)
>> - Clarify `rte_eth_dev_set_supported_ptypes` documentation.
>> - Remove DEV_RX_OFFLOAD_FLOW_MARK emulation from net/octeontx2.
>>
>> v5 Changes:
>> ----------
>> - Fix typos.
>>
>> v4 Changes:
>> ----------
>> - Set the last element in set_ptype array as RTE_PTYPE_UNKNOWN to mark the end
>>    of array.
>> - Fix invalid set ptype function call in examples.
>> - Remove setting rte_eth_dev_set_supported_ptypes to UNKNOWN in l3fwd-power.
>>
>> v3 Changes:
>> ----------
>> - Add missing release notes. (Andrew)
>> - Re-word various descriptions.
>> - Fix ptype set logic.
>>
>> v2 Changes:
>> ----------
>> - Update release notes. (Andrew)
>> - Redo commit logs. (Andrew)
>> - Disable ptype parsing for unsupported examples. (Jerin)
>> - Disable RSS write only in generic mode eventdev_pipeline. (Jerin)
>> - Modify set_supported_ptypes function to return successfuly set mask
>>    instead of failure.
>> - Dropped set_supported_ptypes to drivers by handling in library
>>    layer, interested PMD can add it in.
>>
>> Pavan Nikhilesh (7):
>>    ethdev: add set ptype function
>>    ethdev: add mbuf RSS update as an offload
>>    ethdev: add flow action type update as an offload
>>    drivers/net: update Rx RSS hash offload capabilities
>>    drivers/net: update Rx flow flag and mark capabilities
>>    examples/eventdev_pipeline: add new Rx RSS hash offload
>>    examples: disable Rx packet type parsing
> <...>

Re: [dpdk-dev] [PATCH v12 0/7] ethdev: add new Rx offload flags

[Ferruh Yigit]

On 10/17/2019 1:02 PM, pbhagavatula@marvell.com wrote:
> From: Pavan Nikhilesh <pbhagavatula@marvell.com>
> 
>  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


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?

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?


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?


> 
>  Add new packet type set function `rte_eth_dev_set_supported_ptypes`,
>  allows application to inform PMDs about the packet types it is interested
>  in. Based on ptypes requested by application PMDs can optimize the Rx path.

OK to the API, but why "Packet type parsing" feature updated to say it should
implement this API?
Is this API really required to say "Packet type parsing" supported by PMD?

> 
>  For example, if a given PMD doesn't support any packet types that the
>  application is interested in then the application can disable[1] writes to
>  `mbuf.packet_type` done by the PMD and use a software ptype parser.
>       [1] rte_eth_dev_set_supported_ptypes(*port_id*, RTE_PTYPE_UNKNOWN,
> 					  NULL, 0);

And for the 7/7 patch, why we are updating all examples, is the API something do
we really need to call for any DPDK application? I am for leaving the default
behavior unless there is a very specific case for set or disable packet typing.
Instead implement a command in testpmd to test this feature.

> 
> v12 Changes:
> -----------
> - Rebase onto next-net.
> 
> v11 Changes:
> -----------
> - Use RTE_DIM to get array size.
> - Since we are using a list of MASKs to validate ptype_mask return -EINVAL
>   if any unknown mask is set.
> - Rebase to TOT.
> 
> v10 Changes:
> -----------
> - Modify ptype_mask validation in set_supported_ptypes.(Andrew)
> 
> v9 Changes:
> ----------
> - Add ptype_mask validation in set_supported_ptypes.(Andrew)
> - Make description more verbose.
> 
> v8 Changes:
> ----------
> - Make description more verbose.
> - Set RTE_PTYPE_UNKNOWN in set_ptypes array when either get ot set ptypes
>   is not supported by ethernet device.
> 
> v7 Changes:
> ----------
> - Fix unused variable in net/octeontx2
> 
> v6 Changes:
> ----------
> - Add additional checks for set supported ptypes.(Andrew)
> - Clarify `rte_eth_dev_set_supported_ptypes` documentation.
> - Remove DEV_RX_OFFLOAD_FLOW_MARK emulation from net/octeontx2.
> 
> v5 Changes:
> ----------
> - Fix typos.
> 
> v4 Changes:
> ----------
> - Set the last element in set_ptype array as RTE_PTYPE_UNKNOWN to mark the end
>   of array.
> - Fix invalid set ptype function call in examples.
> - Remove setting rte_eth_dev_set_supported_ptypes to UNKNOWN in l3fwd-power.
> 
> v3 Changes:
> ----------
> - Add missing release notes. (Andrew)
> - Re-word various descriptions.
> - Fix ptype set logic.
> 
> v2 Changes:
> ----------
> - Update release notes. (Andrew)
> - Redo commit logs. (Andrew)
> - Disable ptype parsing for unsupported examples. (Jerin)
> - Disable RSS write only in generic mode eventdev_pipeline. (Jerin)
> - Modify set_supported_ptypes function to return successfuly set mask
>   instead of failure.
> - Dropped set_supported_ptypes to drivers by handling in library
>   layer, interested PMD can add it in.
> 
> Pavan Nikhilesh (7):
>   ethdev: add set ptype function
>   ethdev: add mbuf RSS update as an offload
>   ethdev: add flow action type update as an offload
>   drivers/net: update Rx RSS hash offload capabilities
>   drivers/net: update Rx flow flag and mark capabilities
>   examples/eventdev_pipeline: add new Rx RSS hash offload
>   examples: disable Rx packet type parsing

<...>

[dpdk-dev] [PATCH v12 0/7] ethdev: add new Rx offload flags

[pbhagavatula]

From: Pavan Nikhilesh <pbhagavatula@marvell.com>

 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`.

 Add new packet type set function `rte_eth_dev_set_supported_ptypes`,
 allows application to inform PMDs about the packet types it is interested
 in. Based on ptypes requested by application PMDs can optimize the Rx path.

 For example, if a given PMD doesn't support any packet types that the
 application is interested in then the application can disable[1] writes to
 `mbuf.packet_type` done by the PMD and use a software ptype parser.
      [1] rte_eth_dev_set_supported_ptypes(*port_id*, RTE_PTYPE_UNKNOWN,
					  NULL, 0);

v12 Changes:
-----------
- Rebase onto next-net.

v11 Changes:
-----------
- Use RTE_DIM to get array size.
- Since we are using a list of MASKs to validate ptype_mask return -EINVAL
  if any unknown mask is set.
- Rebase to TOT.

v10 Changes:
-----------
- Modify ptype_mask validation in set_supported_ptypes.(Andrew)

v9 Changes:
----------
- Add ptype_mask validation in set_supported_ptypes.(Andrew)
- Make description more verbose.

v8 Changes:
----------
- Make description more verbose.
- Set RTE_PTYPE_UNKNOWN in set_ptypes array when either get ot set ptypes
  is not supported by ethernet device.

v7 Changes:
----------
- Fix unused variable in net/octeontx2

v6 Changes:
----------
- Add additional checks for set supported ptypes.(Andrew)
- Clarify `rte_eth_dev_set_supported_ptypes` documentation.
- Remove DEV_RX_OFFLOAD_FLOW_MARK emulation from net/octeontx2.

v5 Changes:
----------
- Fix typos.

v4 Changes:
----------
- Set the last element in set_ptype array as RTE_PTYPE_UNKNOWN to mark the end
  of array.
- Fix invalid set ptype function call in examples.
- Remove setting rte_eth_dev_set_supported_ptypes to UNKNOWN in l3fwd-power.

v3 Changes:
----------
- Add missing release notes. (Andrew)
- Re-word various descriptions.
- Fix ptype set logic.

v2 Changes:
----------
- Update release notes. (Andrew)
- Redo commit logs. (Andrew)
- Disable ptype parsing for unsupported examples. (Jerin)
- Disable RSS write only in generic mode eventdev_pipeline. (Jerin)
- Modify set_supported_ptypes function to return successfuly set mask
  instead of failure.
- Dropped set_supported_ptypes to drivers by handling in library
  layer, interested PMD can add it in.

Pavan Nikhilesh (7):
  ethdev: add set ptype function
  ethdev: add mbuf RSS update as an offload
  ethdev: add flow action type update as an offload
  drivers/net: update Rx RSS hash offload capabilities
  drivers/net: update Rx flow flag and mark capabilities
  examples/eventdev_pipeline: add new Rx RSS hash offload
  examples: disable Rx packet type parsing

 doc/guides/nics/features.rst                  |  22 ++-
 doc/guides/rel_notes/release_19_11.rst        |  24 ++++
 drivers/net/bnxt/bnxt_ethdev.c                |   4 +-
 drivers/net/cxgbe/cxgbe.h                     |   3 +-
 drivers/net/dpaa/dpaa_ethdev.c                |   3 +-
 drivers/net/dpaa2/dpaa2_ethdev.c              |   3 +-
 drivers/net/e1000/igb_rxtx.c                  |   3 +-
 drivers/net/enic/enic_res.c                   |   4 +-
 drivers/net/fm10k/fm10k_ethdev.c              |   3 +-
 drivers/net/hinic/hinic_pmd_ethdev.c          |   3 +-
 drivers/net/i40e/i40e_ethdev.c                |   4 +-
 drivers/net/iavf/iavf_ethdev.c                |   4 +-
 drivers/net/ice/ice_ethdev.c                  |   4 +-
 drivers/net/ixgbe/ixgbe_rxtx.c                |   4 +-
 drivers/net/liquidio/lio_ethdev.c             |   3 +-
 drivers/net/mlx4/mlx4_rxq.c                   |   3 +-
 drivers/net/mlx5/mlx5_rxq.c                   |   4 +-
 drivers/net/netvsc/hn_rndis.c                 |   3 +-
 drivers/net/nfp/nfp_net.c                     |   3 +-
 drivers/net/octeontx2/otx2_ethdev.c           |   3 +-
 drivers/net/octeontx2/otx2_ethdev.h           |  16 ++-
 drivers/net/octeontx2/otx2_flow.c             |   9 +-
 drivers/net/octeontx2/otx2_flow.h             |   1 -
 drivers/net/octeontx2/otx2_flow_parse.c       |   5 +-
 drivers/net/qede/qede_ethdev.c                |   3 +-
 drivers/net/sfc/sfc_ef10_essb_rx.c            |   3 +-
 drivers/net/sfc/sfc_ef10_rx.c                 |   3 +-
 drivers/net/sfc/sfc_rx.c                      |   3 +-
 drivers/net/thunderx/nicvf_ethdev.h           |   3 +-
 drivers/net/vmxnet3/vmxnet3_ethdev.c          |   3 +-
 examples/bbdev_app/main.c                     |   2 +
 examples/bond/main.c                          |   2 +
 examples/distributor/Makefile                 |   1 +
 examples/distributor/main.c                   |   1 +
 examples/distributor/meson.build              |   1 +
 examples/eventdev_pipeline/main.c             | 130 +----------------
 examples/eventdev_pipeline/meson.build        |   1 +
 .../pipeline_worker_generic.c                 | 132 ++++++++++++++++++
 .../eventdev_pipeline/pipeline_worker_tx.c    | 128 +++++++++++++++++
 examples/exception_path/Makefile              |   1 +
 examples/exception_path/main.c                |   1 +
 examples/exception_path/meson.build           |   1 +
 examples/flow_classify/flow_classify.c        |   1 +
 examples/flow_filtering/Makefile              |   1 +
 examples/flow_filtering/main.c                |   2 +
 examples/flow_filtering/meson.build           |   1 +
 examples/ip_pipeline/link.c                   |   1 +
 examples/ip_reassembly/Makefile               |   1 +
 examples/ip_reassembly/main.c                 |   2 +
 examples/ip_reassembly/meson.build            |   1 +
 examples/ipsec-secgw/ipsec-secgw.c            |   2 +
 examples/ipv4_multicast/Makefile              |   1 +
 examples/ipv4_multicast/main.c                |   3 +
 examples/ipv4_multicast/meson.build           |   1 +
 examples/kni/main.c                           |   1 +
 examples/l2fwd-cat/Makefile                   |   1 +
 examples/l2fwd-cat/l2fwd-cat.c                |   1 +
 examples/l2fwd-cat/meson.build                |   1 +
 examples/l2fwd-crypto/main.c                  |   2 +
 examples/l2fwd-jobstats/Makefile              |   1 +
 examples/l2fwd-jobstats/main.c                |   2 +
 examples/l2fwd-jobstats/meson.build           |   1 +
 examples/l2fwd-keepalive/Makefile             |   1 +
 examples/l2fwd-keepalive/main.c               |   2 +
 examples/l2fwd-keepalive/meson.build          |   1 +
 examples/l2fwd/Makefile                       |   1 +
 examples/l2fwd/main.c                         |   2 +
 examples/l2fwd/meson.build                    |   1 +
 examples/l3fwd-acl/Makefile                   |   1 +
 examples/l3fwd-acl/main.c                     |   2 +
 examples/l3fwd-acl/meson.build                |   1 +
 examples/l3fwd-vf/Makefile                    |   1 +
 examples/l3fwd-vf/main.c                      |   2 +
 examples/l3fwd-vf/meson.build                 |   1 +
 examples/link_status_interrupt/Makefile       |   1 +
 examples/link_status_interrupt/main.c         |   2 +
 examples/link_status_interrupt/meson.build    |   1 +
 examples/load_balancer/Makefile               |   1 +
 examples/load_balancer/init.c                 |   2 +
 examples/load_balancer/meson.build            |   1 +
 examples/packet_ordering/Makefile             |   1 +
 examples/packet_ordering/main.c               |   1 +
 examples/packet_ordering/meson.build          |   1 +
 examples/ptpclient/Makefile                   |   1 +
 examples/ptpclient/meson.build                |   1 +
 examples/ptpclient/ptpclient.c                |   1 +
 examples/qos_meter/Makefile                   |   1 +
 examples/qos_meter/main.c                     |   2 +
 examples/qos_meter/meson.build                |   1 +
 examples/qos_sched/Makefile                   |   1 +
 examples/qos_sched/init.c                     |   1 +
 examples/qos_sched/meson.build                |   1 +
 examples/quota_watermark/qw/Makefile          |   1 +
 examples/quota_watermark/qw/init.c            |   1 +
 examples/rxtx_callbacks/main.c                |   1 +
 examples/server_node_efd/server/Makefile      |   1 +
 examples/server_node_efd/server/init.c        |   1 +
 examples/skeleton/Makefile                    |   1 +
 examples/skeleton/basicfwd.c                  |   1 +
 examples/skeleton/meson.build                 |   1 +
 examples/tep_termination/Makefile             |   1 +
 examples/tep_termination/meson.build          |   1 +
 examples/tep_termination/vxlan_setup.c        |   1 +
 examples/vhost/Makefile                       |   1 +
 examples/vhost/main.c                         |   1 +
 examples/vm_power_manager/Makefile            |   1 +
 examples/vm_power_manager/main.c              |   1 +
 examples/vm_power_manager/meson.build         |   1 +
 examples/vmdq/Makefile                        |   1 +
 examples/vmdq/main.c                          |   1 +
 examples/vmdq/meson.build                     |   1 +
 examples/vmdq_dcb/Makefile                    |   1 +
 examples/vmdq_dcb/main.c                      |   1 +
 examples/vmdq_dcb/meson.build                 |   1 +
 lib/librte_ethdev/rte_ethdev.c                |  89 +++++++++++-
 lib/librte_ethdev/rte_ethdev.h                |  39 ++++++
 lib/librte_ethdev/rte_ethdev_core.h           |  19 +++
 lib/librte_ethdev/rte_ethdev_version.map      |   1 +
 lib/librte_ethdev/rte_flow.h                  |   6 +-
 119 files changed, 621 insertions(+), 176 deletions(-)

--
2.17.1