[dpdk-dev,v3,2/4] net/mlx5: add software support for rte_flow

Message ID e2b642365ec6d71f6616e7fa4758e84a673d0e92.1482331954.git.nelio.laranjeiro@6wind.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/Intel compilation fail apply patch file failure

Commit Message

Nélio Laranjeiro Dec. 21, 2016, 3:19 p.m. UTC
  Introduce initial software validation for rte_flow rules.

Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
---
 drivers/net/mlx5/mlx5.h         |   2 +
 drivers/net/mlx5/mlx5_flow.c    | 202 ++++++++++++++++++++++++++++++++++------
 drivers/net/mlx5/mlx5_trigger.c |   2 +
 3 files changed, 177 insertions(+), 29 deletions(-)
  

Comments

Ferruh Yigit Dec. 23, 2016, 12:19 p.m. UTC | #1
On 12/21/2016 3:19 PM, Nelio Laranjeiro wrote:
> Introduce initial software validation for rte_flow rules.
> 
> Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
> ---
>  drivers/net/mlx5/mlx5.h         |   2 +
>  drivers/net/mlx5/mlx5_flow.c    | 202 ++++++++++++++++++++++++++++++++++------
>  drivers/net/mlx5/mlx5_trigger.c |   2 +
>  3 files changed, 177 insertions(+), 29 deletions(-)

<...>

> +	for (; items->type != RTE_FLOW_ITEM_TYPE_END; ++items) {
> +		if (items->type == RTE_FLOW_ITEM_TYPE_VOID) {
> +			continue;
> +		} else if (items->type == RTE_FLOW_ITEM_TYPE_ETH) {
> +			if (ilast)
> +				goto exit_item_not_supported;
> +			ilast = items;
> +		} else if ((items->type == RTE_FLOW_ITEM_TYPE_IPV4) ||
> +			   (items->type == RTE_FLOW_ITEM_TYPE_IPV6)) {
> +			if (!ilast)
> +				goto exit_item_not_supported;
> +			else if (ilast->type != RTE_FLOW_ITEM_TYPE_ETH)
> +				goto exit_item_not_supported;
> +			ilast = items;
> +		} else if ((items->type == RTE_FLOW_ITEM_TYPE_UDP) ||
> +			   (items->type == RTE_FLOW_ITEM_TYPE_TCP)) {
> +			if (!ilast)
> +				goto exit_item_not_supported;
> +			else if ((ilast->type != RTE_FLOW_ITEM_TYPE_IPV4) &&
> +				 (ilast->type != RTE_FLOW_ITEM_TYPE_IPV6))
> +				goto exit_item_not_supported;
> +			ilast = items;
> +		} else {
> +			goto exit_item_not_supported;
> +		}
> +	}

I was thinking rte_flow_validate() is validating rule against hardware /
PMD, but here the API input validation is also done.
In patch 3/4 API input validation continues with validating each item
one by one.

Shouldn't each PMD needs to do this kind of input validation?
Why not move generic input validation to rte_flow API?
And if it is valid, call PMD specific one.
  
