From mboxrd@z Thu Jan 1 00:00:00 1970 From: Yongseok Koh Subject: Re: [PATCH 3/4] net/mlx5: add DV encap L2 and L3 operations Date: Wed, 3 Oct 2018 09:32:54 +0000 Message-ID: <20181003093245.GC21743@mtidpdk.mti.labs.mlnx> References: <1538059845-35896-1-git-send-email-dekelp@mellanox.com> <1538059845-35896-4-git-send-email-dekelp@mellanox.com> <20181003065817.GD10028@mtidpdk.mti.labs.mlnx> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Cc: "dev@dpdk.org" , Shahaf Shuler , Ori Kam To: Dekel Peled Return-path: Received: from EUR02-AM5-obe.outbound.protection.outlook.com (mail-eopbgr00073.outbound.protection.outlook.com [40.107.0.73]) by dpdk.org (Postfix) with ESMTP id 7831E695D for ; Wed, 3 Oct 2018 11:32:55 +0200 (CEST) In-Reply-To: Content-Language: en-US Content-ID: <5DF1D9262480CD46A4148F5E0A38A846@eurprd05.prod.outlook.com> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Wed, Oct 03, 2018 at 01:35:16AM -0700, Dekel Peled wrote: > Thanks, PSB. >=20 > > -----Original Message----- > > From: Yongseok Koh > > Sent: Wednesday, October 3, 2018 9:58 AM > > To: Dekel Peled > > Cc: dev@dpdk.org; Shahaf Shuler ; Ori Kam > > > > Subject: Re: [PATCH 3/4] net/mlx5: add DV encap L2 and L3 operations > >=20 > > 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 > > > --- > > > 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 =3D NULL; > > > + const struct rte_flow_action_tunnel_encap *encap_data; > > > + > > > + encap_data =3D (const struct rte_flow_action_tunnel_encap *)action- > > >conf; > > > + encap_verb =3D 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); > >=20 > > Indentation. >=20 > 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 =3D 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, > >=20 > > If it is just priv->ctx, it would be better to get dev as an arg and ma= ke > > mlx5_flow_dv_create_encap*(dev, ...) gets priv->ctx from dev. >=20 > I considered it during implementation, but preferred to give the function= s 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