Thanks, PSB.
> -----Original Message-----
> From: Yongseok Koh
> Sent: Wednesday, October 3, 2018 12:33 PM
> To: Dekel Peled <dek...@mellanox.com>
> Cc: dev@dpdk.org; Shahaf Shuler <shah...@mellanox.com>; Ori Kam
> <or...@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 <dek...@mellanox.com>
> > > Cc: dev@dpdk.org; Shahaf Shuler <shah...@mellanox.com>; Ori Kam
> > > <or...@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 <dek...@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