Adrien Mazarguil Dec. 23, 2016, 1:24 p.m. UTC | #2
On Fri, Dec 23, 2016 at 12:19:30PM +0000, Ferruh Yigit wrote:
> On 12/21/2016 3:19 PM, Nelio Laranjeiro wrote:
> > Introduce initial software validation for rte_flow rules.
> > 
> > Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
> > ---
> >  drivers/net/mlx5/mlx5.h         |   2 +
> >  drivers/net/mlx5/mlx5_flow.c    | 202 ++++++++++++++++++++++++++++++++++------
> >  drivers/net/mlx5/mlx5_trigger.c |   2 +
> >  3 files changed, 177 insertions(+), 29 deletions(-)
> 
> <...>
> 
> > +	for (; items->type != RTE_FLOW_ITEM_TYPE_END; ++items) {
> > +		if (items->type == RTE_FLOW_ITEM_TYPE_VOID) {
> > +			continue;
> > +		} else if (items->type == RTE_FLOW_ITEM_TYPE_ETH) {
> > +			if (ilast)
> > +				goto exit_item_not_supported;
> > +			ilast = items;
> > +		} else if ((items->type == RTE_FLOW_ITEM_TYPE_IPV4) ||
> > +			   (items->type == RTE_FLOW_ITEM_TYPE_IPV6)) {
> > +			if (!ilast)
> > +				goto exit_item_not_supported;
> > +			else if (ilast->type != RTE_FLOW_ITEM_TYPE_ETH)
> > +				goto exit_item_not_supported;
> > +			ilast = items;
> > +		} else if ((items->type == RTE_FLOW_ITEM_TYPE_UDP) ||
> > +			   (items->type == RTE_FLOW_ITEM_TYPE_TCP)) {
> > +			if (!ilast)
> > +				goto exit_item_not_supported;
> > +			else if ((ilast->type != RTE_FLOW_ITEM_TYPE_IPV4) &&
> > +				 (ilast->type != RTE_FLOW_ITEM_TYPE_IPV6))
> > +				goto exit_item_not_supported;
> > +			ilast = items;
> > +		} else {
> > +			goto exit_item_not_supported;
> > +		}
> > +	}
> 
> I was thinking rte_flow_validate() is validating rule against hardware /
> PMD, but here the API input validation is also done.
> In patch 3/4 API input validation continues with validating each item
> one by one.
> 
> Shouldn't each PMD needs to do this kind of input validation?
> Why not move generic input validation to rte_flow API?
> And if it is valid, call PMD specific one.

I think we'll add one eventually, but such a generic function would be
called by PMDs not by applications. PMDs must have the ability to optimize
validate() and create() however they want.

In the meantime in my opinion it's better to let PMDs implement their own to
determine what can be shared later without cluttering rte_flow from the
start.
  

Patch

diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h
index 04f4eaa..ac995a0 100644
--- a/drivers/net/mlx5/mlx5.h
+++ b/drivers/net/mlx5/mlx5.h
@@ -136,6 +136,7 @@  struct priv {
 	unsigned int reta_idx_n; /* RETA index size. */
 	struct fdir_filter_list *fdir_filter_list; /* Flow director rules. */
 	struct fdir_queue *fdir_drop_queue; /* Flow director drop queue. */
+	LIST_HEAD(mlx5_flows, rte_flow) flows; /* RTE Flow rules. */
 	uint32_t link_speed_capa; /* Link speed capabilities. */
 	rte_spinlock_t lock; /* Lock for control functions. */
 };
