Hi Gregory,
> -----Original Message-----
> From: Gregory Etelson <getel...@nvidia.com>
> Sent: Friday, February 2, 2024 16:06
> To: dev@dpdk.org
> Cc: Gregory Etelson <getel...@nvidia.com>; Maayan Kashani
> <mkash...@nvidia.com>; Dariusz Sosnowski <dsosnow...@nvidia.com>;
> Slava Ovsiienko <viachesl...@nvidia.com>; Ori Kam <or...@nvidia.com>;
> Suanming Mou <suanmi...@nvidia.com>; Matan Azrad
> <ma...@nvidia.com>
> Subject: [PATCH 2/2] net/mlx5: improve pattern template validation
>
> Current PMD implementation validates pattern templates that will always be
> rejected during table template creation.
>
> The patch adds basic HWS verifications to pattern validation to ensure that
> the
> pattern can be used in table template.
>
> PMD updates `rte_errno` if pattern template validation failed:
>
> E2BIG - pattern too big for PMD
> ENOTSUP - pattern not supported by PMD
> ENOMEM - PMD allocation failure
>
> Signed-off-by: Gregory Etelson <getel...@nvidia.com>
>
> [snip]
>
> --git a/drivers/net/mlx5/mlx5_flow_hw.c b/drivers/net/mlx5/mlx5_flow_hw.c
> index da873ae2e2..443aa5fcf0 100644
> --- a/drivers/net/mlx5/mlx5_flow_hw.c
> +++ b/drivers/net/mlx5/mlx5_flow_hw.c
> @@ -6840,6 +6840,46 @@ flow_hw_pattern_has_sq_match(const struct
> rte_flow_item *items)
> return false;
> }
>
> +static int
> +pattern_template_validate(struct rte_eth_dev *dev,
> + struct rte_flow_pattern_template *pt[], uint32_t
> pt_num) {
> + uint32_t group = 0;
> + struct rte_flow_error error;
> + struct rte_flow_template_table_attr tbl_attr = {
> + .nb_flows = 64,
> + .insertion_type =
> RTE_FLOW_TABLE_INSERTION_TYPE_PATTERN,
> + .hash_func = RTE_FLOW_TABLE_HASH_FUNC_DEFAULT,
> + .flow_attr = {
> + .ingress = pt[0]->attr.ingress,
> + .egress = pt[0]->attr.egress,
> + .transfer = pt[0]->attr.transfer
> + }
> + };
> + struct mlx5_priv *priv = dev->data->dev_private;
> + struct rte_flow_actions_template *action_template;
> +
> + if (pt[0]->attr.ingress)
> + action_template = priv-
> >action_template_drop[MLX5DR_TABLE_TYPE_NIC_RX];
> + else if (pt[0]->attr.egress)
> + action_template = priv-
> >action_template_drop[MLX5DR_TABLE_TYPE_NIC_TX];
> + else if (pt[0]->attr.transfer)
> + action_template = priv-
> >action_template_drop[MLX5DR_TABLE_TYPE_FDB];
> + else
> + return EINVAL;
> + do {
> + struct rte_flow_template_table *tmpl_tbl;
> +
> + tbl_attr.flow_attr.group = group;
> + tmpl_tbl = flow_hw_template_table_create(dev, &tbl_attr, pt,
> pt_num,
flow_hw_table_create() should be called here.
If E-Switch is enabled, flow_hw_template_table_create() will perform internal
group translation for FDB and TX domain,
so group 0 will be untested.
> + &action_template, 1,
> NULL);
> + if (!tmpl_tbl)
> + return rte_errno;
> + flow_hw_table_destroy(dev, tmpl_tbl, &error);
I don't think that passing error struct is needed here, since this error is not
propagated up.
> [snip]
>
> @@ -9184,6 +9235,66 @@ flow_hw_compare_config(const struct
> mlx5_flow_hw_attr *hw_attr,
> return true;
> }
>
> +/*
> + * No need to explicitly release drop action templates on port stop.
> + * Drop action templates release with other action templates during
> + * mlx5_dev_close -> flow_hw_resource_release ->
> +flow_hw_actions_template_destroy */ static void
> +action_template_drop_release(struct rte_eth_dev *dev) {
> + int i;
> + struct mlx5_priv *priv = dev->data->dev_private;
> +
> + for (i = 0; i < MLX5DR_TABLE_TYPE_MAX; i++) {
> + if (!priv->action_template_drop[i])
> + continue;
> + flow_hw_actions_template_destroy(dev,
> + priv-
> >action_template_drop[i],
> + NULL);
> + }
I'd suggest adding zeroing action_template_drop array entries here.
In case of failure inside rte_flow_configure(), rollback code called on error
must
reset the affected fields in private data to allow safe call to
rte_flow_configure() again.
> [snip]
>
> @@ -9621,10 +9735,7 @@ flow_hw_resource_release(struct rte_eth_dev
> *dev)
> flow_hw_flush_all_ctrl_flows(dev);
> flow_hw_cleanup_tx_repr_tagging(dev);
> flow_hw_cleanup_ctrl_rx_tables(dev);
> - while (!LIST_EMPTY(&priv->flow_hw_grp)) {
> - grp = LIST_FIRST(&priv->flow_hw_grp);
> - flow_hw_group_unset_miss_group(dev, grp, NULL);
> - }
> + action_template_drop_release(dev);
Why is the miss actions cleanup code removed? It does not seem related to the
patch.
Best regards,
Dariusz Sosnowski
