DPDK patches and discussions
 help / color / mirror / Atom feed
* Re: [dpdk-dev] [PATCH v12 0/7] ethdev: add new Rx offload flags
@ 2019-10-21 15:19 Pavan Nikhilesh Bhagavatula
  2019-10-21 15:34 ` Ferruh Yigit
  0 siblings, 1 reply; 14+ messages in thread
From: Pavan Nikhilesh Bhagavatula @ 2019-10-21 15:19 UTC (permalink / raw)
  To: Ferruh Yigit, Andrew Rybchenko, Jerin Jacob Kollanukkaran
  Cc: dev, Adrien Mazarguil, Thomas Monjalon, Xiaolong Ye, Bruce Richardson

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


^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [dpdk-dev] [PATCH v12 0/7] ethdev: add new Rx offload flags
@ 2019-10-24 10:34 Pavan Nikhilesh Bhagavatula
  0 siblings, 0 replies; 14+ messages in thread
From: Pavan Nikhilesh Bhagavatula @ 2019-10-24 10:34 UTC (permalink / raw)
  To: Ferruh Yigit, Andrew Rybchenko, Jerin Jacob Kollanukkaran
  Cc: dev, Adrien Mazarguil, Thomas Monjalon, Xiaolong Ye, Bruce Richardson

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


^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [dpdk-dev] [PATCH v12 0/7] ethdev: add new Rx offload flags
@ 2019-10-22 15:20 Pavan Nikhilesh Bhagavatula
  2019-10-24  9:47 ` Ferruh Yigit
  0 siblings, 1 reply; 14+ messages in thread
From: Pavan Nikhilesh Bhagavatula @ 2019-10-22 15:20 UTC (permalink / raw)
  To: Ferruh Yigit, Andrew Rybchenko, Jerin Jacob Kollanukkaran
  Cc: dev, Adrien Mazarguil, Thomas Monjalon, Xiaolong Ye, Bruce Richardson

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


^ permalink raw reply	[flat|nested] 14+ messages in thread
* [dpdk-dev]  [PATCH v11 0/7] ethdev: add new Rx offload flags
@ 2019-10-10 10:51 pbhagavatula
  2019-10-17 12:02 ` [dpdk-dev] [PATCH v12 " pbhagavatula
  0 siblings, 1 reply; 14+ messages in thread
From: pbhagavatula @ 2019-10-10 10:51 UTC (permalink / raw)
  To: arybchenko, jerinj; +Cc: dev, Pavan Nikhilesh

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);

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                     |   1 +
 examples/bond/main.c                          |   2 +
 examples/distributor/Makefile                 |   1 +
 examples/distributor/main.c                   |   1 +
 examples/distributor/meson.build              |   1 +
 examples/eventdev_pipeline/main.c             | 122 +----------------
 examples/eventdev_pipeline/meson.build        |   1 +
 .../pipeline_worker_generic.c                 | 124 ++++++++++++++++++
 .../eventdev_pipeline/pipeline_worker_tx.c    | 120 +++++++++++++++++
 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                |   1 +
 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                |   2 +
 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                |  88 +++++++++++++
 lib/librte_ethdev/rte_ethdev.h                |  39 ++++++
 lib/librte_ethdev/rte_ethdev_core.h           |  19 +++
 lib/librte_ethdev/rte_ethdev_version.map      |   3 +
 lib/librte_ethdev/rte_flow.h                  |   6 +-
 119 files changed, 604 insertions(+), 167 deletions(-)

--
2.17.1


^ permalink raw reply	[flat|nested] 14+ messages in thread
end of thread, other threads:[~2019-10-24 10:34 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-21 15:19 [dpdk-dev] [PATCH v12 0/7] ethdev: add new Rx offload flags Pavan Nikhilesh Bhagavatula
2019-10-21 15:34 ` Ferruh Yigit
2019-10-22 10:16   ` Andrew Rybchenko
2019-10-22 14:20     ` Ferruh Yigit
  -- strict thread matches above, loose matches on Subject: below --
2019-10-24 10:34 Pavan Nikhilesh Bhagavatula
2019-10-22 15:20 Pavan Nikhilesh Bhagavatula
2019-10-24  9:47 ` Ferruh Yigit
2019-10-24 10:24   ` Ferruh Yigit
2019-10-10 10:51 [dpdk-dev] [PATCH v11 " pbhagavatula
2019-10-17 12:02 ` [dpdk-dev] [PATCH v12 " pbhagavatula
2019-10-17 17:43   ` Ferruh Yigit
2019-10-18  7:32     ` Andrew Rybchenko
2019-10-18  9:42       ` Ferruh Yigit
2019-10-18 10:31         ` Andrew Rybchenko
2019-10-21 15:06           ` Ferruh Yigit

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).