> -----Original Message-----
> From: Ori Kam <or...@mellanox.com>
> Sent: Monday, January 20, 2020 15:03
> To: Bing Zhao <bi...@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: RE: [PATCH v3] net/mlx5: fix modify actions support limitation
>
>
>
> > -----Original Message-----
> > From: Bing Zhao <bi...@mellanox.com>
> > Sent: Monday, January 20, 2020 11:43 AM
> > 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 v3] 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>
> > ---
> Acked-by: Ori Kam <or...@mellanox.com>
Acked-by: Viacheslav Ovsiienko <viachesl...@mellanox.com>
> Thanks,
> Ori
>
>
> > drivers/net/mlx5/mlx5_flow.h | 15 +++---
> > drivers/net/mlx5/mlx5_flow_dv.c | 108 ++++++++++++++++++++++---------
> > ---------
> > 2 files changed, 68 insertions(+), 55 deletions(-)
> >
> > diff --git a/drivers/net/mlx5/mlx5_flow.h
> > b/drivers/net/mlx5/mlx5_flow.h index e42c98a..2e94371 100644
> > --- a/drivers/net/mlx5/mlx5_flow.h
> > +++ b/drivers/net/mlx5/mlx5_flow.h
> > @@ -389,11 +389,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 { @@ -404,9 +407,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 c02517a..eec4c72 100644
> > --- a/drivers/net/mlx5/mlx5_flow_dv.c
> > +++ b/drivers/net/mlx5/mlx5_flow_dv.c
> > @@ -365,7 +365,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");
> > @@ -406,11 +406,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;
> > }
> >
> > @@ -571,7 +571,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");
> > @@ -904,7 +904,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");
> > @@ -916,10 +916,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;
> > }
> >
> > @@ -2334,7 +2330,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 &&
> > @@ -3445,21 +3440,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;
> > }
> >
> > /**
> > @@ -3618,8 +3619,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");
> @@ -3629,17 +3634,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));
> > @@ -3649,18 +3652,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);
> > @@ -6911,10 +6914,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;
> > @@ -6926,15 +6932,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; @@ -6972,7 +6982,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;
> > @@ -6994,7 +7004,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;
> > @@ -7015,7 +7025,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;
> > @@ -7023,7 +7033,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;
> > @@ -7123,7 +7133,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;
> > @@ -7222,7 +7232,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 ?
> > @@ -7232,7 +7242,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 ?
> > @@ -7242,7 +7252,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 ?
> > @@ -7252,7 +7262,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 ==
> > @@ -7262,13 +7272,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; @@ -
> 7276,7 +7286,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 ?
> > @@ -7287,7 +7297,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 ?
> > @@ -7296,13 +7306,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;
> > @@ -7326,23 +7336,23 @@ struct field_modify_info modify_tcp[] = {
> > action_flags |= MLX5_FLOW_ACTION_METER;
> > break;
> > case RTE_FLOW_ACTION_TYPE_SET_IPV4_DSCP:
> > - if
> > (flow_dv_convert_action_modify_ipv4_dscp(&mhdr_res,
> > + if
> > (flow_dv_convert_action_modify_ipv4_dscp(mhdr_res,
> > actions, error))
> > return -rte_errno;
> > action_flags |=
> > MLX5_FLOW_ACTION_SET_IPV4_DSCP;
> > break;
> > case RTE_FLOW_ACTION_TYPE_SET_IPV6_DSCP:
> > - if
> > (flow_dv_convert_action_modify_ipv6_dscp(&mhdr_res,
> > + if
> > (flow_dv_convert_action_modify_ipv6_dscp(mhdr_res,
> > actions, error))
> > return -rte_errno;
> > action_flags |=
> > MLX5_FLOW_ACTION_SET_IPV6_DSCP;
> > 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;
> > @@ -7351,7 +7361,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
