Re: [PATCH 3/4] net/mlx5: add DV encap L2 and L3 operations

[Dekel Peled <dekelp@mellanox.com>]

Thanks, PSB.

> -----Original Message-----
> From: Yongseok Koh
> Sent: Wednesday, October 3, 2018 12:33 PM
> To: Dekel Peled <dekelp@mellanox.com>
> Cc: dev@dpdk.org; Shahaf Shuler <shahafs@mellanox.com>; Ori Kam
> <orika@mellanox.com>
> Subject: Re: [PATCH 3/4] net/mlx5: add DV encap L2 and L3 operations
> 
> On Wed, Oct 03, 2018 at 01:35:16AM -0700, Dekel Peled wrote:
> > Thanks, PSB.
> >
> > > -----Original Message-----
> > > From: Yongseok Koh
> > > Sent: Wednesday, October 3, 2018 9:58 AM
> > > To: Dekel Peled <dekelp@mellanox.com>
> > > Cc: dev@dpdk.org; Shahaf Shuler <shahafs@mellanox.com>; Ori Kam
> > > <orika@mellanox.com>
> > > Subject: Re: [PATCH 3/4] net/mlx5: add DV encap L2 and L3 operations
> > >
> > > On Thu, Sep 27, 2018 at 05:50:44PM +0300, Dekel Peled wrote:
> > > > This patch adds support for Direct Verbs encap operations, L2 and L3.
> > > >
> > > > Signed-off-by: Dekel Peled <dekelp@mellanox.com>
> > > > ---
> > > >  drivers/net/mlx5/mlx5_flow_dv.c | 249
> > > > +++++++++++++++++++++++++++++++++++++++-
> > > >  1 file changed, 244 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/drivers/net/mlx5/mlx5_flow_dv.c
> > > > b/drivers/net/mlx5/mlx5_flow_dv.c index 1f3fcb8..50925ac 100644
> > > > --- a/drivers/net/mlx5/mlx5_flow_dv.c
> > > > +++ b/drivers/net/mlx5/mlx5_flow_dv.c
> [...]
> > > > +			  struct ibv_context *ctx,
> > > > +			  struct rte_flow_error *error) {
> > > > +	struct ibv_flow_action *encap_verb = NULL;
> > > > +	const struct rte_flow_action_tunnel_encap *encap_data;
> > > > +
> > > > +	encap_data = (const struct rte_flow_action_tunnel_encap
> > > > +*)action-
> > > >conf;
> > > > +	encap_verb = mlx5_glue-
> > > >dv_create_flow_action_packet_reformat(ctx,
> > > > +			encap_data->size,
> > > > +			encap_data->size ? encap_data->buf :
> > > > +					   NULL,
> > > > +
> > > 	MLX5DV_FLOW_ACTION_PACKET_REFORMAT_TYPE_L2_TO_L2_TU
> > > NNEL,
> > > > +			MLX5DV_FLOW_TABLE_TYPE_NIC_TX);
> > >
> > > Indentation.
> >
> > I'm using very long MLX5DV_... names defined in rdma-core.
> > If I use the required indentation I get illegal line length.
> 
> The following was my suggestion and it is compliant.
> 
> 	encap_verb = mlx5_glue->dv_create_flow_action_packet_reformat
> 		(ctx, encap_data->size,
> 		 encap_data->size ? encap_data->buf : NULL,
> 
> MLX5DV_FLOW_ACTION_PACKET_REFORMAT_TYPE_L2_TO_L2_TUNNEL,
> 		 MLX5DV_FLOW_TABLE_TYPE_NIC_TX);
> 
> Please make the same change to others.

Done.

> 
> [...]
> > > > @@ -1047,10 +1239,19 @@
> > > >   *   Flow action to translate.
> > > >   * @param[in, out] dev_flow
> > > >   *   Pointer to the mlx5_flow.
> > > > + * @param[in] ctx
> > > > + *   Verbs context.
> > > > + * @param[out] error
> > > > + *   Pointer to the error structure.
> > > > + *
> > > > + * @return
> > > > + *   0 on success, a negative errno value otherwise and rte_ernno is
> set.
> > > >   */
> > > > -static void
> > > > +static int
> > > >  flow_dv_create_action(const struct rte_flow_action *action,
> > > > -		      struct mlx5_flow *dev_flow)
> > > > +		      struct mlx5_flow *dev_flow,
> > > > +		      struct ibv_context *ctx,
> > >
> > > If it is just priv->ctx, it would be better to get dev as an arg and
> > > make mlx5_flow_dv_create_encap*(dev, ...) gets priv->ctx from dev.
> >
> > I considered it during implementation, but preferred to give the functions
> only what they need.
> 
> Two reasons.
> 
> 1) having dev gets better matched with other existing ones. E.g.,
> flow_dv_matcher_register() takes dev and it refers to priv->matchers and
> priv->ctx.
> 
> 2) extensibility. What if flow_dv_create_action() needs more fields of priv
> when adding another new action in the future?

Done.

> 
> Thanks,
> Yongseok