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

[Ferruh Yigit <ferruh.yigit@intel.com>]

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