On 04/09/2018 05:42 PM, Adrien Mazarguil wrote:
On Sat, Apr 07, 2018 at 12:05:51PM +0300, Andrew Rybchenko wrote:
On 04/06/2018 04:25 PM, Adrien Mazarguil wrote:
Since its inception, the rte_flow RSS action has been relying in part on
external struct rte_eth_rss_conf for compatibility with the legacy RSS API.
This structure lacks parameters such as the hash algorithm to use, and more
recently, a method to tell which layer RSS should be performed on [1].
Given struct rte_eth_rss_conf will never be flexible enough to represent a
complete RSS configuration (e.g. RETA table), this patch supersedes it by
extending the rte_flow RSS action directly.
A subsequent patch will add a field to use a non-default RSS hash
algorithm. To that end, a field named "types" replaces the field formerly
known as "rss_hf" and standing for "RSS hash functions" as it was
confusing. Actual RSS hash function types are defined by enum
rte_eth_hash_function.
This patch updates all PMDs and example applications accordingly.
It breaks ABI compatibility for the following public functions:
- rte_flow_copy()
- rte_flow_create()
- rte_flow_query()
- rte_flow_validate()
[1] commit 676b605182a5 ("doc: announce ethdev API change for RSS
configuration")
Signed-off-by: Adrien Mazarguil <adrien.mazarg...@6wind.com>
Cc: Xueming Li <xuemi...@mellanox.com>
Cc: Ferruh Yigit <ferruh.yi...@intel.com>
Cc: Thomas Monjalon <tho...@monjalon.net>
Cc: Wenzhuo Lu <wenzhuo...@intel.com>
Cc: Jingjing Wu <jingjing...@intel.com>
Cc: Beilei Xing <beilei.x...@intel.com>
Cc: Qi Zhang <qi.z.zh...@intel.com>
Cc: Konstantin Ananyev <konstantin.anan...@intel.com>
Cc: Nelio Laranjeiro <nelio.laranje...@6wind.com>
Cc: Yongseok Koh <ys...@mellanox.com>
Cc: Andrew Rybchenko <arybche...@solarflare.com>
Cc: Pascal Mazon <pascal.ma...@6wind.com>
Cc: Radu Nicolau <radu.nico...@intel.com>
Cc: Akhil Goyal <akhil.go...@nxp.com>
---
app/test-pmd/cmdline_flow.c | 59 +++++-----
app/test-pmd/config.c | 39 +++----
doc/guides/prog_guide/rte_flow.rst | 22 ++--
drivers/net/e1000/e1000_ethdev.h | 13 ++-
drivers/net/e1000/igb_ethdev.c | 4 +-
drivers/net/e1000/igb_flow.c | 31 ++---
drivers/net/e1000/igb_rxtx.c | 51 +++++++--
drivers/net/i40e/i40e_ethdev.c | 53 +++++++--
drivers/net/i40e/i40e_ethdev.h | 15 ++-
drivers/net/i40e/i40e_flow.c | 47 ++++----
drivers/net/ixgbe/ixgbe_ethdev.c | 4 +-
drivers/net/ixgbe/ixgbe_ethdev.h | 13 ++-
drivers/net/ixgbe/ixgbe_flow.c | 30 ++---
drivers/net/ixgbe/ixgbe_rxtx.c | 51 +++++++--
drivers/net/mlx4/mlx4.c | 2 +-
drivers/net/mlx4/mlx4_flow.c | 61 +++++-----
drivers/net/mlx4/mlx4_flow.h | 2 +-
drivers/net/mlx4/mlx4_rxq.c | 2 +-
drivers/net/mlx4/mlx4_rxtx.h | 2 +-
drivers/net/mlx5/mlx5_flow.c | 193 +++++++++++++++-----------------
drivers/net/mlx5/mlx5_rxq.c | 22 ++--
drivers/net/mlx5/mlx5_rxtx.h | 26 +++--
drivers/net/sfc/sfc_flow.c | 21 ++--
drivers/net/tap/tap_flow.c | 8 +-
examples/ipsec-secgw/ipsec.c | 10 +-
lib/librte_ether/rte_flow.c | 39 +++----
lib/librte_ether/rte_flow.h | 6 +-
27 files changed, 473 insertions(+), 353 deletions(-)
<...>
diff --git a/drivers/net/sfc/sfc_flow.c b/drivers/net/sfc/sfc_flow.c
index 056405515..1a2c0299c 100644
--- a/drivers/net/sfc/sfc_flow.c
+++ b/drivers/net/sfc/sfc_flow.c
@@ -1234,13 +1234,11 @@ sfc_flow_parse_rss(struct sfc_adapter *sa,
struct sfc_rxq *rxq;
unsigned int rxq_hw_index_min;
unsigned int rxq_hw_index_max;
- const struct rte_eth_rss_conf *rss_conf = rss->rss_conf;
- uint64_t rss_hf;
- uint8_t *rss_key = NULL;
+ const uint8_t *rss_key;
struct sfc_flow_rss *sfc_rss_conf = &flow->rss_conf;
unsigned int i;
- if (rss->num == 0)
+ if (rss->queue_num == 0)
return -EINVAL;
rxq_sw_index = sa->rxq_count - 1;
@@ -1248,7 +1246,7 @@ sfc_flow_parse_rss(struct sfc_adapter *sa,
rxq_hw_index_min = rxq->hw_index;
rxq_hw_index_max = 0;
- for (i = 0; i < rss->num; ++i) {
+ for (i = 0; i < rss->queue_num; ++i) {
rxq_sw_index = rss->queue[i];
if (rxq_sw_index >= sa->rxq_count)
@@ -1263,15 +1261,14 @@ sfc_flow_parse_rss(struct sfc_adapter *sa,
rxq_hw_index_max = rxq->hw_index;
}
- rss_hf = (rss_conf != NULL) ? rss_conf->rss_hf : SFC_RSS_OFFLOADS;
Here we had a fallback to default rss_hf (now types) if rss_conf is
unspecified.
Thing is, rss_action->conf was never supposed to be NULL in the first
place. Crashing on a NULL configuration has always been fine, but until
recently prevented validation with testpmd's broken implementation. This
problem was addressed in a prior series [1][2][3].
Since a value is now always provided, no need for a fallback.
testpmd is not the only application. But in any case I agree that it was
possible have rss_hf==0 before. So, no big changes.
[1] "app/testpmd: fix lack of flow action configuration"
http://dpdk.org/ml/archives/dev/2018-April/095280.html
[2] "app/testpmd: fix RSS flow action configuration"
http://dpdk.org/ml/archives/dev/2018-April/095281.html
[3] "app/testpmd: fix missing RSS fields in flow action"
http://dpdk.org/ml/archives/dev/2018-April/095282.html
- if ((rss_hf & ~SFC_RSS_OFFLOADS) != 0)
+ if ((rss->types & ~SFC_RSS_OFFLOADS) != 0)
return -EINVAL;
- if (rss_conf != NULL) {
- if (rss_conf->rss_key_len != sizeof(sa->rss_key))
+ if (rss->key_len) {
+ if (rss->key_len != sizeof(sa->rss_key))
return -EINVAL;
- rss_key = rss_conf->rss_key;
+ rss_key = rss->key;
} else {
rss_key = sa->rss_key;
}
@@ -1280,11 +1277,11 @@ sfc_flow_parse_rss(struct sfc_adapter *sa,
sfc_rss_conf->rxq_hw_index_min = rxq_hw_index_min;
sfc_rss_conf->rxq_hw_index_max = rxq_hw_index_max;
- sfc_rss_conf->rss_hash_types = sfc_rte_to_efx_hash_type(rss_hf);
+ sfc_rss_conf->rss_hash_types = sfc_rte_to_efx_hash_type(rss->types);
Now types go directly to mapping function and unspecified types (0)
will result in 0 rss_hash_types. Of course, it is a question how to treat
types==0. It is possible to say that it no RSS, but it does not make sense.
So, real options are device defaults (regardless configured on device level)
or device config (rx_adv.conf.rss_conf.rss_hf). I would prefer the later.
Please, document the intended behaviour in rte_flow.rst.
Granted the existing documentation doesn't say much on that topic, but a 0
value for rss_hf does actually mean "no RSS" [4]:
"The *rss_hf* field of the *rss_conf* structure indicates the different
types of IPv4/IPv6 packets to which the RSS hashing must be applied.
Supplying an *rss_hf* equal to zero disables the RSS feature."
Now since this action doesn't use struct rte_eth_rss_conf anymore, we could
define 0 as a PMD-specific behavior, which could be no RSS. It would make
the API easier to use for applications that don't care about the RSS
capabilities of each underlying adapter, 0 would just work everywhere as a
safe default.
PMD-specific is fine with some limits. It should be either device RSS
config or
device defaults. I think it is bad idea to allow types=0 disable RSS as
an option
of the PMD-specific behaviour.
[4] https://dpdk.org/doc/api/structrte__eth__rss__conf.html
If the later is chosen, above we'll have a bug since fallback to fixed
default.
Just use sa->rss_hash_types as fallback. Something like:
if (rss->types)
sfc_rss_conf->rss_hash_types = sfc_rte_to_efx_hash_type(rss->types);
else
sfc_rss_conf->rss_hash_types =sa->rss_hash_types;
Looks like the previous code didn't provide a fallback when rss_hf was 0,
only when rss_conf itself was NULL. So this is not a new issue introduced by
this patch.
Yes, I agree.
I will update documentation to define 0 as described above for the
convenience of application writers and leave the existing code in place.
PMD maintainers will be free to enhance it as they wish later.
Just remember testpmd now always provides a default value for it after
querying the device [2].
Many thanks.
