On Wed, Jun 27, 2018 at 05:07:44PM +0200, Nelio Laranjeiro wrote:
> Signed-off-by: Nelio Laranjeiro <nelio.laranje...@6wind.com>
> ---
> drivers/net/mlx5/mlx5_flow.c | 209 +++++++++++++++++++++++++++++++++++
> 1 file changed, 209 insertions(+)
>
> diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c
> index 57f072c03..a39157533 100644
> --- a/drivers/net/mlx5/mlx5_flow.c
> +++ b/drivers/net/mlx5/mlx5_flow.c
> @@ -52,6 +52,10 @@ extern const struct eth_dev_ops mlx5_dev_ops_isolate;
> #define MLX5_FLOW_FATE_DROP (1u << 0)
> #define MLX5_FLOW_FATE_QUEUE (1u << 1)
>
> +/* Modify a packet. */
> +#define MLX5_FLOW_MOD_FLAG (1u << 0)
> +#define MLX5_FLOW_MOD_MARK (1u << 1)
> +
> /** Handles information leading to a drop fate. */
> struct mlx5_flow_verbs {
> unsigned int size; /**< Size of the attribute. */
> @@ -70,6 +74,8 @@ struct rte_flow {
> struct rte_flow_attr attributes; /**< User flow attribute. */
> uint32_t layers;
> /**< Bit-fields of present layers see MLX5_FLOW_ITEMS_*. */
> + uint32_t modifier;
> + /**< Bit-fields of present modifier see MLX5_FLOW_MOD_*. */
Why do you think flag and mark modify a packet? I don't think modifier is an
appropriate name.
> uint32_t fate;
> /**< Bit-fields of present fate see MLX5_FLOW_FATE_*. */
> struct mlx5_flow_verbs verbs; /* Verbs flow. */
> @@ -954,6 +960,12 @@ mlx5_flow_action_drop(const struct rte_flow_action
> *actions,
> actions,
> "multiple fate actions are not"
> " supported");
> + if (flow->modifier & (MLX5_FLOW_MOD_FLAG | MLX5_FLOW_MOD_MARK))
> + return rte_flow_error_set(error, ENOTSUP,
> + RTE_FLOW_ERROR_TYPE_ACTION,
> + actions,
> + "drop is not compatible with"
> + " flag/mark action");
> if (size < flow_size)
> mlx5_flow_spec_verbs_add(flow, &drop, size);
> flow->fate |= MLX5_FLOW_FATE_DROP;
> @@ -1007,6 +1019,144 @@ mlx5_flow_action_queue(struct rte_eth_dev *dev,
> return 0;
> }
>
> +/**
> + * Validate action flag provided by the user.
> + *
> + * @param actions
> + * Pointer to flow actions array.
> + * @param flow
> + * Pointer to the rte_flow structure.
> + * @param flow_size
> + * Size in bytes of the available space for to store the flow information.
> + * @param error
> + * Pointer to error structure.
> + *
> + * @return
> + * size in bytes necessary for the conversion, a negative errno value
> + * otherwise and rte_errno is set.
Like I asked for the previous patches, please be more verbose for function
description and explanation of args and return value.
> + */
> +static int
> +mlx5_flow_action_flag(const struct rte_flow_action *actions,
> + struct rte_flow *flow, const size_t flow_size,
> + struct rte_flow_error *error)
> +{
> + unsigned int size = sizeof(struct ibv_flow_spec_action_tag);
> + struct ibv_flow_spec_action_tag tag = {
> + .type = IBV_FLOW_SPEC_ACTION_TAG,
> + .size = size,
> + .tag_id = mlx5_flow_mark_set(MLX5_FLOW_MARK_DEFAULT),
> + };
> +
> + if (flow->modifier & MLX5_FLOW_MOD_FLAG)
> + return rte_flow_error_set(error, ENOTSUP,
> + RTE_FLOW_ERROR_TYPE_ACTION,
> + actions,
> + "flag action already present");
> + if (flow->fate & MLX5_FLOW_FATE_DROP)
> + return rte_flow_error_set(error, ENOTSUP,
> + RTE_FLOW_ERROR_TYPE_ACTION,
> + actions,
> + "flag is not compatible with drop"
> + " action");
> + if (flow->modifier & MLX5_FLOW_MOD_MARK)
> + return 0;
> + flow->modifier |= MLX5_FLOW_MOD_FLAG;
> + if (size <= flow_size)
> + mlx5_flow_spec_verbs_add(flow, &tag, size);
> + return size;
> +}
> +
> +/**
> + * Update verbs specification to modify the flag to mark.
> + *
> + * @param flow
> + * Pointer to the rte_flow structure.
> + * @param mark_id
> + * Mark identifier to replace the flag.
> + */
> +static void
> +mlx5_flow_verbs_mark_update(struct rte_flow *flow, uint32_t mark_id)
> +{
> + struct ibv_spec_header *hdr;
> + int i;
> +
> + /* Update Verbs specification. */
> + hdr = (struct ibv_spec_header *)flow->verbs.specs;
> + for (i = 0; i != flow->verbs.attr->num_of_specs; ++i) {
flow->verbs.attr/specs can be null in case of validation call. But you don't
need to fix it because it is anyway changed and fixed when you add RSS action.
> + if (hdr->type == IBV_FLOW_SPEC_ACTION_TAG) {
> + struct ibv_flow_spec_action_tag *t =
> + (struct ibv_flow_spec_action_tag *)hdr;
> +
> + t->tag_id = mlx5_flow_mark_set(mark_id);
> + }
> + hdr = (struct ibv_spec_header *)((uintptr_t)hdr + hdr->size);
> + }
> +}
> +
> +/**
> + * Validate action mark provided by the user.
> + *
> + * @param actions
> + * Pointer to flow actions array.
> + * @param flow
> + * Pointer to the rte_flow structure.
> + * @param flow_size[in]
> + * Size in bytes of the available space for to store the flow information.
> + * @param error
> + * Pointer to error structure.
> + *
> + * @return
> + * size in bytes necessary for the conversion, a negative errno value
> + * otherwise and rte_errno is set.
> + */
> +static int
> +mlx5_flow_action_mark(const struct rte_flow_action *actions,
> + struct rte_flow *flow, const size_t flow_size,
> + struct rte_flow_error *error)
> +{
> + const struct rte_flow_action_mark *mark = actions->conf;
> + unsigned int size = sizeof(struct ibv_flow_spec_action_tag);
> + struct ibv_flow_spec_action_tag tag = {
> + .type = IBV_FLOW_SPEC_ACTION_TAG,
> + .size = size,
> + };
> +
> + if (!mark)
> + return rte_flow_error_set(error, EINVAL,
> + RTE_FLOW_ERROR_TYPE_ACTION,
> + actions,
> + "configuration cannot be null");
> + if (mark->id >= MLX5_FLOW_MARK_MAX)
> + return rte_flow_error_set(error, EINVAL,
> + RTE_FLOW_ERROR_TYPE_ACTION_CONF,
> + &mark->id,
> + "mark must be between 0 and"
> + " 16777199");
Use %d and (MLX5_FLOW_MARK_MAX - 1), instead of fixed string.
> + if (flow->modifier & MLX5_FLOW_MOD_MARK)
> + return rte_flow_error_set(error, ENOTSUP,
> + RTE_FLOW_ERROR_TYPE_ACTION,
> + actions,
> + "mark action already present");
> + if (flow->fate & MLX5_FLOW_FATE_DROP)
> + return rte_flow_error_set(error, ENOTSUP,
> + RTE_FLOW_ERROR_TYPE_ACTION,
> + actions,
> + "mark is not compatible with drop"
> + " action");
> + if (flow->modifier & MLX5_FLOW_MOD_FLAG) {
> + mlx5_flow_verbs_mark_update(flow, mark->id);
> + size = 0; /**< Only an update is done in the specification. */
> + } else {
> + tag.tag_id = mlx5_flow_mark_set(mark->id);
> + if (size <= flow_size) {
> + tag.tag_id = mlx5_flow_mark_set(mark->id);
> + mlx5_flow_spec_verbs_add(flow, &tag, size);
> + }
> + }
> + flow->modifier |= MLX5_FLOW_MOD_MARK;
> + return size;
> +}
> +
> /**
> * Validate actions provided by the user.
> *
> @@ -1039,6 +1189,14 @@ mlx5_flow_actions(struct rte_eth_dev *dev,
> switch (actions->type) {
> case RTE_FLOW_ACTION_TYPE_VOID:
> break;
> + case RTE_FLOW_ACTION_TYPE_FLAG:
> + ret = mlx5_flow_action_flag(actions, flow, remain,
> + error);
> + break;
> + case RTE_FLOW_ACTION_TYPE_MARK:
> + ret = mlx5_flow_action_mark(actions, flow, remain,
> + error);
> + break;
> case RTE_FLOW_ACTION_TYPE_DROP:
> ret = mlx5_flow_action_drop(actions, flow, remain,
> error);
> @@ -1122,6 +1280,23 @@ mlx5_flow_merge(struct rte_eth_dev *dev, struct
> rte_flow *flow,
> return size;
> }
>
> +/**
> + * Mark the Rx queues mark flag if the flow has a mark or flag modifier.
> + *
> + * @param dev
> + * Pointer to Ethernet device.
> + * @param flow
> + * Pointer to flow structure.
> + */
> +static void
> +mlx5_flow_rxq_mark(struct rte_eth_dev *dev, struct rte_flow *flow)
> +{
> + struct priv *priv = dev->data->dev_private;
> +
> + (*priv->rxqs)[flow->queue]->mark |=
> + flow->modifier & (MLX5_FLOW_MOD_FLAG | MLX5_FLOW_MOD_MARK);
This has to be !!(...) as rxq->mark has only 1 bit. But, it is also fixed by
coming RSS patches. Not sure what's benefit of splitting patches in this way.
> +}
> +
> /**
> * Validate a flow supported by the NIC.
> *
> @@ -1281,6 +1456,7 @@ mlx5_flow_list_create(struct rte_eth_dev *dev,
> if (ret < 0)
> goto error;
> }
> + mlx5_flow_rxq_mark(dev, flow);
> TAILQ_INSERT_TAIL(list, flow, next);
> return flow;
> error:
> @@ -1323,8 +1499,31 @@ static void
> mlx5_flow_list_destroy(struct rte_eth_dev *dev, struct mlx5_flows *list,
> struct rte_flow *flow)
> {
> + struct priv *priv = dev->data->dev_private;
> + struct rte_flow *rflow;
> + const uint32_t mask = MLX5_FLOW_MOD_FLAG & MLX5_FLOW_MOD_MARK;
> + int mark = 0;
> +
> mlx5_flow_fate_remove(dev, flow);
> TAILQ_REMOVE(list, flow, next);
> + if (!(flow->modifier & mask)) {
> + rte_free(flow);
> + return;
> + }
> + /*
> + * When a flow is removed and this flow has a flag/mark modifier, all
> + * flows needs to be parse to verify if the Rx queue use by the flow
> + * still need to track the flag/mark request.
> + */
When a flow is created, mlx5_flow_rxq_mark() is called. Is there a specific
reason for not writing a separate function in order to drop rxq->mark bit?
> + TAILQ_FOREACH(rflow, &priv->flows, next) {
> + if (!(rflow->modifier & mask))
> + continue;
> + if (flow->queue == rflow->queue) {
> + mark = 1;
> + break;
> + }
> + }
> + (*priv->rxqs)[flow->queue]->mark = !!mark;
mark can be either 0 or 1, then !!mark == mark anyway.
> rte_free(flow);
> }
>
> @@ -1358,10 +1557,19 @@ mlx5_flow_list_flush(struct rte_eth_dev *dev, struct
> mlx5_flows *list)
> void
> mlx5_flow_stop(struct rte_eth_dev *dev, struct mlx5_flows *list)
> {
> + struct priv *priv = dev->data->dev_private;
> struct rte_flow *flow;
> + unsigned int i;
> + unsigned int idx;
>
> TAILQ_FOREACH_REVERSE(flow, list, mlx5_flows, next)
> mlx5_flow_fate_remove(dev, flow);
> + for (idx = 0, i = 0; idx != priv->rxqs_n; ++i) {
> + if (!(*priv->rxqs)[idx])
> + continue;
> + (*priv->rxqs)[idx]->mark = 0;
> + ++idx;
> + }
Same question here but looks like this part is being moved to
mlx5_flow_rxqs_clear() in the future.
> }
>
> /**
> @@ -1386,6 +1594,7 @@ mlx5_flow_start(struct rte_eth_dev *dev, struct
> mlx5_flows *list)
> ret = mlx5_flow_fate_apply(dev, flow, &error);
> if (ret < 0)
> goto error;
> + mlx5_flow_rxq_mark(dev, flow);
> }
> return 0;
> error:
> --
> 2.18.0
>
