Hi Bing,
I see that your patch is causing some build failure on aarch64 with meson build.
-c ../../root/dpdk/drivers/net/mlx5/mlx5_flow_dv.c
../../root/dpdk/drivers/net/mlx5/mlx5_flow_dv.c: In function
'__flow_dv_translate':
../../root/dpdk/drivers/net/mlx5/mlx5_flow_dv.c:7339:48: error: passing
argument 1 of 'flow_dv_convert_action_modify_ipv4_dscp' from incompatible
pointer type [-Werror=incompatible-pointer-types]
if (flow_dv_convert_action_modify_ipv4_dscp(&mhdr_res,
^
../../root/dpdk/drivers/net/mlx5/mlx5_flow_dv.c:1213:1: note: expected 'struct
mlx5_flow_dv_modify_hdr_resource *' but argument is of type 'struct
mlx5_flow_dv_modify_hdr_resource **'
flow_dv_convert_action_modify_ipv4_dscp
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../root/dpdk/drivers/net/mlx5/mlx5_flow_dv.c:7345:48: error: passing
argument 1 of 'flow_dv_convert_action_modify_ipv6_dscp' from incompatible
pointer type [-Werror=incompatible-pointer-types]
if (flow_dv_convert_action_modify_ipv6_dscp(&mhdr_res,
^
../../root/dpdk/drivers/net/mlx5/mlx5_flow_dv.c:1248:1: note: expected 'struct
mlx5_flow_dv_modify_hdr_resource *' but argument is of type 'struct
mlx5_flow_dv_modify_hdr_resource **'
flow_dv_convert_action_modify_ipv6_dscp
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
ninja: build stopped: subcommand failed.
Can you please fix that and send a V3 for it ?
Kindest regards,
Raslan Darawsheh
> -----Original Message-----
> From: Bing Zhao <bi...@mellanox.com>
> Sent: Sunday, January 12, 2020 5:57 PM
> To: Ori Kam <or...@mellanox.com>; Slava Ovsiienko
> <viachesl...@mellanox.com>; Raslan Darawsheh <rasl...@mellanox.com>;
> Matan Azrad <ma...@mellanox.com>
> Cc: dev@dpdk.org; sta...@dpdk.org
> Subject: [PATCH v2] net/mlx5: fix modify actions support limitation
>
> In the root table, there is some limitation of total number of header
> modify actions, 16 or 8 for each. But in other tables, there is no
> such strict limitation. In an IPv6 case, the IP fields modifying
> will occupy more actions than that in IPv4, so the total support
> number should be increased in order to support as many actions as
> possible for an IPv6 + TCP packet.
> And in the meanwhile, the memory consumption should also be taken
> into consideration because sometimes only several actions are needed.
> The root table checking could also be done in low layer driver and
> the error code will be returned if the actions number is over the
> maximal supported value.
>
> Fixes: 0e9d00027686 ("net/mlx5: check maximum modify actions number")
> Signed-off-by: Bing Zhao <bi...@mellanox.com>
> ---
> drivers/net/mlx5/mlx5_flow.h | 15 +++---
> drivers/net/mlx5/mlx5_flow_dv.c | 104 ++++++++++++++++++++++---------
> ---------
> 2 files changed, 66 insertions(+), 53 deletions(-)
>
> diff --git a/drivers/net/mlx5/mlx5_flow.h b/drivers/net/mlx5/mlx5_flow.h
> index 27d82ac..4b493ee 100644
> --- a/drivers/net/mlx5/mlx5_flow.h
> +++ b/drivers/net/mlx5/mlx5_flow.h
> @@ -385,11 +385,14 @@ struct mlx5_flow_dv_tag_resource {
>
> /*
> * Number of modification commands.
> - * If extensive metadata registers are supported
> - * the maximal actions amount is 16 and 8 otherwise.
> + * If extensive metadata registers are supported, the maximal actions
> amount is
> + * 16 and 8 otherwise on root table. The validation could also be done in the
> + * lower driver layer.
> + * On non-root table, there is no limitation, but 32 is enough right now.
> */
> -#define MLX5_MODIFY_NUM 16
> -#define MLX5_MODIFY_NUM_NO_MREG 8
> +#define MLX5_MAX_MODIFY_NUM 32
> +#define MLX5_ROOT_TBL_MODIFY_NUM 16
> +#define MLX5_ROOT_TBL_MODIFY_NUM_NO_MREG 8
>
> /* Modify resource structure */
> struct mlx5_flow_dv_modify_hdr_resource {
> @@ -400,9 +403,9 @@ struct mlx5_flow_dv_modify_hdr_resource {
> /**< Verbs modify header action object. */
> uint8_t ft_type; /**< Flow table type, Rx or Tx. */
> uint32_t actions_num; /**< Number of modification actions. */
> - struct mlx5_modification_cmd actions[MLX5_MODIFY_NUM];
> - /**< Modification actions. */
> uint64_t flags; /**< Flags for RDMA API. */
> + struct mlx5_modification_cmd actions[];
> + /**< Modification actions. */
> };
>
> /* Jump action resource structure. */
> diff --git a/drivers/net/mlx5/mlx5_flow_dv.c
> b/drivers/net/mlx5/mlx5_flow_dv.c
> index e8a764c..398d60f 100644
> --- a/drivers/net/mlx5/mlx5_flow_dv.c
> +++ b/drivers/net/mlx5/mlx5_flow_dv.c
> @@ -363,7 +363,7 @@ struct field_modify_info modify_tcp[] = {
> uint32_t mask;
> uint32_t data;
>
> - if (i >= MLX5_MODIFY_NUM)
> + if (i >= MLX5_MAX_MODIFY_NUM)
> return rte_flow_error_set(error, EINVAL,
> RTE_FLOW_ERROR_TYPE_ACTION, NULL,
> "too many items to modify");
> @@ -404,11 +404,11 @@ struct field_modify_info modify_tcp[] = {
> ++i;
> ++field;
> } while (field->size);
> - resource->actions_num = i;
> - if (!resource->actions_num)
> + if (resource->actions_num == i)
> return rte_flow_error_set(error, EINVAL,
> RTE_FLOW_ERROR_TYPE_ACTION,
> NULL,
> "invalid modification flow item");
> + resource->actions_num = i;
> return 0;
> }
>
> @@ -569,7 +569,7 @@ struct field_modify_info modify_tcp[] = {
> struct mlx5_modification_cmd *actions = &resource->actions[i];
> struct field_modify_info *field = modify_vlan_out_first_vid;
>
> - if (i >= MLX5_MODIFY_NUM)
> + if (i >= MLX5_MAX_MODIFY_NUM)
> return rte_flow_error_set(error, EINVAL,
> RTE_FLOW_ERROR_TYPE_ACTION, NULL,
> "too many items to modify");
> @@ -902,7 +902,7 @@ struct field_modify_info modify_tcp[] = {
> struct mlx5_modification_cmd *actions = resource->actions;
> uint32_t i = resource->actions_num;
>
> - if (i >= MLX5_MODIFY_NUM)
> + if (i >= MLX5_MAX_MODIFY_NUM)
> return rte_flow_error_set(error, EINVAL,
> RTE_FLOW_ERROR_TYPE_ACTION,
> NULL,
> "too many items to modify");
> @@ -914,10 +914,6 @@ struct field_modify_info modify_tcp[] = {
> actions[i].data1 = rte_cpu_to_be_32(conf->data);
> ++i;
> resource->actions_num = i;
> - if (!resource->actions_num)
> - return rte_flow_error_set(error, EINVAL,
> - RTE_FLOW_ERROR_TYPE_ACTION,
> NULL,
> - "invalid modification flow item");
> return 0;
> }
>
> @@ -2256,7 +2252,6 @@ struct field_modify_info modify_tcp[] = {
> domain = sh->rx_domain;
> else
> domain = sh->tx_domain;
> -
> /* Lookup a matching resource from cache. */
> LIST_FOREACH(cache_resource, &sh->encaps_decaps, next) {
> if (resource->reformat_type == cache_resource-
> >reformat_type &&
> @@ -3367,21 +3362,27 @@ struct field_modify_info modify_tcp[] = {
> *
> * @param dev
> * Pointer to rte_eth_dev structure.
> + * @param flags
> + * Flags bits to check if root level.
> *
> * @return
> * Max number of modify header actions device can support.
> */
> static unsigned int
> -flow_dv_modify_hdr_action_max(struct rte_eth_dev *dev)
> +flow_dv_modify_hdr_action_max(struct rte_eth_dev *dev, uint64_t flags)
> {
> /*
> * There's no way to directly query the max cap. Although it has to be
> * acquried by iterative trial, it is a safe assumption that more
> * actions are supported by FW if extensive metadata register is
> - * supported.
> + * supported. (Only in the root table)
> */
> - return mlx5_flow_ext_mreg_supported(dev) ?
> MLX5_MODIFY_NUM :
> -
> MLX5_MODIFY_NUM_NO_MREG;
> + if (!(flags & MLX5DV_DR_ACTION_FLAGS_ROOT_LEVEL))
> + return MLX5_MAX_MODIFY_NUM;
> + else
> + return mlx5_flow_ext_mreg_supported(dev) ?
> + MLX5_ROOT_TBL_MODIFY_NUM :
> +
> MLX5_ROOT_TBL_MODIFY_NUM_NO_MREG;
> }
>
> /**
> @@ -3472,8 +3473,12 @@ struct field_modify_info modify_tcp[] = {
> struct mlx5_ibv_shared *sh = priv->sh;
> struct mlx5_flow_dv_modify_hdr_resource *cache_resource;
> struct mlx5dv_dr_domain *ns;
> + uint32_t actions_len;
>
> - if (resource->actions_num >
> flow_dv_modify_hdr_action_max(dev))
> + resource->flags =
> + dev_flow->group ? 0 :
> MLX5DV_DR_ACTION_FLAGS_ROOT_LEVEL;
> + if (resource->actions_num > flow_dv_modify_hdr_action_max(dev,
> + resource->flags))
> return rte_flow_error_set(error, EOVERFLOW,
> RTE_FLOW_ERROR_TYPE_ACTION,
> NULL,
> "too many modify header items");
> @@ -3483,17 +3488,15 @@ struct field_modify_info modify_tcp[] = {
> ns = sh->tx_domain;
> else
> ns = sh->rx_domain;
> - resource->flags =
> - dev_flow->group ? 0 :
> MLX5DV_DR_ACTION_FLAGS_ROOT_LEVEL;
> /* Lookup a matching resource from cache. */
> + actions_len = resource->actions_num * sizeof(resource->actions[0]);
> LIST_FOREACH(cache_resource, &sh->modify_cmds, next) {
> if (resource->ft_type == cache_resource->ft_type &&
> resource->actions_num == cache_resource->actions_num
> &&
> resource->flags == cache_resource->flags &&
> !memcmp((const void *)resource->actions,
> (const void *)cache_resource->actions,
> - (resource->actions_num *
> - sizeof(resource->actions[0])))) {
> + actions_len)) {
> DRV_LOG(DEBUG, "modify-header resource %p:
> refcnt %d++",
> (void *)cache_resource,
> rte_atomic32_read(&cache_resource-
> >refcnt));
> @@ -3503,18 +3506,18 @@ struct field_modify_info modify_tcp[] = {
> }
> }
> /* Register new modify-header resource. */
> - cache_resource = rte_calloc(__func__, 1, sizeof(*cache_resource),
> 0);
> + cache_resource = rte_calloc(__func__, 1,
> + sizeof(*cache_resource) + actions_len, 0);
> if (!cache_resource)
> return rte_flow_error_set(error, ENOMEM,
>
> RTE_FLOW_ERROR_TYPE_UNSPECIFIED, NULL,
> "cannot allocate resource
> memory");
> *cache_resource = *resource;
> + rte_memcpy(cache_resource->actions, resource->actions,
> actions_len);
> cache_resource->verbs_action =
> mlx5_glue->dv_create_flow_action_modify_header
> - (sh->ctx, cache_resource->ft_type,
> - ns, cache_resource->flags,
> - cache_resource->actions_num *
> - sizeof(cache_resource->actions[0]),
> + (sh->ctx, cache_resource->ft_type,
> ns,
> + cache_resource->flags, actions_len,
> (uint64_t *)cache_resource-
> >actions);
> if (!cache_resource->verbs_action) {
> rte_free(cache_resource);
> @@ -6739,10 +6742,13 @@ struct field_modify_info modify_tcp[] = {
> };
> int actions_n = 0;
> bool actions_end = false;
> - struct mlx5_flow_dv_modify_hdr_resource mhdr_res = {
> - .ft_type = attr->egress ?
> MLX5DV_FLOW_TABLE_TYPE_NIC_TX :
> -
> MLX5DV_FLOW_TABLE_TYPE_NIC_RX
> - };
> + union {
> + struct mlx5_flow_dv_modify_hdr_resource res;
> + uint8_t len[sizeof(struct
> mlx5_flow_dv_modify_hdr_resource) +
> + sizeof(struct mlx5_modification_cmd) *
> + (MLX5_MAX_MODIFY_NUM + 1)];
> + } mhdr_dummy;
> + struct mlx5_flow_dv_modify_hdr_resource *mhdr_res =
> &mhdr_dummy.res;
> union flow_dv_attr flow_attr = { .attr = 0 };
> uint32_t tag_be;
> union mlx5_flow_tbl_key tbl_key;
> @@ -6754,15 +6760,19 @@ struct field_modify_info modify_tcp[] = {
> uint32_t table;
> int ret = 0;
>
> + mhdr_res->ft_type = attr->egress ?
> MLX5DV_FLOW_TABLE_TYPE_NIC_TX :
> +
> MLX5DV_FLOW_TABLE_TYPE_NIC_RX;
> ret = mlx5_flow_group_to_table(attr, dev_flow->external, attr-
> >group,
> &table, error);
> if (ret)
> return ret;
> dev_flow->group = table;
> if (attr->transfer)
> - mhdr_res.ft_type = MLX5DV_FLOW_TABLE_TYPE_FDB;
> + mhdr_res->ft_type = MLX5DV_FLOW_TABLE_TYPE_FDB;
> if (priority == MLX5_FLOW_PRIO_RSVD)
> priority = dev_conf->flow_prio - 1;
> + /* number of actions must be set to 0 in case of dirty stack. */
> + mhdr_res->actions_num = 0;
> for (; !actions_end ; actions++) {
> const struct rte_flow_action_queue *queue;
> const struct rte_flow_action_rss *rss;
> @@ -6800,7 +6810,7 @@ struct field_modify_info modify_tcp[] = {
> };
>
> if (flow_dv_convert_action_mark(dev,
> &mark,
> - &mhdr_res,
> + mhdr_res,
> error))
> return -rte_errno;
> action_flags |=
> MLX5_FLOW_ACTION_MARK_EXT;
> @@ -6822,7 +6832,7 @@ struct field_modify_info modify_tcp[] = {
> actions->conf;
>
> if (flow_dv_convert_action_mark(dev, mark,
> - &mhdr_res,
> + mhdr_res,
> error))
> return -rte_errno;
> action_flags |=
> MLX5_FLOW_ACTION_MARK_EXT;
> @@ -6843,7 +6853,7 @@ struct field_modify_info modify_tcp[] = {
> break;
> case RTE_FLOW_ACTION_TYPE_SET_META:
> if (flow_dv_convert_action_set_meta
> - (dev, &mhdr_res, attr,
> + (dev, mhdr_res, attr,
> (const struct rte_flow_action_set_meta *)
> actions->conf, error))
> return -rte_errno;
> @@ -6851,7 +6861,7 @@ struct field_modify_info modify_tcp[] = {
> break;
> case RTE_FLOW_ACTION_TYPE_SET_TAG:
> if (flow_dv_convert_action_set_tag
> - (dev, &mhdr_res,
> + (dev, mhdr_res,
> (const struct rte_flow_action_set_tag *)
> actions->conf, error))
> return -rte_errno;
> @@ -6951,7 +6961,7 @@ struct field_modify_info modify_tcp[] = {
> mlx5_update_vlan_vid_pcp(actions, &vlan);
> /* If no VLAN push - this is a modify header action */
> if (flow_dv_convert_action_modify_vlan_vid
> - (&mhdr_res, actions, error))
> + (mhdr_res, actions, error))
> return -rte_errno;
> action_flags |=
> MLX5_FLOW_ACTION_OF_SET_VLAN_VID;
> break;
> @@ -7050,7 +7060,7 @@ struct field_modify_info modify_tcp[] = {
> case RTE_FLOW_ACTION_TYPE_SET_MAC_SRC:
> case RTE_FLOW_ACTION_TYPE_SET_MAC_DST:
> if (flow_dv_convert_action_modify_mac
> - (&mhdr_res, actions, error))
> + (mhdr_res, actions, error))
> return -rte_errno;
> action_flags |= actions->type ==
>
> RTE_FLOW_ACTION_TYPE_SET_MAC_SRC ?
> @@ -7060,7 +7070,7 @@ struct field_modify_info modify_tcp[] = {
> case RTE_FLOW_ACTION_TYPE_SET_IPV4_SRC:
> case RTE_FLOW_ACTION_TYPE_SET_IPV4_DST:
> if (flow_dv_convert_action_modify_ipv4
> - (&mhdr_res, actions, error))
> + (mhdr_res, actions, error))
> return -rte_errno;
> action_flags |= actions->type ==
>
> RTE_FLOW_ACTION_TYPE_SET_IPV4_SRC ?
> @@ -7070,7 +7080,7 @@ struct field_modify_info modify_tcp[] = {
> case RTE_FLOW_ACTION_TYPE_SET_IPV6_SRC:
> case RTE_FLOW_ACTION_TYPE_SET_IPV6_DST:
> if (flow_dv_convert_action_modify_ipv6
> - (&mhdr_res, actions, error))
> + (mhdr_res, actions, error))
> return -rte_errno;
> action_flags |= actions->type ==
>
> RTE_FLOW_ACTION_TYPE_SET_IPV6_SRC ?
> @@ -7080,7 +7090,7 @@ struct field_modify_info modify_tcp[] = {
> case RTE_FLOW_ACTION_TYPE_SET_TP_SRC:
> case RTE_FLOW_ACTION_TYPE_SET_TP_DST:
> if (flow_dv_convert_action_modify_tp
> - (&mhdr_res, actions, items,
> + (mhdr_res, actions, items,
> &flow_attr, error))
> return -rte_errno;
> action_flags |= actions->type ==
> @@ -7090,13 +7100,13 @@ struct field_modify_info modify_tcp[] = {
> break;
> case RTE_FLOW_ACTION_TYPE_DEC_TTL:
> if (flow_dv_convert_action_modify_dec_ttl
> - (&mhdr_res, items, &flow_attr,
> error))
> + (mhdr_res, items, &flow_attr, error))
> return -rte_errno;
> action_flags |= MLX5_FLOW_ACTION_DEC_TTL;
> break;
> case RTE_FLOW_ACTION_TYPE_SET_TTL:
> if (flow_dv_convert_action_modify_ttl
> - (&mhdr_res, actions, items,
> + (mhdr_res, actions, items,
> &flow_attr, error))
> return -rte_errno;
> action_flags |= MLX5_FLOW_ACTION_SET_TTL;
> @@ -7104,7 +7114,7 @@ struct field_modify_info modify_tcp[] = {
> case RTE_FLOW_ACTION_TYPE_INC_TCP_SEQ:
> case RTE_FLOW_ACTION_TYPE_DEC_TCP_SEQ:
> if (flow_dv_convert_action_modify_tcp_seq
> - (&mhdr_res, actions, error))
> + (mhdr_res, actions, error))
> return -rte_errno;
> action_flags |= actions->type ==
>
> RTE_FLOW_ACTION_TYPE_INC_TCP_SEQ ?
> @@ -7115,7 +7125,7 @@ struct field_modify_info modify_tcp[] = {
> case RTE_FLOW_ACTION_TYPE_INC_TCP_ACK:
> case RTE_FLOW_ACTION_TYPE_DEC_TCP_ACK:
> if (flow_dv_convert_action_modify_tcp_ack
> - (&mhdr_res, actions, error))
> + (mhdr_res, actions, error))
> return -rte_errno;
> action_flags |= actions->type ==
>
> RTE_FLOW_ACTION_TYPE_INC_TCP_ACK ?
> @@ -7124,13 +7134,13 @@ struct field_modify_info modify_tcp[] = {
> break;
> case MLX5_RTE_FLOW_ACTION_TYPE_TAG:
> if (flow_dv_convert_action_set_reg
> - (&mhdr_res, actions, error))
> + (mhdr_res, actions, error))
> return -rte_errno;
> action_flags |= MLX5_FLOW_ACTION_SET_TAG;
> break;
> case MLX5_RTE_FLOW_ACTION_TYPE_COPY_MREG:
> if (flow_dv_convert_action_copy_mreg
> - (dev, &mhdr_res, actions, error))
> + (dev, mhdr_res, actions, error))
> return -rte_errno;
> action_flags |= MLX5_FLOW_ACTION_SET_TAG;
> break;
> @@ -7155,10 +7165,10 @@ struct field_modify_info modify_tcp[] = {
> break;
> case RTE_FLOW_ACTION_TYPE_END:
> actions_end = true;
> - if (mhdr_res.actions_num) {
> + if (mhdr_res->actions_num) {
> /* create modify action if needed. */
> if (flow_dv_modify_hdr_resource_register
> - (dev, &mhdr_res, dev_flow, error))
> + (dev, mhdr_res, dev_flow, error))
> return -rte_errno;
> dev_flow-
> >dv.actions[modify_action_position] =
> dev_flow->dv.modify_hdr-
> >verbs_action;
> @@ -7167,7 +7177,7 @@ struct field_modify_info modify_tcp[] = {
> default:
> break;
> }
> - if (mhdr_res.actions_num &&
> + if (mhdr_res->actions_num &&
> modify_action_position == UINT32_MAX)
> modify_action_position = actions_n++;
> }
> --
> 1.8.3.1
