Re: [RFC] random: use per lcore state
09/09/2023 07:45, Mattias Rönnblom пишет:
On 2023-09-09 02:13, Konstantin Ananyev wrote:
06/09/2023 21:02, Mattias Rönnblom пишет:
On 2023-09-06 19:20, Stephen Hemminger wrote:
Move the random number state into thread local storage.
Me and Morten discussed TLS versus other alternatives in some other
thread. The downside of TLS that Morten pointed out, from what I
recall, is that lazy initialization is *required* (since the number
of threads is open-ended), and the data ends up in non-huge page memory.
Hmm.. correct me if I am wrong, but with current implementation,
rand state is also in non-huge memory:
static struct rte_rand_state rand_states[RTE_MAX_LCORE + 1];
Yes. The current pattern is certainly not perfect.
It was also unclear to me what the memory footprint implications
would be,h would large per-lcore data structures be put in TLS. More
specifically, if they would be duplicated across all threads, even
non-lcore threads.
None of these issues affect rte_random.c's potential usage of TLS
(except lazy [re-]initialization makes things more complicated).
Preferably, there should be one pattern that is usable across all or
at least most DPDK modules requiring per-lcore state.
This has a several benefits.
- no false cache sharing from cpu prefetching
- fixes initialization of random state for non-DPDK threads
This seems like a non-reason to me. That bug is easily fixed, if it
isn't already.
- fixes unsafe usage of random state by non-DPDK threads
"Makes random number generation MT safe from all threads (including
unregistered non-EAL threads)."
With current API semantics you may still register an non-EAL thread,
to get MT safe access to this API, so I guess it's more about being
more convenient and less error prone, than anything else.
I understand that we never guaranteed MT safety for non-EAL threads here,
Registered non-EAL threads have a lcore id and thus may safely call
rte_rand().
I am aware about such ability, but for me register/unregister thread
just to call rte_rand() seems like way too much hassle.
Multiple unregistered non-EAL threads may not do so, in
parallel.
but as a user of rte_rand() - it would be much more convenient, if I
can use it
from any thread wthout worring is it a EAL thread or not.
Sure, especially if it comes for free. The for-free solution has yet to
reveal itself though.
About TlS usage and re-seeding - can we use some sort of middle-ground:
extend rte_rand_state with some gen-counter.
Make a 'master' copy of rte_rand_state that will be updated by
rte_srand(),
and TLS copies of rte_rand_state, so rte_rand() can fist compare
its gen-counter value with master copy to decide,
does it need to copy new state from master or not.
Calling threads shouldn't all produce the same sequence. That would be
silly and not very random. The generation number should be tied to the
seed.
Actually, yes you right, probably we don't need a master copy of
rte_rand_state itself.
It seems that just having a 'master' copy of 'seed' value, plus some
counter (to indicate that seed has been updated) is enough here.
The new MT safety guarantees should be in the API docs as well.
Yes, it is an extension to the current API, not a fix.
The initialization of random number state is done by the
lcore (lazy initialization).
Signed-off-by: Stephen Hemminger
---
lib/eal/common/rte_random.c | 38
+++--
1 file changed, 20 insertions(+), 18 deletions(-)
diff --git a/lib/eal/common/rte_random.c b/lib/eal/common/rte_random.c
index 53636331a27b..9657adf6ad3b 100644
--- a/lib/eal/common/rte_random.c
+++ b/lib/eal/common/rte_random.c
@@ -19,13 +19,14 @@ struct rte_rand_state {
uint64_t z3;
uint64_t z4;
uint64_t z5;
-} __rte_cache_aligned;
+ uint64_t seed;
+};
-/* One instance each for every lcore id-equipped thread, and one
- * additional instance to be shared by all others threads (i.e., all
- * unregistered non-EAL threads).
- */
-static struct rte_rand_state rand_states[RTE_MAX_LCORE + 1];
+/* Global random seed */
+static uint64_t rte_rand_seed;
+
+/* Per lcore random state. */
+static RTE_DEFINE_PER_LCORE(struct rte_rand_state, rte_rand_state);
static uint32_t
__rte_rand_lcg32(uint32_t *seed)
@@ -81,11 +82,7 @@ __rte_srand_lfsr258(uint64_t seed, struct
rte_rand_state *state)
void
rte_srand(uint64_t seed)
{
- unsigned int lcore_id;
-
- /* add lcore_id to seed to avoid having the same sequence */
- for (lcore_id = 0; lcore_id < RTE_MAX_LCORE; lcore_id++)
- __rte_srand_lfsr258(seed + lcore_id, &rand_states[lcore_id]);
+ __atomic_store_n(&rte_rand_seed, seed, __ATOMIC_RELAXED);
}
static __rte_always_inline uint64_t
@@ -119,15 +116,18 @@ __rte_rand_lfsr258(struct rte_rand_state *state)
static __rte_always_inline
struct rte_rand_state *__rte_rand_get_state(void)
{
- unsigned int idx;
+ struct rte_rand_state *rand_state =
[PATCH 1/4] net/mlx5/hws: allow relaxed mode in MPLS matching
Remove the previous constrain on relaxed mode, that way it will be like
all other matching items, can be used in relaxed or non-relaxed mode.
The previous constrain was due to HW limitation that supports MPLS over
specific UDP port, now we give the ability to the user to make sure it
is with the right needs for MPLS matching.
Signed-off-by: Erez Shitrit
Reviewed-by: Alex Vesker
Acked-by: Matan Azrad
---
drivers/net/mlx5/hws/mlx5dr_definer.c | 45 ++-
1 file changed, 17 insertions(+), 28 deletions(-)
diff --git a/drivers/net/mlx5/hws/mlx5dr_definer.c
b/drivers/net/mlx5/hws/mlx5dr_definer.c
index 33d0f2d18e..88f22e7f70 100644
--- a/drivers/net/mlx5/hws/mlx5dr_definer.c
+++ b/drivers/net/mlx5/hws/mlx5dr_definer.c
@@ -1320,35 +1320,24 @@ mlx5dr_definer_conv_item_mpls(struct
mlx5dr_definer_conv_data *cd,
return rte_errno;
}
- if (cd->relaxed) {
- DR_LOG(ERR, "Relaxed mode is not supported");
- rte_errno = ENOTSUP;
- return rte_errno;
- }
-
- /* Currently support only MPLSoUDP */
- if (cd->last_item != RTE_FLOW_ITEM_TYPE_UDP &&
- cd->last_item != RTE_FLOW_ITEM_TYPE_MPLS) {
- DR_LOG(ERR, "MPLS supported only after UDP");
- rte_errno = ENOTSUP;
- return rte_errno;
- }
-
- /* In order to match on MPLS we must match on ip_protocol and l4_dport.
*/
- fc = &cd->fc[DR_CALC_FNAME(IP_PROTOCOL, false)];
- if (!fc->tag_set) {
- fc->item_idx = item_idx;
- fc->tag_mask_set = &mlx5dr_definer_ones_set;
- fc->tag_set = &mlx5dr_definer_udp_protocol_set;
- DR_CALC_SET(fc, eth_l2, l4_type_bwc, false);
- }
+ if (!cd->relaxed) {
+ /* In order to match on MPLS we must match on ip_protocol and
l4_dport. */
+ fc = &cd->fc[DR_CALC_FNAME(IP_PROTOCOL, false)];
+ if (!fc->tag_set) {
+ fc->item_idx = item_idx;
+ fc->tag_mask_set = &mlx5dr_definer_ones_set;
+ fc->tag_set = &mlx5dr_definer_udp_protocol_set;
+ DR_CALC_SET(fc, eth_l2, l4_type_bwc, false);
+ }
- fc = &cd->fc[DR_CALC_FNAME(L4_DPORT, false)];
- if (!fc->tag_set) {
- fc->item_idx = item_idx;
- fc->tag_mask_set = &mlx5dr_definer_ones_set;
- fc->tag_set = &mlx5dr_definer_mpls_udp_port_set;
- DR_CALC_SET(fc, eth_l4, destination_port, false);
+ /* Currently support only MPLSoUDP */
+ fc = &cd->fc[DR_CALC_FNAME(L4_DPORT, false)];
+ if (!fc->tag_set) {
+ fc->item_idx = item_idx;
+ fc->tag_mask_set = &mlx5dr_definer_ones_set;
+ fc->tag_set = &mlx5dr_definer_mpls_udp_port_set;
+ DR_CALC_SET(fc, eth_l4, destination_port, false);
+ }
}
if (m && (!is_mem_zero(m->label_tc_s, 3) || m->ttl)) {
--
2.18.2
[PATCH 2/4] net/mlx5/hws: allow attaching action template to matcher
Allow user to add new action-template after the creation of the matcher.
It is allowed only if the user indicates in the mlx5dr_matcher_attr
that he might add new AT in the future by indicating the max_num_of_at
and if needed the max_num_of_actions_in_at value.
With these two values the matcher is configured to get new AT in the
future.
Signed-off-by: Erez Shitrit
Signed-off-by: Alex Vesker
Acked-by: Matan Azrad
---
drivers/net/mlx5/hws/mlx5dr.h | 13 +
drivers/net/mlx5/hws/mlx5dr_matcher.c | 82 +++
2 files changed, 83 insertions(+), 12 deletions(-)
diff --git a/drivers/net/mlx5/hws/mlx5dr.h b/drivers/net/mlx5/hws/mlx5dr.h
index 5592af93c9..184de8feaf 100644
--- a/drivers/net/mlx5/hws/mlx5dr.h
+++ b/drivers/net/mlx5/hws/mlx5dr.h
@@ -139,6 +139,8 @@ struct mlx5dr_matcher_attr {
uint8_t num_log;
} rule;
};
+ /* Optional AT attach configuration - Max number of additional AT */
+ uint8_t max_num_of_at_attach;
};
struct mlx5dr_rule_attr {
@@ -328,6 +330,17 @@ mlx5dr_matcher_create(struct mlx5dr_table *table,
*/
int mlx5dr_matcher_destroy(struct mlx5dr_matcher *matcher);
+/* Attach new action template to direct rule matcher.
+ *
+ * @param[in] matcher
+ * Matcher to attach at to.
+ * @param[in] at
+ * Action template to be attached to the matcher.
+ * @return zero on success non zero otherwise.
+ */
+int mlx5dr_matcher_attach_at(struct mlx5dr_matcher *matcher,
+struct mlx5dr_action_template *at);
+
/* Get the size of the rule handle (mlx5dr_rule) to be used on rule creation.
*
* @return size in bytes of rule handle struct.
diff --git a/drivers/net/mlx5/hws/mlx5dr_matcher.c
b/drivers/net/mlx5/hws/mlx5dr_matcher.c
index 1fe7ec1bc3..a02f42a7e8 100644
--- a/drivers/net/mlx5/hws/mlx5dr_matcher.c
+++ b/drivers/net/mlx5/hws/mlx5dr_matcher.c
@@ -680,6 +680,30 @@ static void mlx5dr_matcher_set_pool_attr(struct
mlx5dr_pool_attr *attr,
}
}
+static int mlx5dr_matcher_check_and_process_at(struct mlx5dr_matcher *matcher,
+ struct mlx5dr_action_template
*at)
+{
+ bool valid;
+ int ret;
+
+ /* Check if action combinabtion is valid */
+ valid = mlx5dr_action_check_combo(at->action_type_arr,
matcher->tbl->type);
+ if (!valid) {
+ DR_LOG(ERR, "Invalid combination in action template");
+ rte_errno = EINVAL;
+ return rte_errno;
+ }
+
+ /* Process action template to setters */
+ ret = mlx5dr_action_template_process(at);
+ if (ret) {
+ DR_LOG(ERR, "Failed to process action template");
+ return ret;
+ }
+
+ return 0;
+}
+
static int mlx5dr_matcher_bind_at(struct mlx5dr_matcher *matcher)
{
bool is_jumbo = mlx5dr_matcher_mt_is_jumbo(matcher->mt);
@@ -689,22 +713,13 @@ static int mlx5dr_matcher_bind_at(struct mlx5dr_matcher
*matcher)
struct mlx5dr_context *ctx = tbl->ctx;
uint32_t required_stes;
int i, ret;
- bool valid;
for (i = 0; i < matcher->num_of_at; i++) {
struct mlx5dr_action_template *at = &matcher->at[i];
- /* Check if action combinabtion is valid */
- valid = mlx5dr_action_check_combo(at->action_type_arr,
matcher->tbl->type);
- if (!valid) {
- DR_LOG(ERR, "Invalid combination in action template
%d", i);
- return rte_errno;
- }
-
- /* Process action template to setters */
- ret = mlx5dr_action_template_process(at);
+ ret = mlx5dr_matcher_check_and_process_at(matcher, at);
if (ret) {
- DR_LOG(ERR, "Failed to process action template %d", i);
+ DR_LOG(ERR, "Invalid at %d", i);
return rte_errno;
}
@@ -924,6 +939,10 @@ mlx5dr_matcher_process_attr(struct mlx5dr_cmd_query_caps
*caps,
DR_LOG(ERR, "Root matcher can't specify FDB direction");
goto not_supported;
}
+ if (attr->max_num_of_at_attach) {
+ DR_LOG(ERR, "Root matcher does not support at
attaching");
+ goto not_supported;
+ }
return 0;
}
@@ -1039,6 +1058,8 @@ mlx5dr_matcher_create_col_matcher(struct mlx5dr_matcher
*matcher)
if (col_matcher->attr.table.sz_row_log >
MLX5DR_MATCHER_ASSURED_ROW_RATIO)
col_matcher->attr.table.sz_row_log -=
MLX5DR_MATCHER_ASSURED_ROW_RATIO;
+ col_matcher->attr.max_num_of_at_attach =
matcher->attr.max_num_of_at_attach;
+
ret = mlx5dr_matcher_process_attr(ctx->caps, col_matcher, false);
if (ret)
goto free_col_matcher;
@@ -1212,6 +1233,42 @@ static int mlx5dr_matcher_uninit
[PATCH 3/4] net/mlx5/hws: print syndrome value on error
Print the syndrome of failure of FW command.
Signed-off-by: Erez Shitrit
Reviewed-by: Alex Vesker
Acked-by: Matan Azrad
---
drivers/net/mlx5/hws/mlx5dr_cmd.c | 48 ++-
1 file changed, 34 insertions(+), 14 deletions(-)
diff --git a/drivers/net/mlx5/hws/mlx5dr_cmd.c
b/drivers/net/mlx5/hws/mlx5dr_cmd.c
index f9f220cc6a..7771aeb8cf 100644
--- a/drivers/net/mlx5/hws/mlx5dr_cmd.c
+++ b/drivers/net/mlx5/hws/mlx5dr_cmd.c
@@ -4,6 +4,12 @@
#include "mlx5dr_internal.h"
+static uint32_t mlx5dr_cmd_get_syndrome(uint32_t *out)
+{
+ /* Assumption: syndrome is always the second u32 */
+ return be32toh(out[1]);
+}
+
int mlx5dr_cmd_destroy_obj(struct mlx5dr_devx_obj *devx_obj)
{
int ret;
@@ -39,7 +45,8 @@ mlx5dr_cmd_flow_table_create(struct ibv_context *ctx,
devx_obj->obj = mlx5_glue->devx_obj_create(ctx, in, sizeof(in), out,
sizeof(out));
if (!devx_obj->obj) {
- DR_LOG(ERR, "Failed to create FT");
+ DR_LOG(ERR, "Failed to create FT (syndrome: %#x)",
+ mlx5dr_cmd_get_syndrome(out));
simple_free(devx_obj);
rte_errno = errno;
return NULL;
@@ -73,7 +80,8 @@ mlx5dr_cmd_flow_table_modify(struct mlx5dr_devx_obj *devx_obj,
ret = mlx5_glue->devx_obj_modify(devx_obj->obj, in, sizeof(in), out,
sizeof(out));
if (ret) {
- DR_LOG(ERR, "Failed to modify FT");
+ DR_LOG(ERR, "Failed to modify FT (syndrome: %#x)",
+ mlx5dr_cmd_get_syndrome(out));
rte_errno = errno;
}
@@ -96,7 +104,8 @@ mlx5dr_cmd_flow_table_query(struct mlx5dr_devx_obj *devx_obj,
ret = mlx5_glue->devx_obj_query(devx_obj->obj, in, sizeof(in), out,
sizeof(out));
if (ret) {
- DR_LOG(ERR, "Failed to query FT");
+ DR_LOG(ERR, "Failed to query FT (syndrome: %#x)",
+ mlx5dr_cmd_get_syndrome(out));
rte_errno = errno;
return ret;
}
@@ -129,7 +138,8 @@ mlx5dr_cmd_flow_group_create(struct ibv_context *ctx,
devx_obj->obj = mlx5_glue->devx_obj_create(ctx, in, sizeof(in), out,
sizeof(out));
if (!devx_obj->obj) {
- DR_LOG(ERR, "Failed to create Flow group");
+ DR_LOG(ERR, "Failed to create Flow group(syndrome: %#x)",
+ mlx5dr_cmd_get_syndrome(out));
simple_free(devx_obj);
rte_errno = errno;
return NULL;
@@ -182,7 +192,8 @@ mlx5dr_cmd_set_fte(struct ibv_context *ctx,
devx_obj->obj = mlx5_glue->devx_obj_create(ctx, in, sizeof(in), out,
sizeof(out));
if (!devx_obj->obj) {
- DR_LOG(ERR, "Failed to create FTE");
+ DR_LOG(ERR, "Failed to create FTE (syndrome: %#x)",
+ mlx5dr_cmd_get_syndrome(out));
rte_errno = errno;
goto free_devx;
}
@@ -325,7 +336,8 @@ mlx5dr_cmd_rtc_create(struct ibv_context *ctx,
devx_obj->obj = mlx5_glue->devx_obj_create(ctx, in, sizeof(in), out,
sizeof(out));
if (!devx_obj->obj) {
- DR_LOG(ERR, "Failed to create RTC");
+ DR_LOG(ERR, "Failed to create RTC (syndrome: %#x)",
+ mlx5dr_cmd_get_syndrome(out));
simple_free(devx_obj);
rte_errno = errno;
return NULL;
@@ -365,7 +377,8 @@ mlx5dr_cmd_stc_create(struct ibv_context *ctx,
devx_obj->obj = mlx5_glue->devx_obj_create(ctx, in, sizeof(in), out,
sizeof(out));
if (!devx_obj->obj) {
- DR_LOG(ERR, "Failed to create STC");
+ DR_LOG(ERR, "Failed to create STC (syndrome: %#x)",
+ mlx5dr_cmd_get_syndrome(out));
simple_free(devx_obj);
rte_errno = errno;
return NULL;
@@ -505,7 +518,8 @@ mlx5dr_cmd_stc_modify(struct mlx5dr_devx_obj *devx_obj,
ret = mlx5_glue->devx_obj_modify(devx_obj->obj, in, sizeof(in), out,
sizeof(out));
if (ret) {
- DR_LOG(ERR, "Failed to modify STC FW action_type %d",
stc_attr->action_type);
+ DR_LOG(ERR, "Failed to modify STC FW action_type %d (syndrome:
%#x)",
+ stc_attr->action_type, mlx5dr_cmd_get_syndrome(out));
rte_errno = errno;
}
@@ -542,7 +556,8 @@ mlx5dr_cmd_arg_create(struct ibv_context *ctx,
devx_obj->obj = mlx5_glue->devx_obj_create(ctx, in, sizeof(in), out,
sizeof(out));
if (!devx_obj->obj) {
- DR_LOG(ERR, "Failed to create ARG");
+ DR_LOG(ERR, "Failed to create ARG (syndrome: %#x)",
+ mlx5dr_cmd_get_syndrome(out));
simple_free(devx_obj);
rte_errno = errno;
return NULL;
@@ -606,7 +621,8 @@ mlx5dr_cmd_header_modify_pattern_create(struct ibv_context
*ctx,
devx_o
[PATCH 4/4] net/mlx5/hws: skip process action-template on coll matcher
Collision matcher uses the same action-template and action STE's as its
parent matcher, so ne need to redo it.
Signed-off-by: Erez Shitrit
Reviewed-by: Alex Vesker
Acked-by: Matan Azrad
---
drivers/net/mlx5/hws/mlx5dr_matcher.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/net/mlx5/hws/mlx5dr_matcher.c
b/drivers/net/mlx5/hws/mlx5dr_matcher.c
index a02f42a7e8..102e4f19c8 100644
--- a/drivers/net/mlx5/hws/mlx5dr_matcher.c
+++ b/drivers/net/mlx5/hws/mlx5dr_matcher.c
@@ -714,6 +714,9 @@ static int mlx5dr_matcher_bind_at(struct mlx5dr_matcher
*matcher)
uint32_t required_stes;
int i, ret;
+ if (matcher->flags & MLX5DR_MATCHER_FLAGS_COLLISION)
+ return 0;
+
for (i = 0; i < matcher->num_of_at; i++) {
struct mlx5dr_action_template *at = &matcher->at[i];
@@ -786,7 +789,7 @@ static void mlx5dr_matcher_unbind_at(struct mlx5dr_matcher
*matcher)
{
struct mlx5dr_table *tbl = matcher->tbl;
- if (!matcher->action_ste.max_stes)
+ if (!matcher->action_ste.max_stes || matcher->flags &
MLX5DR_MATCHER_FLAGS_COLLISION)
return;
mlx5dr_action_free_single_stc(tbl->ctx, tbl->type,
&matcher->action_ste.stc);
--
2.18.2
RE: [PATCH v2] net/iavf: fix port stats not cleared
> -Original Message-
> From: Yiding Zhou
> Sent: Thursday, September 7, 2023 10:40 AM
> To: dev@dpdk.org
> Cc: Zhou, YidingX ; sta...@dpdk.org; Xu, KuanX
>
> Subject: [PATCH v2] net/iavf: fix port stats not cleared
>
> After VF reset, kernel driver may reuse the orignal VSI without reset its
> stats.
> Call 'iavf_dev_stats_reset' during the initialization of the VF in order to
> clear
> any statistics that may exist from the last use of the VF and to avoid
> statistics
> errors.
>
> Fixes: 22b123a36d07 ("net/avf: initialize PMD")
> Cc: sta...@dpdk.org
>
> Signed-off-by: Kuan Xu
> Signed-off-by: Yiding Zhou
> ---
> drivers/net/iavf/iavf_ethdev.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/net/iavf/iavf_ethdev.c b/drivers/net/iavf/iavf_ethdev.c
> index f2fc5a5621..24c6342dee 100644
> --- a/drivers/net/iavf/iavf_ethdev.c
> +++ b/drivers/net/iavf/iavf_ethdev.c
> @@ -2721,6 +2721,7 @@ iavf_dev_init(struct rte_eth_dev *eth_dev)
>
> iavf_default_rss_disable(adapter);
>
> + iavf_dev_stats_reset(eth_dev);
>
> /* Start device watchdog */
> iavf_dev_watchdog_enable(adapter);
> --
> 2.34.1
Acked-by: Qi Zhang
Applied to dpdk-next-net-intel.
Thanks
Qi
RE: [PATCH v3 1/9] net/cpfl: parse flow parser file in devargs
> -Original Message- > From: Qiao, Wenjing > Sent: Wednesday, September 6, 2023 5:34 PM > To: Zhang, Yuying ; dev@dpdk.org; Zhang, Qi Z > ; Wu, Jingjing ; Xing, Beilei > > Cc: Liu, Mingxia ; Qiao, Wenjing > > Subject: [PATCH v3 1/9] net/cpfl: parse flow parser file in devargs > > Add devargs "flow_parser" for rte_flow json parser. > > Signed-off-by: Wenjing Qiao > --- > doc/guides/nics/cpfl.rst | 32 > drivers/net/cpfl/cpfl_ethdev.c | 38 > +- > drivers/net/cpfl/cpfl_ethdev.h | 3 +++ > drivers/net/cpfl/meson.build | 6 ++ > 4 files changed, 78 insertions(+), 1 deletion(-) > > diff --git a/doc/guides/nics/cpfl.rst b/doc/guides/nics/cpfl.rst > index c20334230b..7032dd1a1a 100644 > --- a/doc/guides/nics/cpfl.rst > +++ b/doc/guides/nics/cpfl.rst > @@ -128,12 +128,24 @@ Runtime Configuration > > -a BDF,representor=vf[0-3],representor=c1pf1 > > +- ``flow_parser`` (default ``not enabled``) > + > + The PMD supports using a JSON file to parse rte_flow tokens into low level > hardware > + resources defined in a DDP package file. > + > + The user can specify the path of json file, for example:: > + > +-a ca:00.0,flow_parser="refpkg.json" > + > + Then the PMD will load json file for device ``ca:00.0``. > + The parameter is optional. > > Driver compilation and testing > -- > > Refer to the document :doc:`build_and_test` for details. > > +Rte flow need to install json-c library. > > Features > > @@ -164,3 +176,23 @@ Hairpin queue > E2100 Series can loopback packets from RX port to TX port. > This feature is called port-to-port or hairpin. > Currently, the PMD only supports single port hairpin. > + > +Rte_flow > +~ > + > +Rte_flow uses a json file to direct CPF PMD to parse rte_flow tokens into > +low level hardware resources defined in a DDP package file. > + > +#. install json-c library:: > + > + .. code-block:: console > + > + git clone https://github.com/json-c/json-c.git > + cd json-c > + git checkout 777dd06be83ef7fac71c2218b565557cd068a714 > + Json-c is the dependency, we can install by package management tool, such as apt, can you add that refer? If we need to install from source code, version number might be better that commit id.
RE: [PATCH v4] net/iavf: unregister intr handler before FD close
> -Original Message-
> From: Saurabh Singhal
> Sent: Thursday, September 7, 2023 11:15 AM
> To: Thomas Monjalon ; Wu, Jingjing
> ; Xing, Beilei
> Cc: dev@dpdk.org; Singhal, Saurabh
> Subject: [PATCH v4] net/iavf: unregister intr handler before FD close
>
> Unregister VFIO interrupt handler before the interrupt fd gets closed in case
> iavf_dev_init() returns an error.
>
> dpdk creates a standalone thread named eal-intr-thread for processing
> interrupts for the PCI devices. The interrupt handler callbacks are registered
> by the VF driver(iavf, in this case).
>
> When we do a PCI probe of the network interfaces, we register an interrupt
> handler, open a vfio-device fd using ioctl, and an eventfd in dpdk. These
> interrupt sources are registered in a global linked list that the
> eal-intr-thread
> keeps iterating over for handling the interrupts. In our internal testing, we
> see
> eal-intr-thread crash in these two ways:
>
> Error adding fd 660 epoll_ctl, Operation not permitted
>
> or
>
> Error adding fd 660 epoll_ctl, Bad file descriptor
>
> epoll_ctl() returns EPERM if the target fd does not support poll.
> It returns EBADF when the epoll fd itself is closed or the target fd is
> closed.
>
> When the first type of crash happens, we see that the fd 660 is
> anon_inode:[vfio-device] which does not support poll.
>
> When the second type of crash happens, we could see from the fd map of
> the crashing process that the fd 660 was already closed.
>
> This means the said fd has been closed and in certain cases may have been
> reassigned to a different device by the operating system but the eal-intr-
> thread does not know about it.
>
> We observed that these crashes were always accompanied by an error in
> iavf_dev_init() after rte_intr_callback_register() and
> iavf_enable_irq0() have already happened. In the error path, the
> intr_handle_fd was being closed but the interrupt handler wasn't being
> unregistered.
>
> The fix is to unregister the interrupt handle in the
> iavf_dev_init() error path.
>
> Ensure proper cleanup if iavf_security_init() or
> iavf_security_ctx_create() fail. Earlier, we were leaking memory by simply
> returning from iavf_dev_init().
Fixes: 22b123a36d07 ("net/avf: initialize PMD")
Fixes: 6bc987ecb860 ("net/iavf: support IPsec inline crypto")
Cc: sta...@dpdk.org
>
> Signed-off-by: Saurabh Singhal
Acked-by: Qi Zhang
Applied to dpdk-next-net-intel.
Thanks
Qi
RE: [PATCH 23/36] net/nfp: fix Rx and Tx queue state
Thanks, it looks good to me.
Acked-by: Chaoyong He
> -Original Message-
> From: Jie Hai
> Sent: Friday, September 8, 2023 7:29 PM
> To: dev@dpdk.org; Chaoyong He ; Niklas
> Soderlund ; Chengwen Feng
> ; Lijun Ou ; Konstantin
> Ananyev
> <"konstantin.v.ananyev@yandex.rukonstantin.ananyev"@huawei.com>;
> Thomas Monjalon ; Ferruh Yigit
>
> Cc: haij...@huawei.com; lihuis...@huawei.com
> Subject: [PATCH 23/36] net/nfp: fix Rx and Tx queue state
>
> [Some people who received this message don't often get email from
> haij...@huawei.com. Learn why this is important at
> https://aka.ms/LearnAboutSenderIdentification ]
>
> The DPDK framework reports the queue state, which is stored in
> dev->data->tx_queue_state and dev->data->rx_queue_state. The
> state is maintained by the driver. Users may determine whether a queue
> participates in packet forwarding based on the state.
> Therefore, the driver needs to modify the queue state in time according to the
> actual situation.
>
> Fixes: 9ad9ff476cac ("ethdev: add queue state in queried queue information")
> Cc: sta...@dpdk.org
>
> Signed-off-by: Jie Hai
> ---
> drivers/net/nfp/flower/nfp_flower.c | 8
> drivers/net/nfp/flower/nfp_flower_representor.c | 12
> drivers/net/nfp/nfp_common.c| 2 ++
> drivers/net/nfp/nfp_ethdev.c| 6 ++
> drivers/net/nfp/nfp_ethdev_vf.c | 6 ++
> 5 files changed, 34 insertions(+)
>
> diff --git a/drivers/net/nfp/flower/nfp_flower.c
> b/drivers/net/nfp/flower/nfp_flower.c
> index 77dab864f319..eb7b40a6eb25 100644
> --- a/drivers/net/nfp/flower/nfp_flower.c
> +++ b/drivers/net/nfp/flower/nfp_flower.c
> @@ -85,6 +85,7 @@ int
> nfp_flower_pf_start(struct rte_eth_dev *dev) {
> int ret;
> + uint16_t i;
> uint32_t new_ctrl;
> uint32_t update = 0;
> struct nfp_net_hw *hw;
> @@ -137,6 +138,11 @@ nfp_flower_pf_start(struct rte_eth_dev *dev)
> return -EIO;
> }
>
> + for (i = 0; i < dev->data->nb_rx_queues; i++)
> + dev->data->rx_queue_state[i] = RTE_ETH_QUEUE_STATE_STARTED;
> + for (i = 0; i < dev->data->nb_tx_queues; i++)
> + dev->data->tx_queue_state[i] =
> + RTE_ETH_QUEUE_STATE_STARTED;
> +
> return 0;
> }
>
> @@ -159,11 +165,13 @@ nfp_flower_pf_stop(struct rte_eth_dev *dev)
> for (i = 0; i < dev->data->nb_tx_queues; i++) {
> this_tx_q = dev->data->tx_queues[i];
> nfp_net_reset_tx_queue(this_tx_q);
> + dev->data->tx_queue_state[i] =
> + RTE_ETH_QUEUE_STATE_STOPPED;
> }
>
> for (i = 0; i < dev->data->nb_rx_queues; i++) {
> this_rx_q = dev->data->rx_queues[i];
> nfp_net_reset_rx_queue(this_rx_q);
> + dev->data->rx_queue_state[i] =
> + RTE_ETH_QUEUE_STATE_STOPPED;
> }
>
> if (rte_eal_process_type() == RTE_PROC_PRIMARY) diff --git
> a/drivers/net/nfp/flower/nfp_flower_representor.c
> b/drivers/net/nfp/flower/nfp_flower_representor.c
> index 5f94d20f1b0c..3f97bc9f8a39 100644
> --- a/drivers/net/nfp/flower/nfp_flower_representor.c
> +++ b/drivers/net/nfp/flower/nfp_flower_representor.c
> @@ -303,6 +303,7 @@ nfp_flower_repr_dev_start(struct rte_eth_dev *dev)
> {
> struct nfp_flower_representor *repr;
> struct nfp_app_fw_flower *app_fw_flower;
> + uint16_t i;
>
> repr = dev->data->dev_private;
> app_fw_flower = repr->app_fw_flower; @@ -314,6 +315,11 @@
> nfp_flower_repr_dev_start(struct rte_eth_dev *dev)
>
> nfp_flower_cmsg_port_mod(app_fw_flower, repr->port_id, true);
>
> + for (i = 0; i < dev->data->nb_rx_queues; i++)
> + dev->data->rx_queue_state[i] = RTE_ETH_QUEUE_STATE_STARTED;
> + for (i = 0; i < dev->data->nb_tx_queues; i++)
> + dev->data->tx_queue_state[i] =
> + RTE_ETH_QUEUE_STATE_STARTED;
> +
> return 0;
> }
>
> @@ -322,6 +328,7 @@ nfp_flower_repr_dev_stop(struct rte_eth_dev *dev)
> {
> struct nfp_flower_representor *repr;
> struct nfp_app_fw_flower *app_fw_flower;
> + uint16_t i;
>
> repr = dev->data->dev_private;
> app_fw_flower = repr->app_fw_flower; @@ -333,6 +340,11 @@
> nfp_flower_repr_dev_stop(struct rte_eth_dev *dev)
> repr->nfp_idx, 0);
> }
>
> + for (i = 0; i < dev->data->nb_rx_queues; i++)
> + dev->data->rx_queue_state[i] = RTE_ETH_QUEUE_STATE_STOPPED;
> + for (i = 0; i < dev->data->nb_tx_queues; i++)
> + dev->data->tx_queue_state[i] =
> + RTE_ETH_QUEUE_STATE_STOPPED;
> +
> return 0;
> }
>
> diff --git a/drivers/net/nfp/nfp_common.c b/drivers/net/nfp/nfp_common.c
> index 5092e5869de4..c0d4708f2b3e 100644
> --- a/drivers/net/nfp/nfp_common.c
> +++ b/drivers/net/nfp/nfp_common.c
> @@ -1927,6 +1927,7 @@ nfp_net_stop_rx_queue(struct rte_eth_dev *dev)
>
RE: [PATCH v3 2/9] net/cpfl: add flow json parser
> +static int
> +cpfl_json_object_to_int(json_object *object, const char *name, int *value)
> +{
> + json_object *subobject;
> +
> + if (!object) {
> + PMD_DRV_LOG(ERR, "object doesn't exist.");
> + return -EINVAL;
> + }
> + subobject = json_object_object_get(object, name);
> + if (!subobject) {
> + PMD_DRV_LOG(ERR, "%s doesn't exist.", name);
> + return -EINVAL;
> + }
> + *value = json_object_get_int(subobject);
> +
> + return 0;
> +}
> +
> +static int
> +cpfl_json_object_to_uint16(json_object *object, const char *name, uint16_t
> *value)
> +{
Looks no need to define a new function as there is no difference with
cpfl_json_object_to_int func beside the type of return value.
[...]
> +
> +static int
> +cpfl_flow_js_pattern_key_proto_field(json_object *cjson_field,
> + struct cpfl_flow_js_pr_key_proto *js_field)
> +{
> + int len, i;
> +
> + if (!cjson_field)
> + return 0;
> + len = json_object_array_length(cjson_field);
> + js_field->fields_size = len;
> + if (len == 0)
Move if check above, before set js_field->fields_size?
> + return 0;
> + js_field->fields =
> + rte_malloc(NULL, sizeof(struct cpfl_flow_js_pr_key_proto_field) *
> len, 0);
> + if (!js_field->fields) {
> + PMD_DRV_LOG(ERR, "Failed to alloc memory.");
> + return -ENOMEM;
> + }
> + for (i = 0; i < len; i++) {
> + json_object *object;
> + const char *name, *mask;
> +
> + object = json_object_array_get_idx(cjson_field, i);
> + name = cpfl_json_object_to_string(object, "name");
> + if (!name) {
> + PMD_DRV_LOG(ERR, "Can not parse string 'name'.");
> + goto err;
> + }
> + if (strlen(name) > CPFL_FLOW_JSON_STR_SIZE_MAX) {
> + PMD_DRV_LOG(ERR, "The 'name' is too long.");
> + goto err;
> + }
> + memcpy(js_field->fields[i].name, name, strlen(name));
Is js_field->fields[i].name zeroed? If not, using strlen() cannot guarantee
string copy correct.
> + if (js_field->type == RTE_FLOW_ITEM_TYPE_ETH ||
> + js_field->type == RTE_FLOW_ITEM_TYPE_IPV4) {
> + mask = cpfl_json_object_to_string(object, "mask");
> + if (!mask) {
> + PMD_DRV_LOG(ERR, "Can not parse string
> 'mask'.");
> + goto err;
> + }
> + memcpy(js_field->fields[i].mask, mask, strlen(mask));
The same as above.
> + } else {
> + uint32_t mask_32b;
> + int ret;
> +
> + ret = cpfl_json_object_to_uint32(object, "mask",
> &mask_32b);
> + if (ret < 0) {
> + PMD_DRV_LOG(ERR, "Can not parse uint32
> 'mask'.");
> + goto err;
> + }
> + js_field->fields[i].mask_32b = mask_32b;
> + }
> + }
> +
> + return 0;
> +
> +err:
> + rte_free(js_field->fields);
> + return -EINVAL;
> +}
> +
> +static int
> +cpfl_flow_js_pattern_key_proto(json_object *cjson_pr_key_proto, struct
> cpfl_flow_js_pr *js_pr)
> +{
> + int len, i, ret;
> +
> + len = json_object_array_length(cjson_pr_key_proto);
> + js_pr->key.proto_size = len;
> + js_pr->key.protocols = rte_malloc(NULL, sizeof(struct
> cpfl_flow_js_pr_key_proto) * len, 0);
> + if (!js_pr->key.protocols) {
> + PMD_DRV_LOG(ERR, "Failed to alloc memory.");
> + return -ENOMEM;
> + }
> +
> + for (i = 0; i < len; i++) {
> + json_object *object, *cjson_pr_key_proto_fields;
> + const char *type;
> + enum rte_flow_item_type item_type;
> +
> + object = json_object_array_get_idx(cjson_pr_key_proto, i);
> + /* pr->key->proto->type */
> + type = cpfl_json_object_to_string(object, "type");
> + if (!type) {
> + PMD_DRV_LOG(ERR, "Can not parse string 'type'.");
> + goto err;
> + }
> + item_type = cpfl_get_item_type_by_str(type);
> + if (item_type == RTE_FLOW_ITEM_TYPE_VOID)
> + goto err;
> + js_pr->key.protocols[i].type = item_type;
> + /* pr->key->proto->fields */
> + cjson_pr_key_proto_fields = json_object_object_get(object,
> "fields");
> + ret =
> cpfl_flow_js_pattern_key_proto_field(cjson_pr_key_proto_fields,
> +&js_pr-
> >key.protocols[i]);
> + if (ret < 0)
> + goto err;
> + }
> +
> + return 0;
> +
> +err:
> + rte_free(js_pr->key.protocols);
> + retu
RE: [PATCH v3 4/9] net/cpfl: setup ctrl path
> -Original Message-
> From: Qiao, Wenjing
> Sent: Wednesday, September 6, 2023 5:34 PM
> To: Zhang, Yuying ; dev@dpdk.org; Zhang, Qi Z
> ; Wu, Jingjing ; Xing, Beilei
>
> Cc: Liu, Mingxia ; Qiao, Wenjing
>
> Subject: [PATCH v3 4/9] net/cpfl: setup ctrl path
>
> Setup the control vport and control queue for flow offloading.
>
> Signed-off-by: Yuying Zhang
> Signed-off-by: Beilei Xing
> Signed-off-by: Qi Zhang
> Signed-off-by: Wenjing Qiao
> ---
> drivers/net/cpfl/cpfl_ethdev.c | 267
> + drivers/net/cpfl/cpfl_ethdev.h |
> 14 ++ drivers/net/cpfl/cpfl_vchnl.c | 144 ++
> 3 files changed, 425 insertions(+)
>
> diff --git a/drivers/net/cpfl/cpfl_ethdev.c b/drivers/net/cpfl/cpfl_ethdev.c
> index 3c4a6a4724..22f3e72894 100644
> --- a/drivers/net/cpfl/cpfl_ethdev.c
> +++ b/drivers/net/cpfl/cpfl_ethdev.c
> @@ -1657,6 +1657,10 @@ cpfl_handle_vchnl_event_msg(struct
> cpfl_adapter_ext *adapter, uint8_t *msg, uint
> return;
> }
>
> + /* ignore if it is ctrl vport */
> + if (adapter->ctrl_vport.base.vport_id == vc_event->vport_id)
> + return;
> +
> vport = cpfl_find_vport(adapter, vc_event->vport_id);
> if (!vport) {
> PMD_DRV_LOG(ERR, "Can't find vport."); @@ -1852,6
> +1856,260 @@ cpfl_dev_alarm_handler(void *param)
> rte_eal_alarm_set(CPFL_ALARM_INTERVAL, cpfl_dev_alarm_handler,
> adapter); }
>
> +static int
> +cpfl_stop_cfgqs(struct cpfl_adapter_ext *adapter) {
> + int i, ret;
> +
> + for (i = 0; i < CPFL_TX_CFGQ_NUM; i++) {
> + ret = idpf_vc_queue_switch(&adapter->ctrl_vport.base, i,
> false, false);
> + if (ret) {
> + PMD_DRV_LOG(ERR, "Fail to disable Tx config
> queue.");
> + return ret;
> + }
> + }
> +
> + for (i = 0; i < CPFL_RX_CFGQ_NUM; i++) {
> + ret = idpf_vc_queue_switch(&adapter->ctrl_vport.base, i,
> true, false);
> + if (ret) {
> + PMD_DRV_LOG(ERR, "Fail to disable Rx config
> queue.");
> + return ret;
> + }
> + }
> +
> + return 0;
> +}
> +
> +static int
> +cpfl_start_cfgqs(struct cpfl_adapter_ext *adapter) {
> + int i, ret;
> +
> + ret = cpfl_config_ctlq_tx(adapter);
> + if (ret) {
> + PMD_DRV_LOG(ERR, "Fail to configure Tx config queue.");
> + return ret;
> + }
> +
> + ret = cpfl_config_ctlq_rx(adapter);
> + if (ret) {
> + PMD_DRV_LOG(ERR, "Fail to configure Rx config queue.");
> + return ret;
> + }
> +
> + for (i = 0; i < CPFL_TX_CFGQ_NUM; i++) {
> + ret = idpf_vc_queue_switch(&adapter->ctrl_vport.base, i,
> false, true);
> + if (ret) {
> + PMD_DRV_LOG(ERR, "Fail to enable Tx config
> queue.");
> + return ret;
> + }
> + }
> +
> + for (i = 0; i < CPFL_RX_CFGQ_NUM; i++) {
> + ret = idpf_vc_queue_switch(&adapter->ctrl_vport.base, i,
> true, true);
> + if (ret) {
> + PMD_DRV_LOG(ERR, "Fail to enable Rx config
> queue.");
> + return ret;
> + }
> + }
> +
> + return 0;
> +}
> +
> +static void
> +cpfl_remove_cfgqs(struct cpfl_adapter_ext *adapter) {
> + struct idpf_hw *hw = (struct idpf_hw *)(&adapter->base.hw);
> + struct cpfl_ctlq_create_info *create_cfgq_info;
> + int i;
> +
> + create_cfgq_info = adapter->cfgq_info;
> +
> + for (i = 0; i < CPFL_CFGQ_NUM; i++) {
> + cpfl_vport_ctlq_remove(hw, adapter->ctlqp[i]);
[Liu, Mingxia] Sometimes adapter->ctlqp[i] maybe NULL, then the an error will
be reported in cpfl_vport_ctlq_remove (), right?
Such as when this function is called by cpfl_add_cfgqs(), So better to check if
adapter->ctlqp[i] == NULL.
> + if (create_cfgq_info[i].ring_mem.va)
> + idpf_free_dma_mem(&adapter->base.hw,
> &create_cfgq_info[i].ring_mem);
> + if (create_cfgq_info[i].buf_mem.va)
> + idpf_free_dma_mem(&adapter->base.hw,
> &create_cfgq_info[i].buf_mem);
> + }
> +}
> +
> +static int
> +cpfl_add_cfgqs(struct cpfl_adapter_ext *adapter) {
> + struct idpf_ctlq_info *cfg_cq;
> + int ret = 0;
> + int i = 0;
> +
> + for (i = 0; i < CPFL_CFGQ_NUM; i++) {
> + ret = cpfl_vport_ctlq_add((struct idpf_hw *)(&adapter-
> >base.hw),
> + &adapter->cfgq_info[i],
> + &cfg_cq);
> + if (ret || !cfg_cq) {
[Liu, Mingxia] Before each loop, better to set cfg_cq with NULL ?
> + PMD_DRV_LOG(ERR, "ctlq add failed for queue
> id: %d",
> + adapter->cfgq_info[i].id);
> + cpfl_remove_cfgqs(adapter);
> + return ret;
> + }
> + PMD_DRV
RE: [PATCH v3 4/9] net/cpfl: setup ctrl path
> -Original Message- > From: Qiao, Wenjing > Sent: Wednesday, September 6, 2023 5:34 PM > To: Zhang, Yuying ; dev@dpdk.org; Zhang, Qi Z > ; Wu, Jingjing ; Xing, Beilei > > Cc: Liu, Mingxia ; Qiao, Wenjing > > Subject: [PATCH v3 4/9] net/cpfl: setup ctrl path > > Setup the control vport and control queue for flow offloading. In general, "[PATCH 3/9] net/cpfl: add FXP low level implementation" also contains ctrl queue setup functions. May need better organize the patch set.
