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

[Yongseok Koh <yskoh@mellanox.com>]

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.

[...]
> > > @@ -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?

Thanks,
Yongseok