From mboxrd@z Thu Jan 1 00:00:00 1970 From: Yongseok Koh Subject: Re: [PATCH v2 12/20] net/mlx5: add mark/flag flow action Date: Wed, 4 Jul 2018 01:34:19 -0700 Message-ID: <20180704083418.GA45405@minint-98vp2qg> References: <90a73b5f33e147ffa3a668f5d19410de17f96045.1530111623.git.nelio.laranjeiro@6wind.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: dev@dpdk.org, Adrien Mazarguil To: Nelio Laranjeiro Return-path: Received: from EUR03-AM5-obe.outbound.protection.outlook.com (mail-eopbgr30045.outbound.protection.outlook.com [40.107.3.45]) by dpdk.org (Postfix) with ESMTP id 0C61D1BEE5 for ; Wed, 4 Jul 2018 10:34:40 +0200 (CEST) Content-Disposition: inline In-Reply-To: <90a73b5f33e147ffa3a668f5d19410de17f96045.1530111623.git.nelio.laranjeiro@6wind.com> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Wed, Jun 27, 2018 at 05:07:44PM +0200, Nelio Laranjeiro wrote: > Signed-off-by: Nelio Laranjeiro > --- > 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 >