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

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

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