在 2020/10/21 17:38, Ferruh Yigit 写道:
On 10/21/2020 9:19 AM, oulijun wrote:
在 2020/10/20 22:34, Ferruh Yigit 写道:
On 10/20/2020 2:35 PM, oulijun wrote:
在 2020/10/20 18:02, Ferruh Yigit 写道:
On 10/20/2020 10:00 AM, oulijun wrote:
在 2020/10/16 18:57, Ferruh Yigit 写道:
On 10/16/2020 11:04 AM, oulijun wrote:
在 2020/10/16 7:53, Ferruh Yigit 写道:
On 10/16/2020 12:21 AM, Ophir Munk wrote:
From: Ferruh Yigit <ferruh.yi...@intel.com>
On 10/15/2020 1:41 PM, Lijun Ou wrote:
When start the testpmd, the pmd driver initializes the RSS
configuration. Generally, the recommended RSS hash key is
used as
the default key in the driver. In addition, the default
key is
different from the default RSS flow in testpmd without
specifying
RSS hash key. So. if you do not specify the RSS key when
creating
an RSS rule, the testpmd uses the default key as the
default RSS
key of the RSS rule. As a result, you may mistakenly
consider that
the RSS key in use is the valid default key of the NIC,
actually,
the key and the valid default key of the NIC are two values.
Consider the follow usage with testpmd:
1. first, startup testpmd:
testpmd> show port 0 rss-hash key
RSS functions:
all ipv4-frag ipv4-other ipv6-frag ipv6-other ip RSS
key:
-
6D5A56DA255B0EC24167253D43A38FB0D0CA2BCBAE7B30B477CB2DA3803
0F
-20C6A42B73BBEAC01FA
2. create a rss rule
testpmd> flow create 0 ingress pattern eth / ipv4 / udp /
end
testpmd> actions rss \
types ipv4-udp end queues end / end
3. show rss-hash key
testpmd> show port 0 rss-hash key
RSS functions:
all ipv4-udp udp
RSS key:
-
74657374706D6427732064656661756C74205253532068617368206B65792C
206F
-76657272696465
In order to solve the above problems, it use the NIC
valid default
RSS key instead of the testpmd dummy RSS key in the flow
configuration when the RSS key is not specified in the
flow rule.
If the NIC RSS key is invalid, it will use testpmd dummy
RSS key as the
default key.
Fixes: ac8d22de2394 ("ethdev: flatten RSS configuration
in flow
API")
Cc: sta...@dpdk.org
Signed-off-by: Lijun Ou <ouli...@huawei.com>
Reviewed-by: Phil Yang <phil.y...@arm.com>
---
V4->V5:
-rewrite the commit log
-add reviewed-by
Hi Lijun,
There were multiple other comments, it seems they are not
addressed
but only updated the commit log, can you please check
comments to
prev versions.
Before going into the details, my question was what
happens if
default key not provided at all?
It seems this has been already tried by Ophir [1], later
reverted
back [2] bringing the initial issue back.
According commit, the reason of revert is to support
following
command:
"flow create 0 <pattern> actions rss queues 0 1 end
key_len 40 / end"
@Ophir, @Lijun,
Can we ignore the 'key_len' if the 'key' is not supported
and solve
current issue as initially intended ([1])?
Hi, Ferruh
I have discussed with Phil Yang about the problem in
[1]. I think
there may be other problems with the idea and there is no
better
solution. and we need to remove key_len definition from
rte_rss_conf
structure. They don't have a plan. And [1] was eventually
reverted.
Why ignoring 'key_len' (set it to zero) when there is no 'key'
provided doesn't work?
What do you think [1] + following update, will it work?
diff --git a/lib/librte_ethdev/rte_flow.c
b/lib/librte_ethdev/rte_flow.c
index ee4f3464fe..e7789c87b3 100644
--- a/lib/librte_ethdev/rte_flow.c
+++ b/lib/librte_ethdev/rte_flow.c
@@ -535,7 +535,7 @@ rte_flow_conv_action_conf(void *buf,
const size_t
size,
}),
size > sizeof(*dst.rss) ?
sizeof(*dst.rss) : size);
off = sizeof(*dst.rss);
- if (src.rss->key_len) {
+ if (src.rss->key_len && src.rss->key) {
off = RTE_ALIGN_CEIL(off,
sizeof(*dst.rss->key));
tmp = sizeof(*src.rss->key) *
src.rss->key_len;
if (size >= off + tmp)
Ferruh, your suggestion ([1] + update) looks correct. I also
verified it on mlx5 PMD.
Advantage: it's a generic fix for all dpdk applications using
rte_flows (not just testpmd).
It reduces code.
With this fix the responsibility of handling key==NULL and/or
len==0 is moved to the PMDs (which is good).
With regard to Lijun patch - I liked the approach of
overriding the default testpmd key with the default PMD key.
But it only addresses testpmd. More code was added.
It seems OK to call rte_eth_dev_rss_hash_conf_get() as part of
parsing RSS, but it would feel more confident if we could
confirm it for all the PMDs (by testing) or at least review
the PMDs rss_hash_conf_get() implementations.
Lijun's idea can work. There was a problem in implementation
related to the key size assumption, which can be fixed.
Even it is fixed, when user gives a rss rule without a key, we
are getting key from device and feeding same key back to
device, this is unnecessary I think.
When user didn't provide a key, rss rule shouldn't touch the
key at all.
Do you mean that the driver should not configure the key to the
hardware when the RSS key is not specified?
We are talking about 'key' in RSS rte flow rule, not generally
configuring the device for RSS.
Yes, I know. However, I am talking about some implementations. The
configuration interface may be the same as that for configuring RSS
parameters of the device in general mode.
If a specific rss rte flow rule, lets say to filter some defined
packets to some specific queues, don't have 'key' as a part of
rule, I am suggesting not touch 'key' configuration of the device.
Yes, I agree. I think your point of view is very reasonable and a
more
appropriate implementation. However, for the sake of simplicity and
other considerations in the architecture design of some devices, the
hardware may support reasonable hybrid configuration for paramter
configuration in the RSS config, rather than independent
configuration
for hw, that is, maybe touch A of rss configuration of the device and
must touch B of rss configuration. As a result, If the preceding
scenario occurs, it is reasonable to configure an RSS config that
does not change and specified.What do you think?
If so, we cannot identify when the user does not specify the RSS
key to determine whether to configure the key.
I think we can. If the rule has 'key' attribute, driver can use
that key to program the device, if 'key' attribute is missing,
don't do anything related to the rss key.
Yes, if 'key' attribute is missing, don't do anything related to
the rss key. It's more reasonable.
But, If he can't do that, I think it is reasonable to configure
the default key that already exists on the device. The only
disadvantage is wasteful.
In hardware design, most vendors do not configure keys for
hardware independently, which may be associated with other RSS
config parameters.
I think it would be reasonable for us to reconfigure the RSS key
with the default value configured and valid in the hardware as
the default value if the user does not specify the RSS key.
There are already APIs to update the RSS configuration, like RSS
key, hash functions etc.. They are independent from the rte flow
RSS rule.
Application should configure RSS according needs using those APIs.
Yes, should do this. Are there any unreasonable users who simply
do not know these APIs or do not want to use them and want to
configure some parameters through the rte flow RSS rule without
changing other parameters?
The question here is about each RSS rte flow rule that can update
the key. Am I missing something?
I don't think you've misunderstood the general situation.
Even if the RSS key is not specified in a rule, the driver uses
the default key value to re-touch the device.
Complication was when user provides key_len without a key, I
think both ignoring or returning error in this case is OK.
I agree. However, I think it is meaningless to expose the RSS
key length to users. Do you consider deleting the RSS key length
directly?
Isn't both 'key' and 'key_len' needed to program the RSS key? Can
the driver know the size of the 'key' without 'key_len'?
And driver should verify these provided values.
I know and agree.It is only during the analysis of the [1] scheme,
from the time it was proposed to the time it was withdrawn, that I
found some guys in the community questioned the significance of
key_len.
Back to the subject, what is your plan for the solution in the patch?
I think, Ophir's old reverted patch [1] +
'rte_flow_conv_action_conf()' update I proposed above can work. Is
this working for you? If so can you please send a patch?
[1] https://git.dpdk.org/dpdk/commit/?id=a4391f8bae8
.
Hi, Ferruh
I've just used [1] + 'rte_flow_conv_action_conf()' for testing
based on the Kunpeng920 NIC plation and use hns3 pmd driver. it can
be resolve the RSS key question.
testpmd> show port 0 rss-hash key
RSS functions:
all ipv4-frag ipv4-other ipv6-frag ipv6-other ip
RSS key:
6D5A56DA255B0EC24167253D43A38FB0D0CA2BCBAE7B30B477CB2DA38030F20C6A42B73BBEAC01FA
testpmd> flow create 0 ingress pattern end actions rss func
symmetric_toeplitz types all end / end
0000:7d:00.0 hns3_config_rss_filter(): modified RSS types based on
hardware support, requested:7f83fffc configured:3ef8
0000:7d:00.0 hns3_parse_rss_key(): no valid RSS key config, using
default
Flow rule #0 created
testpmd> show port 0 rss-hash key
RSS functions:
all ipv4-frag ipv4-tcp ipv4-udp ipv4-sctp ipv4-other ipv6-frag
ipv6-tcp ipv6-udp ipv6-sctp ipv6-other ip udp tcp sctp
RSS key:
6D5A56DA255B0EC24167253D43A38FB0D0CA2BCBAE7B30B477CB2DA38030F20C6A42B73BBEAC01FA
However, I also tested the case. I am not specified the RSS key and
specified the key_len when create a RSS flow rule, it can create
success.
testpmd> flow create 0 ingress pattern end actions rss queues 0 1
end key_len 40 / end
0000:7d:00.0 hns3_config_rss_filter(): modified RSS types based on
hardware support, requested:a38c configured:2288
0000:7d:00.0 hns3_parse_rss_key(): no valid RSS key config, using
default
Flow rule #0 created
testpmd> show port 0 rss-hash key
RSS functions:
all ipv4-frag ipv4-other ipv6-frag ipv6-other ip
RSS key:
6D5A56DA255B0EC24167253D43A38FB0D0CA2BCBAE7B30B477CB2DA38030F20C6A42B73BBEAC01FA
So are you happy with this solution?
.
Yes, thanks.
cool, will you able to send a new version? Thanks.
.
Yes