@@ -283,5 +284,6 @@  struct rte_flow *mlx5_flow_create(struct rte_eth_dev *,
 int mlx5_flow_destroy(struct rte_eth_dev *, struct rte_flow *,
 		      struct rte_flow_error *);
 int mlx5_flow_flush(struct rte_eth_dev *, struct rte_flow_error *);
+void priv_flow_flush(struct priv *);
 
 #endif /* RTE_PMD_MLX5_H_ */
diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c
index a514dff..3e5098a 100644
--- a/drivers/net/mlx5/mlx5_flow.c
+++ b/drivers/net/mlx5/mlx5_flow.c
@@ -30,11 +30,119 @@ 
  *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  */
 
+#include <sys/queue.h>
+
 #include <rte_ethdev.h>
 #include <rte_flow.h>
 #include <rte_flow_driver.h>
+#include <rte_malloc.h>
+
 #include "mlx5.h"
 
+struct rte_flow {
+	LIST_ENTRY(rte_flow) next; /* Pointer to the next rte_flow structure. */
+};
+
+/**
+ * Validate a flow supported by the NIC.
+ *
+ * @param priv
+ *   Pointer to private structure.
+ * @param[in] attr
+ *   Flow rule attributes.
+ * @param[in] pattern
+ *   Pattern specification (list terminated by the END pattern item).
+ * @param[in] actions
+ *   Associated actions (list terminated by the END action).
+ * @param[out] error
+ *   Perform verbose error reporting if not NULL.
+ *
+ * @return
+ *   0 on success, a negative errno value otherwise and rte_errno is set.
+ */
+static int
+priv_flow_validate(struct priv *priv,
+		   const struct rte_flow_attr *attr,
+		   const struct rte_flow_item items[],
+		   const struct rte_flow_action actions[],
+		   struct rte_flow_error *error)
+{
+	(void)priv;
+	const struct rte_flow_item *ilast = NULL;
+
+	if (attr->group) {
+		rte_flow_error_set(error, ENOTSUP,
+				   RTE_FLOW_ERROR_TYPE_ATTR_GROUP,
+				   NULL,
+				   "groups are not supported");
+		return -rte_errno;
+	}
+	if (attr->priority) {
+		rte_flow_error_set(error, ENOTSUP,
+				   RTE_FLOW_ERROR_TYPE_ATTR_PRIORITY,
+				   NULL,
+				   "priorities are not supported");
+		return -rte_errno;
+	}
+	if (attr->egress) {
+		rte_flow_error_set(error, ENOTSUP,
+				   RTE_FLOW_ERROR_TYPE_ATTR_EGRESS,
+				   NULL,
+				   "egress is not supported");
+		return -rte_errno;
+	}
+	if (!attr->ingress) {
+		rte_flow_error_set(error, ENOTSUP,
+				   RTE_FLOW_ERROR_TYPE_ATTR_INGRESS,
+				   NULL,
+				   "only ingress is supported");
+		return -rte_errno;
+	}
+	for (; items->type != RTE_FLOW_ITEM_TYPE_END; ++items) {
+		if (items->type == RTE_FLOW_ITEM_TYPE_VOID) {
+			continue;
+		} else if (items->type == RTE_FLOW_ITEM_TYPE_ETH) {
+			if (ilast)
+				goto exit_item_not_supported;
+			ilast = items;
+		} else if ((items->type == RTE_FLOW_ITEM_TYPE_IPV4) ||
+			   (items->type == RTE_FLOW_ITEM_TYPE_IPV6)) {
+			if (!ilast)
+				goto exit_item_not_supported;
+			else if (ilast->type != RTE_FLOW_ITEM_TYPE_ETH)
+				goto exit_item_not_supported;
+			ilast = items;
+		} else if ((items->type == RTE_FLOW_ITEM_TYPE_UDP) ||
+			   (items->type == RTE_FLOW_ITEM_TYPE_TCP)) {
+			if (!ilast)
+				goto exit_item_not_supported;
+			else if ((ilast->type != RTE_FLOW_ITEM_TYPE_IPV4) &&
+				 (ilast->type != RTE_FLOW_ITEM_TYPE_IPV6))
+				goto exit_item_not_supported;
+			ilast = items;
+		} else {
+			goto exit_item_not_supported;
+		}
+	}
+	for (; actions->type != RTE_FLOW_ACTION_TYPE_END; ++actions) {
+		if (actions->type == RTE_FLOW_ACTION_TYPE_VOID ||
+		    actions->type == RTE_FLOW_ACTION_TYPE_QUEUE ||
+		    actions->type == RTE_FLOW_ACTION_TYPE_DROP)
+			continue;
+		else
+			goto exit_action_not_supported;
+	}
+	return 0;
+exit_item_not_supported:
+	rte_flow_error_set(error, ENOTSUP, RTE_FLOW_ERROR_TYPE_ITEM,
+			   items, "item not supported");
+	return -rte_errno;
+exit_action_not_supported:
+	rte_flow_error_set(error, ENOTSUP, RTE_FLOW_ERROR_TYPE_ACTION,
+			   actions, "action not supported");
+	return -rte_errno;
+}
+
 /**
  * Validate a flow supported by the NIC.
  *
@@ -48,15 +156,13 @@  mlx5_flow_validate(struct rte_eth_dev *dev,
 		   const struct rte_flow_action actions[],
 		   struct rte_flow_error *error)
 {
-	(void)dev;
-	(void)attr;
-	(void)items;
-	(void)actions;
-	(void)error;
-	rte_flow_error_set(error, ENOTSUP,
-			   RTE_FLOW_ERROR_TYPE_NONE,
-			   NULL, "not implemented yet");
-	return -rte_errno;
+	struct priv *priv = dev->data->dev_private;
+	int ret;
+
+	priv_lock(priv);
+	ret = priv_flow_validate(priv, attr, items, actions, error);
+	priv_unlock(priv);
+	return ret;
 }
 
 /**
@@ -72,15 +178,35 @@  mlx5_flow_create(struct rte_eth_dev *dev,
 		 const struct rte_flow_action actions[],
 		 struct rte_flow_error *error)
 {
-	(void)dev;
-	(void)attr;
-	(void)items;
-	(void)actions;
-	(void)error;
-	rte_flow_error_set(error, ENOTSUP,
-			   RTE_FLOW_ERROR_TYPE_NONE,
-			   NULL, "not implemented yet");
-	return NULL;
+	struct priv *priv = dev->data->dev_private;
+	struct rte_flow *flow;
+
+	priv_lock(priv);
+	if (priv_flow_validate(priv, attr, items, actions, error)) {
+		priv_unlock(priv);
+		return NULL;
+	}
+	flow = rte_malloc(__func__, sizeof(struct rte_flow), 0);
+	LIST_INSERT_HEAD(&priv->flows, flow, next);
+	priv_unlock(priv);
+	return flow;
+}
+
+/**
+ * Destroy a flow.
+ *
+ * @param  priv
+ *   Pointer to private structure.
+ * @param[in] flow
+ *   Pointer to the flow to destroy.
+ */
+static void
+priv_flow_destroy(struct priv *priv,
+		  struct rte_flow *flow)
+{
+	(void)priv;
+	LIST_REMOVE(flow, next);
+	rte_free(flow);
 }
 
 /**
@@ -94,13 +220,30 @@  mlx5_flow_destroy(struct rte_eth_dev *dev,
 		  struct rte_flow *flow,
 		  struct rte_flow_error *error)
 {
-	(void)dev;
-	(void)flow;
+	struct priv *priv = dev->data->dev_private;
+
 	(void)error;
-	rte_flow_error_set(error, ENOTSUP,
-			   RTE_FLOW_ERROR_TYPE_NONE,
-			   NULL, "not implemented yet");
-	return -rte_errno;
+	priv_lock(priv);
+	priv_flow_destroy(priv, flow);
+	priv_unlock(priv);
+	return 0;
+}
+
+/**
+ * Destroy all flows.
+ *
+ * @param  priv
+ *   Pointer to private structure.
+ */
+void
+priv_flow_flush(struct priv *priv)
+{
+	while (!LIST_EMPTY(&priv->flows)) {
+		struct rte_flow *flow;
+
+		flow = LIST_FIRST(&priv->flows);
+		priv_flow_destroy(priv, flow);
+	}
 }
 
 /**
@@ -113,10 +256,11 @@  int
 mlx5_flow_flush(struct rte_eth_dev *dev,
 		struct rte_flow_error *error)
 {
-	(void)dev;
+	struct priv *priv = dev->data->dev_private;
+
 	(void)error;
-	rte_flow_error_set(error, ENOTSUP,
-			   RTE_FLOW_ERROR_TYPE_NONE,
-			   NULL, "not implemented yet");
-	return -rte_errno;
+	priv_lock(priv);
+	priv_flow_flush(priv);
+	priv_unlock(priv);
+	return 0;
 }
diff --git a/drivers/net/mlx5/mlx5_trigger.c b/drivers/net/mlx5/mlx5_trigger.c
index d4dccd8..4a359d7 100644
--- a/drivers/net/mlx5/mlx5_trigger.c
+++ b/drivers/net/mlx5/mlx5_trigger.c
@@ -90,6 +90,7 @@  mlx5_dev_start(struct rte_eth_dev *dev)
 	if (dev->data->dev_conf.fdir_conf.mode != RTE_FDIR_MODE_NONE)
 		priv_fdir_enable(priv);
 	priv_dev_interrupt_handler_install(priv, dev);
+	LIST_INIT(&priv->flows);
 	priv_unlock(priv);
 	return -err;
 }
@@ -120,6 +121,7 @@  mlx5_dev_stop(struct rte_eth_dev *dev)
 	priv_mac_addrs_disable(priv);
 	priv_destroy_hash_rxqs(priv);
 	priv_fdir_disable(priv);
+	priv_flow_flush(priv);
 	priv_dev_interrupt_handler_uninstall(priv, dev);
 	priv->started = 0;
 	priv_unlock(priv);