Re: [dpdk-stable] [PATCH 19.11] net/mlx5: fix drop action for Direct Rules/Verbs

[Christian Ehrhardt <christian.ehrhardt@canonical.com>]

On Wed, May 19, 2021 at 7:08 PM Viacheslav Ovsiienko
<viacheslavo@nvidia.com> wrote:
>
> [ upstream commit da845ae9d7c1499cbf76e766604cc981ddd9eb17 ]
>
> There are multiple branches in rdma-core library backing
> the rte flows:
>   - Verbs
>   - Direct Verbs (DV)
>   - Direct Rules (DR)

Thanks for the backport, applied to 19.11.9 now (soon there should be
an -rc2 with it)

> The Verbs API always requires the specifying the queue even
> if there is the drop action in the flow, though the kernel
> optimizes out the actual queue usage for the flows containing
> the drop action. The PMD handles the dedicated Rx queue to
> provide Verbs API compatibility.
>
> The DV/DR API does not require explicit specifying the queue
> at the flow creation, but PMD still specified the dedicated
> drop queue as action. It performed the packet forwarding to
> the dummy queue (that was not polled at all) causing the
> steering pipeline resources usage and degrading the overall
> packet processing rate. For example, with inserted flow to
> drop all the ingress packets the statistics reported only
> 15Mpps of 64B packets were received over 100Gbps line.
>
> Since the Direct Rule API for E-Switch was introduced the
> rdma-core supports the dedicated drop action, that is recognized
> both for DV and DR and can be used for the entire device in
> unified fashion, regardless of steering domain. The similar drop
> action was introduced for E-Switch, the usage of this one can be
> extended for other steering domains, not for E-Switch's one only.
>
> This patch:
>   - renames esw_drop_action to dr_drop_action to emphasize
>     the global nature of the variable (not only E-Switch domain)
>   - specifies this global drop action instead of dedicated
>     drop queue for the DR/DV flows
>
> Fixes: 34fa7c0268e7 ("net/mlx5: add drop action to Direct Verbs E-Switch")
> Cc: stable@dpdk.org
>
> Signed-off-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
> Acked-by: Matan Azrad <matan@nvidia.com>
> ---
>  drivers/net/mlx5/mlx5.c         | 24 +++++++++++++++++-------
>  drivers/net/mlx5/mlx5.h         |  2 +-
>  drivers/net/mlx5/mlx5_flow_dv.c | 10 +++++++++-
>  3 files changed, 27 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c
> index 8c5e27042..e0ba60086 100644
> --- a/drivers/net/mlx5/mlx5.c
> +++ b/drivers/net/mlx5/mlx5.c
> @@ -998,7 +998,17 @@ mlx5_alloc_shared_dr(struct mlx5_priv *priv)
>                         goto error;
>                 }
>                 sh->fdb_domain = domain;
> -               sh->esw_drop_action = mlx5_glue->dr_create_flow_action_drop();
> +       }
> +       /*
> +        * The drop action is just some dummy placeholder in rdma-core. It
> +        * does not belong to domains and has no any attributes, and, can be
> +        * shared by the entire device.
> +        */
> +       sh->dr_drop_action = mlx5_glue->dr_create_flow_action_drop();
> +       if (!sh->dr_drop_action) {
> +               DRV_LOG(ERR, "FDB mlx5dv_dr_create_flow_action_drop");
> +               err = errno;
> +               goto error;
>         }
>  #endif
>         sh->pop_vlan_action = mlx5_glue->dr_create_flow_action_pop_vlan();
> @@ -1018,9 +1028,9 @@ mlx5_alloc_shared_dr(struct mlx5_priv *priv)
>                 mlx5_glue->dr_destroy_domain(sh->fdb_domain);
>                 sh->fdb_domain = NULL;
>         }
> -       if (sh->esw_drop_action) {
> -               mlx5_glue->destroy_flow_action(sh->esw_drop_action);
> -               sh->esw_drop_action = NULL;
> +       if (sh->dr_drop_action) {
> +               mlx5_glue->destroy_flow_action(sh->dr_drop_action);
> +               sh->dr_drop_action = NULL;
>         }
>         if (sh->pop_vlan_action) {
>                 mlx5_glue->destroy_flow_action(sh->pop_vlan_action);
> @@ -1063,9 +1073,9 @@ mlx5_free_shared_dr(struct mlx5_priv *priv)
>                 mlx5_glue->dr_destroy_domain(sh->fdb_domain);
>                 sh->fdb_domain = NULL;
>         }
> -       if (sh->esw_drop_action) {
> -               mlx5_glue->destroy_flow_action(sh->esw_drop_action);
> -               sh->esw_drop_action = NULL;
> +       if (sh->dr_drop_action) {
> +               mlx5_glue->destroy_flow_action(sh->dr_drop_action);
> +               sh->dr_drop_action = NULL;
>         }
>  #endif
>         if (sh->pop_vlan_action) {
> diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h
> index f3b6ece19..70509959f 100644
> --- a/drivers/net/mlx5/mlx5.h
> +++ b/drivers/net/mlx5/mlx5.h
> @@ -679,7 +679,7 @@ struct mlx5_ibv_shared {
>  #endif
>         struct mlx5_hlist *flow_tbls;
>         /* Direct Rules tables for FDB, NIC TX+RX */
> -       void *esw_drop_action; /* Pointer to DR E-Switch drop action. */
> +       void *dr_drop_action; /* Pointer to DR drop action, any domain. */
>         void *pop_vlan_action; /* Pointer to DR pop VLAN action. */
>         LIST_HEAD(encap_decap, mlx5_flow_dv_encap_decap_resource) encaps_decaps;
>         LIST_HEAD(modify_cmd, mlx5_flow_dv_modify_hdr_resource) modify_cmds;
> diff --git a/drivers/net/mlx5/mlx5_flow_dv.c b/drivers/net/mlx5/mlx5_flow_dv.c
> index 6dc4e680e..cea45a6d9 100644
> --- a/drivers/net/mlx5/mlx5_flow_dv.c
> +++ b/drivers/net/mlx5/mlx5_flow_dv.c
> @@ -7918,8 +7918,15 @@ __flow_dv_apply(struct rte_eth_dev *dev, struct rte_flow *flow,
>                 n = dv->actions_n;
>                 if (dev_flow->actions & MLX5_FLOW_ACTION_DROP) {
>                         if (dev_flow->transfer) {
> -                               dv->actions[n++] = priv->sh->esw_drop_action;
> +                               assert(priv->sh->dr_drop_action);
> +                               dv->actions[n++] = priv->sh->dr_drop_action;
>                         } else {
> +#ifdef HAVE_MLX5DV_DR
> +                               /* DR supports drop action placeholder. */
> +                               assert(priv->sh->dr_drop_action);
> +                               dv->actions[n++] = priv->sh->dr_drop_action;
> +#else
> +                               /* For DV we use the explicit drop queue. */
>                                 dv->hrxq = mlx5_hrxq_drop_new(dev);
>                                 if (!dv->hrxq) {
>                                         rte_flow_error_set
> @@ -7930,6 +7937,7 @@ __flow_dv_apply(struct rte_eth_dev *dev, struct rte_flow *flow,
>                                         goto error;
>                                 }
>                                 dv->actions[n++] = dv->hrxq->action;
> +#endif
>                         }
>                 } else if (dev_flow->actions &
>                            (MLX5_FLOW_ACTION_QUEUE | MLX5_FLOW_ACTION_RSS)) {
> --
> 2.18.1
>


-- 
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd