| Message ID | 1538059845-35896-4-git-send-email-dekelp@mellanox.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Shahaf Shuler |
| Headers | show |
| Series | support DV encap and decap actions | expand |
| Context | Check | Description |
|---|---|---|
| ci/checkpatch | success | coding style OK |
| ci/Intel-compilation | success | Compilation OK |
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 > @@ -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 set. > + */ > +static int > +mlx5_flow_validate_action_tunnel_encap(uint64_t action_flags, A static func in this file should start from flow_dv_. Or, you can move these to mlx5_flow.c as general validation helpers. I prefer the latter. > + 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 = action->conf; > + > + if (!tunnel_encap || !(tunnel_encap->buf)) > + return rte_flow_error_set(error, EINVAL, > + RTE_FLOW_ERROR_TYPE_ACTION, > + action, Two lines above can be merged? Please check others and merge lines as much as possible. > + "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 set. > + */ > +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 = > + action->conf; Indentation. const struct rte_flow_action_tunnel_encap_l3 *tunnel_encap_l3 = action->conf; > + > + 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, Change to flow_dv_create_encap() > + 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_TUNNEL, > + MLX5DV_FLOW_TABLE_TYPE_NIC_TX); Indentation. 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. > + 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, Change to flow_dv_create_encap_l3() > + struct ibv_context *ctx, > + struct rte_flow_error *error) > +{ > + struct ibv_flow_action *encap_l3_verb = NULL; > + const struct rte_flow_action_tunnel_encap_l3 *encap_l3_data; > + > + encap_l3_data = > + (const struct rte_flow_action_tunnel_encap_l3 *)action->conf; > + encap_l3_verb = 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_TUNNEL, > + 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 and store > * them in the @p flow if everything is correct. > * > @@ -337,6 +509,26 @@ > action_flags |= MLX5_ACTION_COUNT; > ++actions_n; > break; > + case RTE_FLOW_ACTION_TYPE_TUNNEL_ENCAP: > + ret = mlx5_flow_validate_action_tunnel_encap > + (action_flags, > + actions, > + attr, > + error); Can be one line. > + if (ret < 0) > + return ret; > + action_flags |= MLX5_ACTION_TUNNEL_ENCAP; > + break; > + case RTE_FLOW_ACTION_TYPE_TUNNEL_ENCAP_L3: > + ret = mlx5_flow_validate_action_tunnel_encap_l3 > + (action_flags, > + actions, > + attr, > + error); > + if (ret < 0) > + return ret; > + action_flags |= 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 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. > + 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 |= MLX5_ACTION_RSS; > break; > + case RTE_FLOW_ACTION_TYPE_TUNNEL_ENCAP: > + dev_flow->dv.actions[actions_n].type = > + MLX5DV_FLOW_ACTION_IBV_FLOW_ACTION; > + dev_flow->dv.actions[actions_n].action = > + mlx5_flow_dv_create_encap(action, ctx, error); > + if (!(dev_flow->dv.actions[actions_n].action)) > + return -rte_errno; > + dev_flow->dv.encap_verb = > + dev_flow->dv.actions[actions_n].action; > + flow->actions |= MLX5_ACTION_TUNNEL_ENCAP; > + actions_n++; > + break; > + case RTE_FLOW_ACTION_TYPE_TUNNEL_ENCAP_L3: > + dev_flow->dv.actions[actions_n].type = > + MLX5DV_FLOW_ACTION_IBV_FLOW_ACTION; > + dev_flow->dv.actions[actions_n].action = > + 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 = > + dev_flow->dv.actions[actions_n].action; > + flow->actions |= MLX5_ACTION_TUNNEL_ENCAP_L3; > + actions_n++; > + break; > default: > break; > } > dev_flow->dv.actions_n = actions_n; > + return 0; > } > > static uint32_t matcher_zero[MLX5_ST_SZ_DW(fte_match_param)] = { 0 }; > @@ -1247,6 +1473,7 @@ > }; > void *match_value = dev_flow->dv.value.buf; > uint8_t inner = 0; > + int retval = 0; > > if (priority == MLX5_FLOW_PRIO_RSVD) > priority = priv->config.flow_prio - 1; > @@ -1262,9 +1489,14 @@ > matcher.egress = attr->egress; > if (flow_dv_matcher_register(dev, &matcher, dev_flow, error)) > return -rte_errno; > - for (; actions->type != RTE_FLOW_ACTION_TYPE_END; actions++) > - flow_dv_create_action(actions, dev_flow); > - return 0; > + for (; actions->type != RTE_FLOW_ACTION_TYPE_END; actions++) { > + retval = > + flow_dv_create_action(actions, dev_flow, priv->ctx, error); > + if (retval) > + break; The following looks better matched? if (flow_dv_create_action(dev, actions, dev_flow, error)) return -rte_errno; And leave 'return 0;' as is below. 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 that up? > + } > + No blank line is allowed. Please fix other occurrences too. > + 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 = NULL; > + } > + > rte_free(dev_flow); > } > } > -- > 1.8.3.1 >
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 > > @@ -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 set. > > + */ > > +static int > > +mlx5_flow_validate_action_tunnel_encap(uint64_t action_flags, > > A static func in this file should start from flow_dv_. Or, you can move these > 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. > > > + 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 = > > +action->conf; > > + > > + if (!tunnel_encap || !(tunnel_encap->buf)) > > + return rte_flow_error_set(error, EINVAL, > > + RTE_FLOW_ERROR_TYPE_ACTION, > > + action, > > Two lines above can be merged? Please check others and merge lines as > much as possible. Done. > > > + "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 set. > > + */ > > +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 = > > + action->conf; > > Indentation. > > const struct rte_flow_action_tunnel_encap_l3 *tunnel_encap_l3 = > action->conf; Done. > > > + > > + 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, > > Change to flow_dv_create_encap() Done. > > > + 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. > > 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. > > > + 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, > > Change to flow_dv_create_encap_l3() Done. > > > + struct ibv_context *ctx, > > + struct rte_flow_error *error) { > > + struct ibv_flow_action *encap_l3_verb = NULL; > > + const struct rte_flow_action_tunnel_encap_l3 *encap_l3_data; > > + > > + encap_l3_data = > > + (const struct rte_flow_action_tunnel_encap_l3 *)action- > >conf; > > + encap_l3_verb = 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 and > store > > * them in the @p flow if everything is correct. > > * > > @@ -337,6 +509,26 @@ > > action_flags |= MLX5_ACTION_COUNT; > > ++actions_n; > > break; > > + case RTE_FLOW_ACTION_TYPE_TUNNEL_ENCAP: > > + ret = mlx5_flow_validate_action_tunnel_encap > > + (action_flags, > > + actions, > > + attr, > > + error); > > Can be one line. Done. > > > + if (ret < 0) > > + return ret; > > + action_flags |= MLX5_ACTION_TUNNEL_ENCAP; > > + break; > > + case RTE_FLOW_ACTION_TYPE_TUNNEL_ENCAP_L3: > > + ret = mlx5_flow_validate_action_tunnel_encap_l3 > > + (action_flags, > > + actions, > > + attr, > > + error); > > + if (ret < 0) > > + return ret; > > + action_flags |= 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 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. > > > + 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 |= MLX5_ACTION_RSS; > > break; > > + case RTE_FLOW_ACTION_TYPE_TUNNEL_ENCAP: > > + dev_flow->dv.actions[actions_n].type = > > + MLX5DV_FLOW_ACTION_IBV_FLOW_ACTION; > > + dev_flow->dv.actions[actions_n].action = > > + mlx5_flow_dv_create_encap(action, ctx, error); > > + if (!(dev_flow->dv.actions[actions_n].action)) > > + return -rte_errno; > > + dev_flow->dv.encap_verb = > > + dev_flow->dv.actions[actions_n].action; > > + flow->actions |= MLX5_ACTION_TUNNEL_ENCAP; > > + actions_n++; > > + break; > > + case RTE_FLOW_ACTION_TYPE_TUNNEL_ENCAP_L3: > > + dev_flow->dv.actions[actions_n].type = > > + MLX5DV_FLOW_ACTION_IBV_FLOW_ACTION; > > + dev_flow->dv.actions[actions_n].action = > > + 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 = > > + dev_flow->dv.actions[actions_n].action; > > + flow->actions |= MLX5_ACTION_TUNNEL_ENCAP_L3; > > + actions_n++; > > + break; > > default: > > break; > > } > > dev_flow->dv.actions_n = actions_n; > > + return 0; > > } > > > > static uint32_t matcher_zero[MLX5_ST_SZ_DW(fte_match_param)] = { 0 }; > > @@ -1247,6 +1473,7 @@ > > }; > > void *match_value = dev_flow->dv.value.buf; > > uint8_t inner = 0; > > + int retval = 0; > > > > if (priority == MLX5_FLOW_PRIO_RSVD) > > priority = priv->config.flow_prio - 1; @@ -1262,9 +1489,14 > @@ > > matcher.egress = attr->egress; > > if (flow_dv_matcher_register(dev, &matcher, dev_flow, error)) > > return -rte_errno; > > - for (; actions->type != RTE_FLOW_ACTION_TYPE_END; actions++) > > - flow_dv_create_action(actions, dev_flow); > > - return 0; > > + for (; actions->type != RTE_FLOW_ACTION_TYPE_END; actions++) { > > + retval = > > + flow_dv_create_action(actions, dev_flow, priv->ctx, error); > > + if (retval) > > + break; > > The following looks better matched? > > if (flow_dv_create_action(dev, actions, dev_flow, error)) > return -rte_errno; > > And leave 'return 0;' as is below. OK. > > 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 that up? > > > + } > > + > > No blank line is allowed. Please fix other occurrences too. Done. > > > + 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 = NULL; > > + } > > + > > rte_free(dev_flow); > > } > > } > > -- > > 1.8.3.1 > >
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
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
One more issue, PSB. > -----Original Message----- > From: Dekel Peled > Sent: Wednesday, October 3, 2018 11:35 AM > To: Yongseok Koh <yskoh@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 > > 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 > > > @@ -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 set. > > > + */ > > > +static int > > > +mlx5_flow_validate_action_tunnel_encap(uint64_t action_flags, > > > > A static func in this file should start from flow_dv_. Or, you can > > move these 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. > > > > > > + 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 = > > > +action->conf; > > > + > > > + if (!tunnel_encap || !(tunnel_encap->buf)) > > > + return rte_flow_error_set(error, EINVAL, > > > + RTE_FLOW_ERROR_TYPE_ACTION, > > > + action, > > > > Two lines above can be merged? Please check others and merge lines as > > much as possible. > > Done. > > > > > > + "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 set. > > > + */ > > > +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 = > > > + action->conf; > > > > Indentation. > > > > const struct rte_flow_action_tunnel_encap_l3 *tunnel_encap_l3 = > > action->conf; > > Done. > > > > > > + > > > + 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, > > > > Change to flow_dv_create_encap() > > Done. > > > > > > + 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. > > > > > 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. > > > > > + 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, > > > > Change to flow_dv_create_encap_l3() > > Done. > > > > > > + struct ibv_context *ctx, > > > + struct rte_flow_error *error) { > > > + struct ibv_flow_action *encap_l3_verb = NULL; > > > + const struct rte_flow_action_tunnel_encap_l3 *encap_l3_data; > > > + > > > + encap_l3_data = > > > + (const struct rte_flow_action_tunnel_encap_l3 *)action- > > >conf; > > > + encap_l3_verb = 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 > > > and > > store > > > * them in the @p flow if everything is correct. > > > * > > > @@ -337,6 +509,26 @@ > > > action_flags |= MLX5_ACTION_COUNT; > > > ++actions_n; > > > break; > > > + case RTE_FLOW_ACTION_TYPE_TUNNEL_ENCAP: > > > + ret = mlx5_flow_validate_action_tunnel_encap > > > + (action_flags, > > > + actions, > > > + attr, > > > + error); > > > > Can be one line. > > Done. > > > > > > + if (ret < 0) > > > + return ret; > > > + action_flags |= MLX5_ACTION_TUNNEL_ENCAP; > > > + break; > > > + case RTE_FLOW_ACTION_TYPE_TUNNEL_ENCAP_L3: > > > + ret = mlx5_flow_validate_action_tunnel_encap_l3 > > > + (action_flags, > > > + actions, > > > + attr, > > > + error); > > > + if (ret < 0) > > > + return ret; > > > + action_flags |= 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 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. > > > > > > + 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 |= MLX5_ACTION_RSS; > > > break; > > > + case RTE_FLOW_ACTION_TYPE_TUNNEL_ENCAP: > > > + dev_flow->dv.actions[actions_n].type = > > > + MLX5DV_FLOW_ACTION_IBV_FLOW_ACTION; > > > + dev_flow->dv.actions[actions_n].action = > > > + mlx5_flow_dv_create_encap(action, ctx, error); > > > + if (!(dev_flow->dv.actions[actions_n].action)) > > > + return -rte_errno; > > > + dev_flow->dv.encap_verb = > > > + dev_flow->dv.actions[actions_n].action; > > > + flow->actions |= MLX5_ACTION_TUNNEL_ENCAP; > > > + actions_n++; > > > + break; > > > + case RTE_FLOW_ACTION_TYPE_TUNNEL_ENCAP_L3: > > > + dev_flow->dv.actions[actions_n].type = > > > + MLX5DV_FLOW_ACTION_IBV_FLOW_ACTION; > > > + dev_flow->dv.actions[actions_n].action = > > > + 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 = > > > + dev_flow->dv.actions[actions_n].action; > > > + flow->actions |= MLX5_ACTION_TUNNEL_ENCAP_L3; > > > + actions_n++; > > > + break; > > > default: > > > break; > > > } > > > dev_flow->dv.actions_n = actions_n; > > > + return 0; > > > } > > > > > > static uint32_t matcher_zero[MLX5_ST_SZ_DW(fte_match_param)] = { 0 > > > }; @@ -1247,6 +1473,7 @@ > > > }; > > > void *match_value = dev_flow->dv.value.buf; > > > uint8_t inner = 0; > > > + int retval = 0; > > > > > > if (priority == MLX5_FLOW_PRIO_RSVD) > > > priority = priv->config.flow_prio - 1; @@ -1262,9 +1489,14 > > @@ > > > matcher.egress = attr->egress; > > > if (flow_dv_matcher_register(dev, &matcher, dev_flow, error)) > > > return -rte_errno; > > > - for (; actions->type != RTE_FLOW_ACTION_TYPE_END; actions++) > > > - flow_dv_create_action(actions, dev_flow); > > > - return 0; > > > + for (; actions->type != RTE_FLOW_ACTION_TYPE_END; actions++) { > > > + retval = > > > + flow_dv_create_action(actions, dev_flow, priv->ctx, error); > > > + if (retval) > > > + break; > > > > The following looks better matched? > > > > if (flow_dv_create_action(dev, actions, dev_flow, error)) > > return -rte_errno; > > > > And leave 'return 0;' as is below. > > OK. > > > > > 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 that up? Missed this in the first time. Moved the create_action loop earlier, right after the create_item loop, and before the matcher_register call. > > > > > + } > > > + > > > > No blank line is allowed. Please fix other occurrences too. > > Done. > > > > > > + 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 = NULL; > > > + } > > > + > > > rte_free(dev_flow); > > > } > > > } > > > -- > > > 1.8.3.1 > > >
PSB > -----Original Message----- > From: Dekel Peled > Sent: Wednesday, October 3, 2018 2:47 PM > To: Yongseok Koh <yskoh@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 > > One more issue, PSB. > > > -----Original Message----- > > From: Dekel Peled > > Sent: Wednesday, October 3, 2018 11:35 AM > > To: Yongseok Koh <yskoh@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 > > > > 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 > > > > @@ -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 > set. > > > > + */ > > > > +static int > > > > +mlx5_flow_validate_action_tunnel_encap(uint64_t action_flags, > > > > > > A static func in this file should start from flow_dv_. Or, you can > > > move these 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. > > > > > > > > > + 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 = > > > > +action->conf; > > > > + > > > > + if (!tunnel_encap || !(tunnel_encap->buf)) > > > > + return rte_flow_error_set(error, EINVAL, > > > > + RTE_FLOW_ERROR_TYPE_ACTION, > > > > + action, > > > > > > Two lines above can be merged? Please check others and merge lines > > > as much as possible. > > > > Done. > > > > > > > > > + "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 > set. > > > > + */ > > > > +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 = > > > > + action->conf; > > > > > > Indentation. > > > > > > const struct rte_flow_action_tunnel_encap_l3 *tunnel_encap_l3 = > > > action->conf; > > > > Done. > > > > > > > > > + > > > > + 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, > > > > > > Change to flow_dv_create_encap() > > > > Done. > > > > > > > > > + 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. > > > > > > > > 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. > > > > > > > + 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, > > > > > > Change to flow_dv_create_encap_l3() > > > > Done. > > > > > > > > > + struct ibv_context *ctx, > > > > + struct rte_flow_error *error) { > > > > + struct ibv_flow_action *encap_l3_verb = NULL; > > > > + const struct rte_flow_action_tunnel_encap_l3 *encap_l3_data; > > > > + > > > > + encap_l3_data = > > > > + (const struct rte_flow_action_tunnel_encap_l3 *)action- > > > >conf; > > > > + encap_l3_verb = 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 and > > > store > > > > * them in the @p flow if everything is correct. > > > > * > > > > @@ -337,6 +509,26 @@ > > > > action_flags |= MLX5_ACTION_COUNT; > > > > ++actions_n; > > > > break; > > > > + case RTE_FLOW_ACTION_TYPE_TUNNEL_ENCAP: > > > > + ret = mlx5_flow_validate_action_tunnel_encap > > > > + (action_flags, > > > > + actions, > > > > + attr, > > > > + error); > > > > > > Can be one line. > > > > Done. > > > > > > > > > + if (ret < 0) > > > > + return ret; > > > > + action_flags |= MLX5_ACTION_TUNNEL_ENCAP; > > > > + break; > > > > + case RTE_FLOW_ACTION_TYPE_TUNNEL_ENCAP_L3: > > > > + ret = mlx5_flow_validate_action_tunnel_encap_l3 > > > > + (action_flags, > > > > + actions, > > > > + attr, > > > > + error); > > > > + if (ret < 0) > > > > + return ret; > > > > + action_flags |= 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 > 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. > > > > > > > > > + 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 |= MLX5_ACTION_RSS; > > > > break; > > > > + case RTE_FLOW_ACTION_TYPE_TUNNEL_ENCAP: > > > > + dev_flow->dv.actions[actions_n].type = > > > > + MLX5DV_FLOW_ACTION_IBV_FLOW_ACTION; > > > > + dev_flow->dv.actions[actions_n].action = > > > > + mlx5_flow_dv_create_encap(action, ctx, error); > > > > + if (!(dev_flow->dv.actions[actions_n].action)) > > > > + return -rte_errno; > > > > + dev_flow->dv.encap_verb = > > > > + dev_flow->dv.actions[actions_n].action; > > > > + flow->actions |= MLX5_ACTION_TUNNEL_ENCAP; > > > > + actions_n++; > > > > + break; > > > > + case RTE_FLOW_ACTION_TYPE_TUNNEL_ENCAP_L3: > > > > + dev_flow->dv.actions[actions_n].type = > > > > + MLX5DV_FLOW_ACTION_IBV_FLOW_ACTION; > > > > + dev_flow->dv.actions[actions_n].action = > > > > + 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 = > > > > + dev_flow->dv.actions[actions_n].action; > > > > + flow->actions |= MLX5_ACTION_TUNNEL_ENCAP_L3; > > > > + actions_n++; > > > > + break; > > > > default: > > > > break; > > > > } > > > > dev_flow->dv.actions_n = actions_n; > > > > + return 0; > > > > } > > > > > > > > static uint32_t matcher_zero[MLX5_ST_SZ_DW(fte_match_param)] = { > > > > 0 }; @@ -1247,6 +1473,7 @@ > > > > }; > > > > void *match_value = dev_flow->dv.value.buf; > > > > uint8_t inner = 0; > > > > + int retval = 0; > > > > > > > > if (priority == MLX5_FLOW_PRIO_RSVD) > > > > priority = priv->config.flow_prio - 1; @@ -1262,9 +1489,14 > > > @@ > > > > matcher.egress = attr->egress; > > > > if (flow_dv_matcher_register(dev, &matcher, dev_flow, error)) > > > > return -rte_errno; > > > > - for (; actions->type != RTE_FLOW_ACTION_TYPE_END; actions++) > > > > - flow_dv_create_action(actions, dev_flow); > > > > - return 0; > > > > + for (; actions->type != RTE_FLOW_ACTION_TYPE_END; actions++) { > > > > + retval = > > > > + flow_dv_create_action(actions, dev_flow, priv->ctx, error); > > > > + if (retval) > > > > + break; > > > > > > The following looks better matched? > > > > > > if (flow_dv_create_action(dev, actions, dev_flow, error)) > > > return -rte_errno; > > > > > > And leave 'return 0;' as is below. > > > > OK. > > > > > > > > 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 that up? > > Missed this in the first time. > Moved the create_action loop earlier, right after the create_item loop, and > before the matcher_register call. > Please ignore previous comment. If the create_action fails, the destroy function will clean up the matcher allocations as well. > > > > > > > + } > > > > + > > > > > > No blank line is allowed. Please fix other occurrences too. > > > > Done. > > > > > > > > > + 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 = NULL; > > > > + } > > > > + > > > > rte_free(dev_flow); > > > > } > > > > } > > > > -- > > > > 1.8.3.1 > > > >
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 set. + */ +static int +mlx5_flow_validate_action_tunnel_encap(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 *tunnel_encap = action->conf; + + if (!tunnel_encap || !(tunnel_encap->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; +} + +/** + * 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 set. + */ +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 = + action->conf; + + 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, + 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_TUNNEL, + MLX5DV_FLOW_TABLE_TYPE_NIC_TX); + 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, + struct ibv_context *ctx, + struct rte_flow_error *error) +{ + struct ibv_flow_action *encap_l3_verb = NULL; + const struct rte_flow_action_tunnel_encap_l3 *encap_l3_data; + + encap_l3_data = + (const struct rte_flow_action_tunnel_encap_l3 *)action->conf; + encap_l3_verb = 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_TUNNEL, + 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 and store * them in the @p flow if everything is correct. * @@ -337,6 +509,26 @@ action_flags |= MLX5_ACTION_COUNT; ++actions_n; break; + case RTE_FLOW_ACTION_TYPE_TUNNEL_ENCAP: + ret = mlx5_flow_validate_action_tunnel_encap + (action_flags, + actions, + attr, + error); + if (ret < 0) + return ret; + action_flags |= MLX5_ACTION_TUNNEL_ENCAP; + break; + case RTE_FLOW_ACTION_TYPE_TUNNEL_ENCAP_L3: + ret = mlx5_flow_validate_action_tunnel_encap_l3 + (action_flags, + actions, + attr, + error); + if (ret < 0) + return ret; + action_flags |= 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 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, + 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 |= MLX5_ACTION_RSS; break; + case RTE_FLOW_ACTION_TYPE_TUNNEL_ENCAP: + dev_flow->dv.actions[actions_n].type = + MLX5DV_FLOW_ACTION_IBV_FLOW_ACTION; + dev_flow->dv.actions[actions_n].action = + mlx5_flow_dv_create_encap(action, ctx, error); + if (!(dev_flow->dv.actions[actions_n].action)) + return -rte_errno; + dev_flow->dv.encap_verb = + dev_flow->dv.actions[actions_n].action; + flow->actions |= MLX5_ACTION_TUNNEL_ENCAP; + actions_n++; + break; + case RTE_FLOW_ACTION_TYPE_TUNNEL_ENCAP_L3: + dev_flow->dv.actions[actions_n].type = + MLX5DV_FLOW_ACTION_IBV_FLOW_ACTION; + dev_flow->dv.actions[actions_n].action = + 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 = + dev_flow->dv.actions[actions_n].action; + flow->actions |= MLX5_ACTION_TUNNEL_ENCAP_L3; + actions_n++; + break; default: break; } dev_flow->dv.actions_n = actions_n; + return 0; } static uint32_t matcher_zero[MLX5_ST_SZ_DW(fte_match_param)] = { 0 }; @@ -1247,6 +1473,7 @@ }; void *match_value = dev_flow->dv.value.buf; uint8_t inner = 0; + int retval = 0; if (priority == MLX5_FLOW_PRIO_RSVD) priority = priv->config.flow_prio - 1; @@ -1262,9 +1489,14 @@ matcher.egress = attr->egress; if (flow_dv_matcher_register(dev, &matcher, dev_flow, error)) return -rte_errno; - for (; actions->type != RTE_FLOW_ACTION_TYPE_END; actions++) - flow_dv_create_action(actions, dev_flow); - return 0; + for (; actions->type != RTE_FLOW_ACTION_TYPE_END; actions++) { + retval = + flow_dv_create_action(actions, dev_flow, priv->ctx, error); + if (retval) + break; + } + + 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 = NULL; + } + rte_free(dev_flow); } }
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(-)