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

[Pavan Nikhilesh Bhagavatula <pbhagavatula@marvell.com>]

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