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.yi...@intel.com>
Sent: Monday, October 21, 2019 8:37 PM
To: Andrew Rybchenko <arybche...@solarflare.com>; Pavan Nikhilesh
Bhagavatula <pbhagavat...@marvell.com>; Jerin Jacob Kollanukkaran
<jer...@marvell.com>
Cc: dev@dpdk.org; Adrien Mazarguil <adrien.mazarg...@6wind.com>;
Thomas Monjalon <tho...@monjalon.net>; Xiaolong Ye
<xiaolong...@intel.com>; Bruce Richardson
<bruce.richard...@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, pbhagavat...@marvell.com wrote:
From: Pavan Nikhilesh <pbhagavat...@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]
