[v2,12/20] net/mlx5: add mark/flag flow action
Checks
Commit Message
Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
---
drivers/net/mlx5/mlx5_flow.c | 209 +++++++++++++++++++++++++++++++++++
1 file changed, 209 insertions(+)
Comments
On Wed, Jun 27, 2018 at 05:07:44PM +0200, Nelio Laranjeiro wrote:
> Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@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
>
On Wed, Jul 04, 2018 at 01:34:19AM -0700, Yongseok Koh wrote:
> On Wed, Jun 27, 2018 at 05:07:44PM +0200, Nelio Laranjeiro wrote:
> > Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@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.
API terminology: "Actions that modify matching traffic contents or its
properties. This includes adding/removing encapsulation, encryption,
compression and marks."
> > 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.
I've update the documentation of all patches it would be strange to see
some with correct comments and some without :)
> > + */
> > +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.
You are right, but it still need to be fixed, if for some reason a
bisect is used this may break the bug research.
> > + 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.
It needs an snprintf, rte_flow_error_set() does not accept formatting
strings.
>[...]
> > +/**
> > + * 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.
Same answer as above, even if fixed after, it still need a fix here.
> > +}
> > +
> > /**
> > * 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.
Addressing both question here, for the flow_stop() and flow_destroy()
the process is different, for the stop, the flow remains with the mark
bit set but all queues must me cleared, there is no comparison to make.
As you can see, it don't even get a flow, it directly unset the mask bit
in the Rx queues.
For the destroy the issue is different, several flows may be using the
same Rx queues, if one of them will remains and has a mark, then the
associated queues must keep their mark bit set.
As the process is different, it would end in two distinct functions and
each one used by a single function.
For the mlx5_flow_rxq_mark(), the situation is different, the same
process is make when a flow is created and the flow are started.
> > }
> >
> > /**
> > @@ -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
Thanks,
On Thu, Jul 05, 2018 at 10:47:35AM +0200, Nélio Laranjeiro wrote:
> On Wed, Jul 04, 2018 at 01:34:19AM -0700, Yongseok Koh wrote:
> > On Wed, Jun 27, 2018 at 05:07:44PM +0200, Nelio Laranjeiro wrote:
> > > Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@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.
>
> API terminology: "Actions that modify matching traffic contents or its
> properties. This includes adding/removing encapsulation, encryption,
> compression and marks."
>
> > > 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.
>
> I've update the documentation of all patches it would be strange to see
> some with correct comments and some without :)
>
> > > + */
> > > +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.
>
> You are right, but it still need to be fixed, if for some reason a
> bisect is used this may break the bug research.
>
> > > + 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.
>
> It needs an snprintf, rte_flow_error_set() does not accept formatting
> strings.
I think the following would work but never mind. I'm okay with leaving it as is.
No need to make a change here.
#define STRINGIFY(x) #x
#define TOSTRING(x) STRINGIFY(x)
"mark must be between 0 and "
TOSTRING(MLX5_FLOW_MARK_MAX - 1));
> >[...]
> > > +/**
> > > + * 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.
>
> Same answer as above, even if fixed after, it still need a fix here.
>
> > > +}
> > > +
> > > /**
> > > * 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.
>
> Addressing both question here, for the flow_stop() and flow_destroy()
> the process is different, for the stop, the flow remains with the mark
> bit set but all queues must me cleared, there is no comparison to make.
> As you can see, it don't even get a flow, it directly unset the mask bit
> in the Rx queues.
> For the destroy the issue is different, several flows may be using the
> same Rx queues, if one of them will remains and has a mark, then the
> associated queues must keep their mark bit set.
> As the process is different, it would end in two distinct functions and
> each one used by a single function.
>
> For the mlx5_flow_rxq_mark(), the situation is different, the same
> process is make when a flow is created and the flow are started.
I knew the differences but I just wanted to ask if having a separate function
can be a viable option, e.g.,
mlx5_flow_rxq_mark_set()
mlx5_flow_rxq_mark_clear()
mlx5_flow_rxq_mark_trim()
Thanks,
Yongseok
On Thu, Jul 05, 2018 at 12:56:09PM -0700, Yongseok Koh wrote:
>[...]
> > > > + 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.
> >
> > It needs an snprintf, rte_flow_error_set() does not accept formatting
> > strings.
>
> I think the following would work but never mind. I'm okay with leaving it as is.
> No need to make a change here.
>
> #define STRINGIFY(x) #x
> #define TOSTRING(x) STRINGIFY(x)
> "mark must be between 0 and "
> TOSTRING(MLX5_FLOW_MARK_MAX - 1));
>
It was to avoid adding a macro, but indeed there is the same kind in
mlx4, I'll port them for mlx5.
> > >[...]
> > Addressing both question here, for the flow_stop() and flow_destroy()
> > the process is different, for the stop, the flow remains with the mark
> > bit set but all queues must me cleared, there is no comparison to make.
> > As you can see, it don't even get a flow, it directly unset the mask bit
> > in the Rx queues.
> > For the destroy the issue is different, several flows may be using the
> > same Rx queues, if one of them will remains and has a mark, then the
> > associated queues must keep their mark bit set.
> > As the process is different, it would end in two distinct functions and
> > each one used by a single function.
> >
> > For the mlx5_flow_rxq_mark(), the situation is different, the same
> > process is make when a flow is created and the flow are started.
>
> I knew the differences but I just wanted to ask if having a separate function
> can be a viable option, e.g.,
>
> mlx5_flow_rxq_mark_set()
> mlx5_flow_rxq_mark_clear()
> mlx5_flow_rxq_mark_trim()
Certainly, the point is those functions have a short life as few patches
letter they will be removed.
I suppose you prefer to have them and I don't think it will take too
much time to add such function, it will make part of the next revision
;).
Thanks,
@@ -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_*. */
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.
+ */
+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) {
+ 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");
+ 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);
+}
+
/**
* 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.
+ */
+ 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;
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;
+ }
}
/**
@@ -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: