From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dekel Peled Subject: Re: [PATCH 3/4] net/mlx5: add DV encap L2 and L3 operations Date: Wed, 3 Oct 2018 08:35:16 +0000 Message-ID: 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: Yongseok Koh Return-path: Received: from EUR04-DB3-obe.outbound.protection.outlook.com (mail-eopbgr60073.outbound.protection.outlook.com [40.107.6.73]) by dpdk.org (Postfix) with ESMTP id 7AA515688 for ; Wed, 3 Oct 2018 10:35:18 +0200 (CEST) In-Reply-To: <20181003065817.GD10028@mtidpdk.mti.labs.mlnx> Content-Language: en-US List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Thanks, PSB. > -----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 > > @@ -85,6 +85,178 @@ > > } > > > > /** > > + * Validate the tunnel encap action. > > + * > > + * @param[in] action_flags > > + * holds the actions detected until now. > > + * @param[in] action > > + * Pointer to the encap action. > > + * @param[in] attr > > + * Pointer to flow attributes > > + * @param[out] error > > + * Pointer to error structure. > > + * > > + * @return > > + * 0 on success, a negative errno value otherwise and rte_ernno is s= et. > > + */ > > +static int > > +mlx5_flow_validate_action_tunnel_encap(uint64_t action_flags, >=20 > A static func in this file should start from flow_dv_. Or, you can move t= hese > to mlx5_flow.c as general validation helpers. I prefer the latter. Renamed functions to flow_dv_validate_action_tunnel_... Left it as static in mlx5_flow_dv.c, since it is relevant for DV only. >=20 > > + const struct rte_flow_action *action, > > + const struct rte_flow_attr *attr, > > + struct rte_flow_error *error) { > > + const struct rte_flow_action_tunnel_encap *tunnel_encap =3D > > +action->conf; > > + > > + if (!tunnel_encap || !(tunnel_encap->buf)) > > + return rte_flow_error_set(error, EINVAL, > > + RTE_FLOW_ERROR_TYPE_ACTION, > > + action, >=20 > Two lines above can be merged? Please check others and merge lines as > much as possible. Done. >=20 > > + "configuration cannot be null"); > > + if (action_flags & MLX5_ACTION_DROP) > > + return rte_flow_error_set(error, > > + ENOTSUP, > > + RTE_FLOW_ERROR_TYPE_ACTION, > > + NULL, > > + "can't drop and encap in same > flow"); > > + if (action_flags & (MLX5_ACTION_TUNNEL_ENCAP | > > + MLX5_ACTION_TUNNEL_ENCAP_L3)) > > + return rte_flow_error_set(error, > > + ENOTSUP, > > + RTE_FLOW_ERROR_TYPE_ACTION, > > + NULL, > > + "can't have 2 encap actions in same" > > + " flow"); > > + if (attr->ingress) > > + return rte_flow_error_set(error, ENOTSUP, > > + > RTE_FLOW_ERROR_TYPE_ATTR_INGRESS, > > + NULL, > > + "encap action not supported for " > > + "ingress"); > > + return 0; > > +} > > + > > +/** > > + * Validate the tunnel encap L3 action > > + * > > + * @param[in] action_flags > > + * holds the actions detected until now. > > + * @param[in] action > > + * Pointer to the encap action. > > + * @param[in] attr > > + * Pointer to flow attributes > > + * @param[out] error > > + * Pointer to error structure. > > + * > > + * @return > > + * 0 on success, a negative errno value otherwise and rte_ernno is s= et. > > + */ > > +static int > > +mlx5_flow_validate_action_tunnel_encap_l3(uint64_t action_flags, > > + const struct rte_flow_action *action, > > + const struct rte_flow_attr *attr, > > + struct rte_flow_error *error) > > +{ > > + const struct rte_flow_action_tunnel_encap_l3 *tunnel_encap_l3 =3D > > + action->conf; >=20 > Indentation. >=20 > const struct rte_flow_action_tunnel_encap_l3 *tunnel_encap_l3 =3D > action->conf; Done. >=20 > > + > > + if (!tunnel_encap_l3 || !(tunnel_encap_l3->buf)) > > + return rte_flow_error_set(error, EINVAL, > > + RTE_FLOW_ERROR_TYPE_ACTION, > > + action, > > + "configuration cannot be null"); > > + if (action_flags & MLX5_ACTION_DROP) > > + return rte_flow_error_set(error, > > + ENOTSUP, > > + RTE_FLOW_ERROR_TYPE_ACTION, > > + NULL, > > + "can't drop and encap in same > flow"); > > + if (action_flags & (MLX5_ACTION_TUNNEL_ENCAP | > > + MLX5_ACTION_TUNNEL_ENCAP_L3)) > > + return rte_flow_error_set(error, > > + ENOTSUP, > > + RTE_FLOW_ERROR_TYPE_ACTION, > > + NULL, > > + "can't have 2 encap actions in same" > > + " flow"); > > + if (attr->ingress) > > + return rte_flow_error_set(error, ENOTSUP, > > + > RTE_FLOW_ERROR_TYPE_ATTR_INGRESS, > > + NULL, > > + "encap action not supported for " > > + "ingress"); > > + return 0; > > +} > > + > > +/** > > + * Convert encap action to DV specification. > > + * > > + * @param[in] action > > + * Pointer to action structure. > > + * @param[in] ctx > > + * Verbs context. > > + * @param[out] error > > + * Pointer to the error structure. > > + * > > + * @return > > + * Pointer to action on success, NULL otherwise and rte_errno is set= . > > + */ > > +static struct ibv_flow_action * > > +mlx5_flow_dv_create_encap(const struct rte_flow_action *action, >=20 > Change to flow_dv_create_encap() Done. >=20 > > + 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. I'm using very long MLX5DV_... names defined in rdma-core. If I use the required indentation I get illegal line length. >=20 > encap_verb =3D mlx5_glue->dv_create_flow_action_packet_reformat > (ctx, encap_data->size, > encap_data->size ? encap_data->buf : NULL, >=20 > MLX5DV_FLOW_ACTION_PACKET_REFORMAT_TYPE_L2_TO_L2_TUNNEL, > MLX5DV_FLOW_TABLE_TYPE_NIC_TX); >=20 > Please make the same change to others. >=20 > > + if (!encap_verb) > > + rte_flow_error_set(error, EINVAL, > RTE_FLOW_ERROR_TYPE_ACTION, > > + NULL, "cannot create encap action"); > > + return encap_verb; > > +} > > + > > +/** > > + * Convert encap L3 action to DV specification. > > + * > > + * @param[in] action > > + * Pointer to action structure. > > + * @param[in] ctx > > + * Verbs context. > > + * @param[out] error > > + * Pointer to the error structure. > > + * > > + * @return > > + * Pointer to action on success, NULL otherwise and rte_errno is set= . > > + */ > > +static struct ibv_flow_action * > > +mlx5_flow_dv_create_encap_l3(const struct rte_flow_action *action, >=20 > Change to flow_dv_create_encap_l3() Done. >=20 > > + struct ibv_context *ctx, > > + struct rte_flow_error *error) { > > + struct ibv_flow_action *encap_l3_verb =3D NULL; > > + const struct rte_flow_action_tunnel_encap_l3 *encap_l3_data; > > + > > + encap_l3_data =3D > > + (const struct rte_flow_action_tunnel_encap_l3 *)action- > >conf; > > + encap_l3_verb =3D mlx5_glue- > >dv_create_flow_action_packet_reformat(ctx, > > + encap_l3_data->size, > > + encap_l3_data->size ? encap_l3_data->buf : > > + NULL, > > + > MLX5DV_FLOW_ACTION_PACKET_REFORMAT_TYPE_L2_TO_L3_TU > NNEL, > > + MLX5DV_FLOW_TABLE_TYPE_NIC_TX); > > + if (!encap_l3_verb) > > + rte_flow_error_set(error, EINVAL, > RTE_FLOW_ERROR_TYPE_ACTION, > > + NULL, "cannot create encap L3 action"); > > + return encap_l3_verb; > > +} > > + > > +/** > > * Verify the @p attributes will be correctly understood by the NIC an= d > store > > * them in the @p flow if everything is correct. > > * > > @@ -337,6 +509,26 @@ > > action_flags |=3D MLX5_ACTION_COUNT; > > ++actions_n; > > break; > > + case RTE_FLOW_ACTION_TYPE_TUNNEL_ENCAP: > > + ret =3D mlx5_flow_validate_action_tunnel_encap > > + (action_flags, > > + actions, > > + attr, > > + error); >=20 > Can be one line. Done. >=20 > > + if (ret < 0) > > + return ret; > > + action_flags |=3D MLX5_ACTION_TUNNEL_ENCAP; > > + break; > > + case RTE_FLOW_ACTION_TYPE_TUNNEL_ENCAP_L3: > > + ret =3D mlx5_flow_validate_action_tunnel_encap_l3 > > + (action_flags, > > + actions, > > + attr, > > + error); > > + if (ret < 0) > > + return ret; > > + action_flags |=3D MLX5_ACTION_TUNNEL_ENCAP_L3; > > + break; > > default: > > return rte_flow_error_set(error, ENOTSUP, > > > RTE_FLOW_ERROR_TYPE_ACTION, > > @@ -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 s= et. > > */ > > -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 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. >=20 > > + struct rte_flow_error *error) > > { > > const struct rte_flow_action_queue *queue; > > const struct rte_flow_action_rss *rss; @@ -1095,10 +1296,35 @@ > > /* Added to array only in apply since we need the QP */ > > flow->actions |=3D MLX5_ACTION_RSS; > > break; > > + case RTE_FLOW_ACTION_TYPE_TUNNEL_ENCAP: > > + dev_flow->dv.actions[actions_n].type =3D > > + MLX5DV_FLOW_ACTION_IBV_FLOW_ACTION; > > + dev_flow->dv.actions[actions_n].action =3D > > + mlx5_flow_dv_create_encap(action, ctx, error); > > + if (!(dev_flow->dv.actions[actions_n].action)) > > + return -rte_errno; > > + dev_flow->dv.encap_verb =3D > > + dev_flow->dv.actions[actions_n].action; > > + flow->actions |=3D MLX5_ACTION_TUNNEL_ENCAP; > > + actions_n++; > > + break; > > + case RTE_FLOW_ACTION_TYPE_TUNNEL_ENCAP_L3: > > + dev_flow->dv.actions[actions_n].type =3D > > + MLX5DV_FLOW_ACTION_IBV_FLOW_ACTION; > > + dev_flow->dv.actions[actions_n].action =3D > > + mlx5_flow_dv_create_encap_l3(action, ctx, error); > > + if (!(dev_flow->dv.actions[actions_n].action)) > > + return -rte_errno; > > + dev_flow->dv.encap_verb =3D > > + dev_flow->dv.actions[actions_n].action; > > + flow->actions |=3D MLX5_ACTION_TUNNEL_ENCAP_L3; > > + actions_n++; > > + break; > > default: > > break; > > } > > dev_flow->dv.actions_n =3D actions_n; > > + return 0; > > } > > > > static uint32_t matcher_zero[MLX5_ST_SZ_DW(fte_match_param)] =3D { 0 }= ; > > @@ -1247,6 +1473,7 @@ > > }; > > void *match_value =3D dev_flow->dv.value.buf; > > uint8_t inner =3D 0; > > + int retval =3D 0; > > > > if (priority =3D=3D MLX5_FLOW_PRIO_RSVD) > > priority =3D priv->config.flow_prio - 1; @@ -1262,9 +1489,14 > @@ > > matcher.egress =3D attr->egress; > > if (flow_dv_matcher_register(dev, &matcher, dev_flow, error)) > > return -rte_errno; > > - for (; actions->type !=3D RTE_FLOW_ACTION_TYPE_END; actions++) > > - flow_dv_create_action(actions, dev_flow); > > - return 0; > > + for (; actions->type !=3D RTE_FLOW_ACTION_TYPE_END; actions++) { > > + retval =3D > > + flow_dv_create_action(actions, dev_flow, priv->ctx, error); > > + if (retval) > > + break; >=20 > The following looks better matched? >=20 > if (flow_dv_create_action(dev, actions, dev_flow, error)) > return -rte_errno; >=20 > And leave 'return 0;' as is below. OK. >=20 > BTW, flow_dv_matcher_register() allocates mlx5_flow_dv_matcher or > increase the refcnt. Then, if it fails here, doesn't it need to clean tha= t up? >=20 > > + } > > + >=20 > No blank line is allowed. Please fix other occurrences too. Done. >=20 > > + return retval; > > } > > > > /** > > @@ -1447,6 +1679,13 @@ > > LIST_REMOVE(dev_flow, next); > > if (dev_flow->dv.matcher) > > flow_dv_matcher_release(dev, dev_flow- > >dv.matcher); > > + > > + if (dev_flow->dv.encap_verb) { > > + claim_zero(mlx5_glue->destroy_flow_action > > + (dev_flow->dv.encap_verb)); > > + dev_flow->dv.encap_verb =3D NULL; > > + } > > + > > rte_free(dev_flow); > > } > > } > > -- > > 1.8.3.1 > >