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

[Suanming Mou <suanmingm@nvidia.com>]

Hi,

> -----Original Message-----
> From: Bing Zhao <bingz@nvidia.com>
> Sent: Monday, November 13, 2023 9:51 PM
> To: Suanming Mou <suanmingm@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 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.

OK, got it. Then no need for v2.  Thanks.

> 
> >
> >
> > > +		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