On Wed, Jun 28, 2017 at 05:03:56PM +0300, Vasily Philipov wrote:
> The isolated mode should be enabled.
> The number of queues in RSS ring must be power of 2.
> The sharing a queue between several RSS rings is impossible.
>
> Signed-off-by: Vasily Philipov <vasi...@mellanox.com>
Alternative suggestion for commit log:
net/mlx4: support flow API RSS action
This commit adds support for the flow API RSS action with the following
limitations:
- Only supported when isolated mode is enabled.
- The number of queues specified by the action (rte_flow_action_rss.num)
must be a power of two.
- Each queue index can be specified at most once in the configuration
(rte_flow_action_rss.queue[]).
- Because a queue can be associated with a single RSS context, it cannot be
targeted by multiple RSS actions simultaneously.
A few more comments about this patch, see below.
> ---
> drivers/net/mlx4/mlx4.c | 21 +++--
> drivers/net/mlx4/mlx4.h | 5 ++
> drivers/net/mlx4/mlx4_flow.c | 197
> ++++++++++++++++++++++++++++++++++++++++++-
> drivers/net/mlx4/mlx4_flow.h | 3 +-
> 4 files changed, 211 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/net/mlx4/mlx4.c b/drivers/net/mlx4/mlx4.c
> index 22fa7c6..6ab7241 100644
> --- a/drivers/net/mlx4/mlx4.c
> +++ b/drivers/net/mlx4/mlx4.c
> @@ -554,9 +554,9 @@ void priv_unlock(struct priv *priv)
> * The number of entries in queues[].
> *
> * @return
> - * 0 on success, negative errno value on failure.
> + * Pointer to a parent rxq structure, NULL on failure.
> */
> -static int
> +struct rxq *
> priv_create_parent(struct priv *priv,
> uint16_t queues[],
> uint16_t children_n)
> @@ -568,13 +568,15 @@ void priv_unlock(struct priv *priv)
> parent = rte_zmalloc("parent queue",
> sizeof(*parent),
> RTE_CACHE_LINE_SIZE);
> - if (!parent)
> - return -ENOMEM;
> + if (!parent) {
> + ERROR("cannot allocate memory for RSS parent queue");
> + return NULL;
> + }
> ret = rxq_setup(priv->dev, parent, 0, 0, 0,
> NULL, NULL, children_n, NULL);
> if (ret) {
> rte_free(parent);
> - return -ret;
> + return NULL;
> }
> parent->rss.queues_n = children_n;
> if (queues) {
> @@ -587,7 +589,7 @@ void priv_unlock(struct priv *priv)
> parent->rss.queues[i] = i;
> }
> LIST_INSERT_HEAD(&priv->parents, parent, next);
> - return 0;
> + return parent;
> }
>
> /**
> @@ -639,7 +641,6 @@ void priv_unlock(struct priv *priv)
> unsigned int rxqs_n = dev->data->nb_rx_queues;
> unsigned int txqs_n = dev->data->nb_tx_queues;
> unsigned int tmp;
> - int ret;
>
> priv->rxqs = (void *)dev->data->rx_queues;
> priv->txqs = (void *)dev->data->tx_queues;
> @@ -696,14 +697,12 @@ void priv_unlock(struct priv *priv)
> priv->rxqs_n = rxqs_n;
> if (priv->isolated)
> return 0;
> - ret = priv_create_parent(priv, NULL, priv->rxqs_n);
> - if (!ret)
> + if (priv_create_parent(priv, NULL, priv->rxqs_n))
> return 0;
> /* Failure, rollback. */
> priv->rss = 0;
> priv->rxqs_n = tmp;
> - assert(ret > 0);
> - return ret;
> + return ENOMEM;
> }
I think the above changes should be merged in the previous commit
("net/mlx4: RSS parent queues new method maintenance"). priv_create_parent()
can return the rxq pointer (or NULL in case of error) from the start.
> /**
> diff --git a/drivers/net/mlx4/mlx4.h b/drivers/net/mlx4/mlx4.h
> index b5fe1b4..f45e017 100644
> --- a/drivers/net/mlx4/mlx4.h
> +++ b/drivers/net/mlx4/mlx4.h
> @@ -370,4 +370,9 @@ struct priv {
> void
> rxq_parent_cleanup(struct rxq *parent);
>
> +struct rxq *
> +priv_create_parent(struct priv *priv,
> + uint16_t queues[],
> + uint16_t children_n);
> +
> #endif /* RTE_PMD_MLX4_H_ */
> diff --git a/drivers/net/mlx4/mlx4_flow.c b/drivers/net/mlx4/mlx4_flow.c
> index 3fd2716..9c0fba1 100644
> --- a/drivers/net/mlx4/mlx4_flow.c
> +++ b/drivers/net/mlx4/mlx4_flow.c
> @@ -112,6 +112,7 @@ struct rte_flow_drop {
> static const enum rte_flow_action_type valid_actions[] = {
> RTE_FLOW_ACTION_TYPE_DROP,
> RTE_FLOW_ACTION_TYPE_QUEUE,
> + RTE_FLOW_ACTION_TYPE_RSS,
> RTE_FLOW_ACTION_TYPE_END,
> };
>
> @@ -672,6 +673,76 @@ struct rte_flow_drop {
> if (!queue || (queue->index > (priv->rxqs_n - 1)))
> goto exit_action_not_supported;
> action.queue = 1;
> + action.queues_n = 1;
> + action.queues[0] = queue->index;
> + } else if (actions->type == RTE_FLOW_ACTION_TYPE_RSS) {
> + int i;
> + int ierr;
> + const struct rte_flow_action_rss *rss =
> + (const struct rte_flow_action_rss *)
> + actions->conf;
> +
> + if (!priv->hw_rss) {
> + rte_flow_error_set(error, ENOTSUP,
> + RTE_FLOW_ERROR_TYPE_ACTION,
> + actions,
> + "RSS cannot be used with "
> + "the current configuration");
> + return -rte_errno;
> + }
> + if (!priv->isolated) {
> + rte_flow_error_set(error, ENOTSUP,
> + RTE_FLOW_ERROR_TYPE_ACTION,
> + actions,
> + "RSS cannot be used without "
> + "isolated mode");
> + return -rte_errno;
> + }
> + if (!rte_is_power_of_2(rss->num)) {
> + rte_flow_error_set(error, ENOTSUP,
> + RTE_FLOW_ERROR_TYPE_ACTION,
> + actions,
> + "the number of queues "
> + "should be power of two");
> + return -rte_errno;
> + }
> + if (priv->max_rss_tbl_sz < rss->num) {
> + rte_flow_error_set(error, ENOTSUP,
> + RTE_FLOW_ERROR_TYPE_ACTION,
> + actions,
> + "the number of queues "
> + "is too large");
> + return -rte_errno;
> + }
> + /* checking indexes array */
> + ierr = 0;
> + for (i = 0; i < rss->num; ++i) {
> + int j;
> + if (rss->queue[i] >= priv->rxqs_n)
> + ierr = 1;
> + /*
> + * Prevent the user from specifying
> + * the same queue twice in the RSS array.
> + */
> + for (j = i + 1; j < rss->num && !ierr; ++j)
> + if (rss->queue[j] == rss->queue[i])
> + ierr = 1;
> + if (ierr) {
> + rte_flow_error_set(
> + error,
> + ENOTSUP,
> + RTE_FLOW_ERROR_TYPE_HANDLE,
> + NULL,
> + "RSS action only supports "
> + "unique queue indices "
> + "in a list");
> + return -rte_errno;
> + }
> + }
> + action.queue = 1;
> + action.queues_n = rss->num;
> + for (i = 0; i < rss->num; ++i)
> + action.queues[i] = rss->queue[i];
> } else {
> goto exit_action_not_supported;
> }
> @@ -797,6 +868,82 @@ struct rte_flow_drop {
> }
>
> /**
> + * Get RSS parent rxq structure for given queues.
> + *
> + * Creates a new or returns an existed one.
> + *
> + * @param priv
> + * Pointer to private structure.
> + * @param queues
> + * queues indices array, NULL in default RSS case.
> + * @param children_n
> + * the size of queues array.
> + *
> + * @return
> + * Pointer to a parent rxq structure, NULL on failure.
> + */
> +static struct rxq *
> +priv_get_parent(struct priv *priv,
Please use a common prefix for all these functions for consistency, you
should rename it "priv_parent_get()".
> + uint16_t queues[],
> + uint16_t children_n,
> + struct rte_flow_error *error)
> +{
> + unsigned int i;
> + struct rxq *parent;
> +
> + for (parent = LIST_FIRST(&priv->parents);
> + parent;
> + parent = LIST_NEXT(parent, next)) {
> + unsigned int same = 0;
> + unsigned int overlap = 0;
> +
> + /*
> + * Find out whether an appropriate parent queue already exists
> + * and can be reused, otherwise make sure there are no overlaps.
> + */
> + for (i = 0; i < children_n; ++i) {
> + unsigned int j;
> +
> + for (j = 0; j < parent->rss.queues_n; ++j) {
> + if (parent->rss.queues[j] != queues[i])
> + continue;
> + ++overlap;
> + if (i == j)
> + ++same;
> + }
> + }
> + if (same == children_n &&
> + children_n == parent->rss.queues_n)
> + return parent;
> + else if (overlap)
> + goto error;
> + }
> + /* Exclude the cases when some QPs were created without RSS */
> + for (i = 0; i < children_n; ++i) {
> + struct rxq *rxq = (*priv->rxqs)[queues[i]];
> + if (rxq->qp)
> + goto error;
> + }
> + parent = priv_create_parent(priv, queues, children_n);
> + if (!parent) {
> + rte_flow_error_set(error,
> + ENOMEM, RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
> + NULL, "flow rule creation failure");
> + return NULL;
> + }
> + return parent;
> +
> +error:
> + rte_flow_error_set(error,
> + EEXIST,
> + RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
> + NULL,
> + "sharing a queue between several"
> + " RSS groups is not supported");
> + return NULL;
> +}
> +
> +/**
> * Complete flow rule creation.
> *
> * @param priv
> @@ -819,6 +966,7 @@ struct rte_flow_drop {
> {
> struct ibv_qp *qp;
> struct rte_flow *rte_flow;
> + struct rxq *rxq_parent = NULL;
>
> assert(priv->pd);
> assert(priv->ctx);
> @@ -831,9 +979,39 @@ struct rte_flow_drop {
> if (action->drop) {
> qp = priv->flow_drop_queue->qp;
> } else {
> - struct rxq *rxq = (*priv->rxqs)[action->queue_id];
> + int ret;
> + unsigned int i;
> + struct rxq *rxq = NULL;
>
> - qp = rxq->qp;
> + if (action->queues_n > 1) {
> + rxq_parent = priv_get_parent(priv, action->queues,
> + action->queues_n, error);
> + if (!rxq_parent)
> + goto error;
> + }
> + for (i = 0; i < action->queues_n; ++i) {
> + rxq = (*priv->rxqs)[action->queues[i]];
> + /*
> + * In case of isolated mode we postpone
> + * ibv receive queue creation till the first
> + * rte_flow rule will be applied on that queue.
> + */
> + if (!rxq->qp) {
> + assert(priv->isolated);
> + ret = rxq_create_qp(rxq, rxq->elts_n,
> + 0, 0, rxq_parent);
> + if (ret) {
> + rte_flow_error_set(
> + error,
> + ENOMEM,
> + RTE_FLOW_ERROR_TYPE_HANDLE,
> + NULL,
> + "flow rule creation failure");
> + goto error;
> + }
> + }
> + }
> + qp = action->queues_n > 1 ? rxq_parent->qp : rxq->qp;
> rte_flow->qp = qp;
> }
> rte_flow->ibv_attr = ibv_attr;
> @@ -846,6 +1024,8 @@ struct rte_flow_drop {
> return rte_flow;
>
> error:
> + if (rxq_parent)
> + rxq_parent_cleanup(rxq_parent);
> rte_free(rte_flow);
> return NULL;
> }
> @@ -909,11 +1089,22 @@ struct rte_flow_drop {
> continue;
> } else if (actions->type == RTE_FLOW_ACTION_TYPE_QUEUE) {
> action.queue = 1;
> - action.queue_id =
> + action.queues_n = 1;
> + action.queues[0] =
> ((const struct rte_flow_action_queue *)
> actions->conf)->index;
> } else if (actions->type == RTE_FLOW_ACTION_TYPE_DROP) {
> action.drop = 1;
> + } else if (actions->type == RTE_FLOW_ACTION_TYPE_RSS) {
> + unsigned int i;
> + const struct rte_flow_action_rss *rss =
> + (const struct rte_flow_action_rss *)
> + actions->conf;
> +
> + action.queue = 1;
> + action.queues_n = rss->num;
> + for (i = 0; i < rss->num; ++i)
> + action.queues[i] = rss->queue[i];
> } else {
> rte_flow_error_set(error, ENOTSUP,
> RTE_FLOW_ERROR_TYPE_ACTION,
> diff --git a/drivers/net/mlx4/mlx4_flow.h b/drivers/net/mlx4/mlx4_flow.h
> index 4d007da..beabcf2 100644
> --- a/drivers/net/mlx4/mlx4_flow.h
> +++ b/drivers/net/mlx4/mlx4_flow.h
> @@ -98,7 +98,8 @@ struct mlx4_flow {
> struct mlx4_flow_action {
> uint32_t drop:1; /**< Target is a drop queue. */
> uint32_t queue:1; /**< Target is a receive queue. */
> - uint32_t queue_id; /**< Identifier of the queue. */
> + uint16_t queues[RTE_MAX_QUEUES_PER_PORT]; /**< Queue indices to use. */
> + uint16_t queues_n; /**< Number of entries in queue[] */
> };
>
> int mlx4_priv_flow_start(struct priv *priv);
> --
> 1.8.3.1
--
Adrien Mazarguil
6WIND
