From: Ori Kam <orika@mellanox.com>
To: Bing Zhao <bingz@mellanox.com>,
Slava Ovsiienko <viacheslavo@mellanox.com>,
Raslan Darawsheh <rasland@mellanox.com>,
Matan Azrad <matan@mellanox.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>, "stable@dpdk.org" <stable@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH v3] net/mlx5: fix modify actions support limitation
Date: Mon, 20 Jan 2020 13:03:21 +0000 [thread overview]
Message-ID: <AM5PR0501MB25796C574096D0A04F6A2CA6DB320@AM5PR0501MB2579.eurprd05.prod.outlook.com> (raw)
In-Reply-To: <1579513387-138985-1-git-send-email-bingz@mellanox.com>
> -----Original Message-----
> From: Bing Zhao <bingz@mellanox.com>
> Sent: Monday, January 20, 2020 11:43 AM
> To: Ori Kam <orika@mellanox.com>; Slava Ovsiienko
> <viacheslavo@mellanox.com>; Raslan Darawsheh <rasland@mellanox.com>;
> Matan Azrad <matan@mellanox.com>
> Cc: dev@dpdk.org; stable@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 <bingz@mellanox.com>
> ---
Acked-by: Ori Kam <orika@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
next prev parent reply other threads:[~2020-01-20 13:03 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-01-06 6:31 [dpdk-dev] [PATCH] " Bing Zhao
2020-01-12 15:56 ` [dpdk-dev] [PATCH v2] " Bing Zhao
2020-01-16 14:34 ` Ori Kam
2020-01-19 16:21 ` Raslan Darawsheh
2020-01-20 9:43 ` [dpdk-dev] [PATCH v3] " Bing Zhao
2020-01-20 13:03 ` Ori Kam [this message]
2020-01-20 13:06 ` Slava Ovsiienko
2020-01-20 14:04 ` Raslan Darawsheh
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=AM5PR0501MB25796C574096D0A04F6A2CA6DB320@AM5PR0501MB2579.eurprd05.prod.outlook.com \
--to=orika@mellanox.com \
--cc=bingz@mellanox.com \
--cc=dev@dpdk.org \
--cc=matan@mellanox.com \
--cc=rasland@mellanox.com \
--cc=stable@dpdk.org \
--cc=viacheslavo@mellanox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).