RE: [PATCH] net/mlx5: fix the LACP redirection in Rx domain

[Bing Zhao <bingz@nvidia.com>]

Hi Suanming,

My answers are inline.

> -----Original Message-----
> From: Suanming Mou <suanmingm@nvidia.com>
> Sent: Monday, November 13, 2023 9:14 PM
> To: Bing Zhao <bingz@nvidia.com>; Matan Azrad <matan@nvidia.com>; Slava
> Ovsiienko <viacheslavo@nvidia.com>; Raslan Darawsheh
> <rasland@nvidia.com>; Ori Kam <orika@nvidia.com>
> Cc: dev@dpdk.org; Dariusz Sosnowski <dsosnowski@nvidia.com>;
> stable@dpdk.org
> Subject: RE: [PATCH] net/mlx5: fix the LACP redirection in Rx domain
> 
> Hi,
> 
> One small comment, v2 is needed.
> 
> > -----Original Message-----
> > From: Bing Zhao <bingz@nvidia.com>
> > Sent: Monday, November 13, 2023 8:42 PM
> > To: Matan Azrad <matan@nvidia.com>; Slava Ovsiienko
> > <viacheslavo@nvidia.com>; Raslan Darawsheh <rasland@nvidia.com>;
> > Suanming Mou <suanmingm@nvidia.com>; Ori Kam <orika@nvidia.com>
> > Cc: dev@dpdk.org; Dariusz Sosnowski <dsosnowski@nvidia.com>;
> > stable@dpdk.org
> > Subject: [PATCH] net/mlx5: fix the LACP redirection in Rx domain
> >
> > When the "lacp_by_user" is not set from the application in bond mode,
> > the LACP traffic should be handled by the kernel driver by default.
> >
> > This commit adds the missing support in the template API when
> "dv_flow_en=2".
> > The behavior will be the same as that in the DV mode with
> > "dv_flow_en=1". The LACP packets will be redirected to the kernel when
> > starting the steering in the NIC Rx domain.
> >
> > With this commit, the DEFAULT_MISS action usage is refactored a bit.
> > In the HWS, one unique action can be created with supported bits set in the
> "flag"
> > per port. The *ROOT_FDB and *HWS_FDB flag bits will only be set when
> > the port is in switchdev mode and working as the E-Switch manager
> > proxy port. The SF/VF and all other representors won't have the FDB
> > flag bits when creating the DEFAULT_MISS action.
> >
> > Fixes: 9fa7c1cddb85 ("net/mlx5: create control flow rules with HWS")
> > Cc: dsosnowski@nvidia.com
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Bing Zhao <bingz@nvidia.com>
> > ---
> >  drivers/net/mlx5/linux/mlx5_os.c |   8 +-
> >  drivers/net/mlx5/mlx5.h          |   1 +
> >  drivers/net/mlx5/mlx5_flow.h     |   1 +
> >  drivers/net/mlx5/mlx5_flow_hw.c  | 249
> +++++++++++++++++++++++++++++--
> >  drivers/net/mlx5/mlx5_trigger.c  |   3 +
> >  5 files changed, 249 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/net/mlx5/linux/mlx5_os.c
> > b/drivers/net/mlx5/linux/mlx5_os.c
> 
> [snip]
> 
> > @@ -8327,6 +8503,10 @@ flow_hw_create_ctrl_tables(struct rte_eth_dev
> > *dev, struct rte_flow_error *error
> >  		ret = rte_flow_error_set(error, EINVAL,
> >
> > RTE_FLOW_ERROR_TYPE_UNSPECIFIED, NULL,
> >  					 "Failed to create control tables.");
> > +	if (priv->hw_tx_meta_cpy_tbl) {
> 
> I assume it should be hw_lacp_rx_tbl here.  Can you please send v2 for that?
> And LGTM for left code.  It's OK to add my ack for v2.
> Acked-by: Suanming Mou <suanmingm@nvidia.com>

Since it is a "goto" label, when arriving here from the LACP error branch, the previous tables were created successfully.
The LACP table is the last one from the code and it will never be NULL when reaching here. What should be rollbaked is the previous one.

This is some disadvantage of using goto in the code, not quite straightforward.

> 
> 
> > +		flow_hw_table_destroy(dev, priv->hw_tx_meta_cpy_tbl,
> NULL);
> > +		priv->hw_tx_meta_cpy_tbl = NULL;
> > +	}
> 
> [snip]
> 
> > 2.34.1

BR. Bing