[PATCH] lib/librte_ether: new filter APIs definition

[PATCH] lib/librte_ether: new filter APIs definition

[Jingjing Wu]

Define new APIs to support configure multi-kind filters using same APIs,
instead of creating each API set for each kind of filter.
 - rte_eth_dev_filter_supported
 - rte_eth_dev_filter_ctrl

Filter types, operations, and structures are defined specifically
in new header file lib/librte_eth/rte_dev_ctrl.h.

As to the implementation discussion, please refer to
http://dpdk.org/ml/archives/dev/2014-September/005179.html

Signed-off-by: Jingjing Wu <jingjing.wu-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 lib/librte_ether/Makefile       |  1 +
 lib/librte_ether/rte_eth_ctrl.h | 80 +++++++++++++++++++++++++++++++++++++++++
 lib/librte_ether/rte_ethdev.c   | 32 +++++++++++++++++
 lib/librte_ether/rte_ethdev.h   | 44 +++++++++++++++++++++++
 4 files changed, 157 insertions(+)
 create mode 100644 lib/librte_ether/rte_eth_ctrl.h

diff --git a/lib/librte_ether/Makefile b/lib/librte_ether/Makefile
index b310f8b..a461c31 100644
--- a/lib/librte_ether/Makefile
+++ b/lib/librte_ether/Makefile
@@ -46,6 +46,7 @@ SRCS-y += rte_ethdev.c
 #
 SYMLINK-y-include += rte_ether.h
 SYMLINK-y-include += rte_ethdev.h
+SYMLINK-y-include += rte_eth_ctrl.h
 
 # this lib depends upon:
 DEPDIRS-y += lib/librte_eal lib/librte_mempool lib/librte_ring lib/librte_mbuf
diff --git a/lib/librte_ether/rte_eth_ctrl.h b/lib/librte_ether/rte_eth_ctrl.h
new file mode 100644
index 0000000..e4d54c4
--- /dev/null
+++ b/lib/librte_ether/rte_eth_ctrl.h
@@ -0,0 +1,80 @@
+/*-
+ *   BSD LICENSE
+ *
+ *   Copyright(c) 2010-2014 Intel Corporation. All rights reserved.
+ *   All rights reserved.
+ *
+ *   Redistribution and use in source and binary forms, with or without
+ *   modification, are permitted provided that the following conditions
+ *   are met:
+ *
+ *     * Redistributions of source code must retain the above copyright
+ *       notice, this list of conditions and the following disclaimer.
+ *     * Redistributions in binary form must reproduce the above copyright
+ *       notice, this list of conditions and the following disclaimer in
+ *       the documentation and/or other materials provided with the
+ *       distribution.
+ *     * Neither the name of Intel Corporation nor the names of its
+ *       contributors may be used to endorse or promote products derived
+ *       from this software without specific prior written permission.
+ *
+ *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#ifndef _RTE_ETH_CTRL_H_
+#define _RTE_ETH_CTRL_H_
+
+/**
+ * @file
+ *
+ * Ethernet device features and related data structures used
+ * by control APIs should be defined in this file.
+ *
+ */
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+/**
+ * Feature filter types
+ */
+enum rte_filter_type {
+	RTE_ETH_FILTER_NONE = 0,
+	RTE_ETH_FILTER_HASH,
+	RTE_ETH_FILTER_FDIR,
+	RTE_ETH_FILTER_MAX,
+};
+
+/**
+ * All generic operations to filters
+ */
+enum rte_filter_op {
+	RTE_ETH_FILTER_OP_NONE = 0,
+	/**< used to check whether the type filter is supported */
+	RTE_ETH_FILTER_OP_ADD,      /**< add filter entry */
+	RTE_ETH_FILTER_OP_UPDATE,   /**< update filter entry */
+	RTE_ETH_FILTER_OP_DELETE,   /**< delete filter entry */
+	RTE_ETH_FILTER_OP_FLUSH,    /**< flush all entries */
+	RTE_ETH_FILTER_OP_GET,      /**< get filter entry */
+	RTE_ETH_FILTER_OP_SET,      /**< configurations */
+	RTE_ETH_FILTER_OP_GET_INFO,
+	/**< get information of filter, such as status or statistics */
+	RTE_ETH_FILTER_OP_MAX,
+};
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif /* _RTE_ETH_CTRL_H_ */
diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index 1659340..1edc816 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -3148,3 +3148,35 @@ rte_eth_dev_get_flex_filter(uint8_t port_id, uint16_t index,
 	return (*dev->dev_ops->get_flex_filter)(dev, index, filter,
 						rx_queue);
 }
+
+int
+rte_eth_dev_filter_supported(uint8_t port_id, enum rte_filter_type filter_type)
+{
+	struct rte_eth_dev *dev;
+
+	if (port_id >= nb_ports) {
+		PMD_DEBUG_TRACE("Invalid port_id=%d\n", port_id);
+		return -ENODEV;
+	}
+
+	dev = &rte_eth_devices[port_id];
+	FUNC_PTR_OR_ERR_RET(*dev->dev_ops->filter_ctrl, -ENOTSUP);
+	return (*dev->dev_ops->filter_ctrl)(dev, filter_type,
+				RTE_ETH_FILTER_OP_NONE, NULL);
+}
+
+int
+rte_eth_dev_filter_ctrl(uint8_t port_id, enum rte_filter_type filter_type,
+		       enum rte_filter_op filter_op, void *arg)
+{
+	struct rte_eth_dev *dev;
+
+	if (port_id >= nb_ports) {
+		PMD_DEBUG_TRACE("Invalid port_id=%d\n", port_id);
+		return -ENODEV;
+	}
+
+	dev = &rte_eth_devices[port_id];
+	FUNC_PTR_OR_ERR_RET(*dev->dev_ops->filter_ctrl, -ENOTSUP);
+	return (*dev->dev_ops->filter_ctrl)(dev, filter_type, filter_op, arg);
+}
diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index 13be711..85c229d 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -177,6 +177,7 @@ extern "C" {
 #include <rte_pci.h>
 #include <rte_mbuf.h>
 #include "rte_ether.h"
+#include "rte_eth_ctrl.h"
 
 /**
  * A structure used to retrieve statistics for an Ethernet port.
@@ -1385,6 +1386,12 @@ typedef int (*eth_get_flex_filter_t)(struct rte_eth_dev *dev,
 			uint16_t *rx_queue);
 /**< @internal Get a flex filter rule on an Ethernet device */
 
+typedef int (*eth_filter_ctrl_t)(struct rte_eth_dev *dev,
+				 enum rte_filter_type filter_type,
+				 enum rte_filter_op filter_op,
+				 void *arg);
+/**< @internal Take operations to assigned filter type on an Ethernet device */
+
 /**
  * @internal A structure containing the functions exported by an Ethernet driver.
  */
@@ -1493,6 +1500,7 @@ struct eth_dev_ops {
 	eth_add_flex_filter_t          add_flex_filter;      /**< add flex filter. */
 	eth_remove_flex_filter_t       remove_flex_filter;   /**< remove flex filter. */
 	eth_get_flex_filter_t          get_flex_filter;      /**< get flex filter. */
+	eth_filter_ctrl_t              filter_ctrl;          /**< common filter control*/
 };
 
 /**
@@ -3622,6 +3630,42 @@ int rte_eth_dev_remove_flex_filter(uint8_t port_id, uint16_t index);
 int rte_eth_dev_get_flex_filter(uint8_t port_id, uint16_t index,
 			struct rte_flex_filter *filter, uint16_t *rx_queue);
 
+/**
+ * Check whether the filter type is supported on an Ethernet device.
+ * All the supported filter types are defined in 'rte_eth_ctrl.h'.
+ *
+ * @param port_id
+ *   The port identifier of the Ethernet device.
+ * @param filter_type
+ *   filter type.
+ * @return
+ *   - (0) if successful.
+ *   - (-ENOTSUP) if hardware doesn't support this filter type.
+ *   - (-ENODEV) if *port_id* invalid.
+ */
+int rte_eth_dev_filter_supported(uint8_t port_id, enum rte_filter_type filter_type);
+
+/**
+ * Take operations to assigned filter type on an Ethernet device.
+ * All the supported operations and filter types are defined in 'rte_eth_ctrl.h'.
+ *
+ * @param port_id
+ *   The port identifier of the Ethernet device.
+ * @param filter_type
+ *   filter type.
+ * @param filter_op
+ *   The operation taken to assigned filter.
+ * @param arg
+ *   A pointer to arguments defined specifically for the operation.
+ * @return
+ *   - (0) if successful.
+ *   - (-ENOTSUP) if hardware doesn't support.
+ *   - (-ENODEV) if *port_id* invalid.
+ *   - others depends on the specific operations implementation.
+ */
+int rte_eth_dev_filter_ctrl(uint8_t port_id, enum rte_filter_type filter_type,
+			enum rte_filter_op filter_op, void *arg);
+
 #ifdef __cplusplus
 }
 #endif
-- 
1.8.1.4

Re: [PATCH] lib/librte_ether: new filter APIs definition

[Zhang, Helin]

Hi

> Define new APIs to support configure multi-kind filters using same APIs, instead
> of creating each API set for each kind of filter.
>  - rte_eth_dev_filter_supported
>  - rte_eth_dev_filter_ctrl
> 
> Filter types, operations, and structures are defined specifically in new header
> file lib/librte_eth/rte_dev_ctrl.h.
> 
> As to the implementation discussion, please refer to
> http://dpdk.org/ml/archives/dev/2014-September/005179.html
> 
> Signed-off-by: Jingjing Wu <jingjing.wu-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Acked-by: Helin Zhang <helin.zhang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>, if the title can be changed to "ethdev: new filter APIs definition".


Regards,
Helin

Re: [PATCH] lib/librte_ether: new filter APIs definition

[Thomas Monjalon]

2014-10-17 07:29, Jingjing Wu:
> Define new APIs to support configure multi-kind filters using same APIs,
> instead of creating each API set for each kind of filter.
>  - rte_eth_dev_filter_supported
>  - rte_eth_dev_filter_ctrl
> 
> Filter types, operations, and structures are defined specifically
> in new header file lib/librte_eth/rte_dev_ctrl.h.
> 
> As to the implementation discussion, please refer to
> http://dpdk.org/ml/archives/dev/2014-September/005179.html
[...]
> --- /dev/null
> +++ b/lib/librte_ether/rte_eth_ctrl.h

Why this name? I think we can reserve this file for filtering API.
So rte_eth_rx_filter.h would be more appropriate.

> +/**
> + * All generic operations to filters
> + */

rewording: "Generic operations on filters"
Could you elaborate on "generic"? What would mean "specific"?

> +enum rte_filter_op {
> +	RTE_ETH_FILTER_OP_NONE = 0,
> +	/**< used to check whether the type filter is supported */
> +	RTE_ETH_FILTER_OP_ADD,      /**< add filter entry */
> +	RTE_ETH_FILTER_OP_UPDATE,   /**< update filter entry */
> +	RTE_ETH_FILTER_OP_DELETE,   /**< delete filter entry */
> +	RTE_ETH_FILTER_OP_FLUSH,    /**< flush all entries */
> +	RTE_ETH_FILTER_OP_GET,      /**< get filter entry */
> +	RTE_ETH_FILTER_OP_SET,      /**< configurations */
> +	RTE_ETH_FILTER_OP_GET_INFO,

Could we remove "OP", except for OP_NONE and OP_MAX?

> +	/**< get information of filter, such as status or statistics */
> +	RTE_ETH_FILTER_OP_MAX,
> +};

> +int
> +rte_eth_dev_filter_supported(uint8_t port_id, enum rte_filter_type filter_type)

This function is really important for compatibility. Good

> +/**
> + * Take operations to assigned filter type on an Ethernet device.
> + * All the supported operations and filter types are defined in 'rte_eth_ctrl.h'.
> + *
> + * @param port_id
> + *   The port identifier of the Ethernet device.
> + * @param filter_type
> + *   filter type.
> + * @param filter_op
> + *   The operation taken to assigned filter.

Rewording: "Type of operation"

> + * @param arg
> + *   A pointer to arguments defined specifically for the operation.

Actually, arg is specific to the filter type.
Could it be also specific to the operation. Maybe.
I think we will have to explicitly specify which operations can be used with
each structure (in its comments).

> + * @return
> + *   - (0) if successful.
> + *   - (-ENOTSUP) if hardware doesn't support.
> + *   - (-ENODEV) if *port_id* invalid.
> + *   - others depends on the specific operations implementation.
> + */
> +int rte_eth_dev_filter_ctrl(uint8_t port_id, enum rte_filter_type filter_type,
> +			enum rte_filter_op filter_op, void *arg);

Could we add rx in the name? rte_eth_dev_rx_filter_ctrl
If you agree with this naming, it should be added in several other places.

This API is quite simple (which is a good thing).
Let's see how it fits when integrating filtering features.
If something appears to be wrongly designed when integrating a feature
or when implementing it in a driver, feel free to fix the API.

Thanks
-- 
Thomas

Re: [PATCH] lib/librte_ether: new filter APIs definition

[Richardson, Bruce]

> -----Original Message-----
> From: dev [mailto:dev-bounces-VfR2kkLFssw@public.gmane.org] On Behalf Of Thomas Monjalon
> Sent: Friday, October 17, 2014 10:08 AM
> To: Wu, Jingjing
> Cc: dev-VfR2kkLFssw@public.gmane.org
> Subject: Re: [dpdk-dev] [PATCH] lib/librte_ether: new filter APIs definition
> 
> 2014-10-17 07:29, Jingjing Wu:
> > Define new APIs to support configure multi-kind filters using same APIs,
> > instead of creating each API set for each kind of filter.
> >  - rte_eth_dev_filter_supported
> >  - rte_eth_dev_filter_ctrl
> >
> > Filter types, operations, and structures are defined specifically
> > in new header file lib/librte_eth/rte_dev_ctrl.h.
> >
> > As to the implementation discussion, please refer to
> > http://dpdk.org/ml/archives/dev/2014-September/005179.html
> [...]
> > --- /dev/null
> > +++ b/lib/librte_ether/rte_eth_ctrl.h
> 
> Why this name? I think we can reserve this file for filtering API.
> So rte_eth_rx_filter.h would be more appropriate.
> 
> > +/**
> > + * All generic operations to filters
> > + */
> 
> rewording: "Generic operations on filters"
> Could you elaborate on "generic"? What would mean "specific"?
> 
> > +enum rte_filter_op {
> > +	RTE_ETH_FILTER_OP_NONE = 0,
> > +	/**< used to check whether the type filter is supported */
> > +	RTE_ETH_FILTER_OP_ADD,      /**< add filter entry */
> > +	RTE_ETH_FILTER_OP_UPDATE,   /**< update filter entry */
> > +	RTE_ETH_FILTER_OP_DELETE,   /**< delete filter entry */
> > +	RTE_ETH_FILTER_OP_FLUSH,    /**< flush all entries */
> > +	RTE_ETH_FILTER_OP_GET,      /**< get filter entry */
> > +	RTE_ETH_FILTER_OP_SET,      /**< configurations */
> > +	RTE_ETH_FILTER_OP_GET_INFO,
> 
> Could we remove "OP", except for OP_NONE and OP_MAX?

OP_NONE ==> NOP or NOOP perhaps?

/Bruce

Re: [PATCH] lib/librte_ether: new filter APIs definition

[Wu, Jingjing]

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon-pdR9zngts4EAvxtiuMwx3w@public.gmane.org]
> Sent: Friday, October 17, 2014 5:08 PM
> To: Wu, Jingjing
> Cc: dev-VfR2kkLFssw@public.gmane.org
> Subject: Re: [dpdk-dev] [PATCH] lib/librte_ether: new filter APIs definition
> 
> 2014-10-17 07:29, Jingjing Wu:
> > Define new APIs to support configure multi-kind filters using same APIs,
> > instead of creating each API set for each kind of filter.
> >  - rte_eth_dev_filter_supported
> >  - rte_eth_dev_filter_ctrl
> >
> > Filter types, operations, and structures are defined specifically
> > in new header file lib/librte_eth/rte_dev_ctrl.h.
> >
> > As to the implementation discussion, please refer to
> > http://dpdk.org/ml/archives/dev/2014-September/005179.html
> [...]
> > --- /dev/null
> > +++ b/lib/librte_ether/rte_eth_ctrl.h
> 
> Why this name? I think we can reserve this file for filtering API.
> So rte_eth_rx_filter.h would be more appropriate.
> 
Yes, the name rte_eth_ctrl.h can be extensible for XX_ctrl APIs.
While the rte_eth_rx_filter.h only can be used for rx filter features.

> > +/**
> > + * All generic operations to filters
> > + */
> 
> rewording: "Generic operations on filters"
OK.
> Could you elaborate on "generic"? What would mean "specific"?
> 
OK, will add more description.

> > +enum rte_filter_op {
> > +	RTE_ETH_FILTER_OP_NONE = 0,
> > +	/**< used to check whether the type filter is supported */
> > +	RTE_ETH_FILTER_OP_ADD,      /**< add filter entry */
> > +	RTE_ETH_FILTER_OP_UPDATE,   /**< update filter entry */
> > +	RTE_ETH_FILTER_OP_DELETE,   /**< delete filter entry */
> > +	RTE_ETH_FILTER_OP_FLUSH,    /**< flush all entries */
> > +	RTE_ETH_FILTER_OP_GET,      /**< get filter entry */
> > +	RTE_ETH_FILTER_OP_SET,      /**< configurations */
> > +	RTE_ETH_FILTER_OP_GET_INFO,
> 
> Could we remove "OP", except for OP_NONE and OP_MAX?
> 
Fine, ADD/UPDATE/... is operation definitely, OP may be redundant.

> > +	/**< get information of filter, such as status or statistics */
> > +	RTE_ETH_FILTER_OP_MAX,
> > +};
> 
> > +int
> > +rte_eth_dev_filter_supported(uint8_t port_id, enum rte_filter_type filter_type)
> 
> This function is really important for compatibility. Good
> 
> > +/**
> > + * Take operations to assigned filter type on an Ethernet device.
> > + * All the supported operations and filter types are defined in 'rte_eth_ctrl.h'.
> > + *
> > + * @param port_id
> > + *   The port identifier of the Ethernet device.
> > + * @param filter_type
> > + *   filter type.
> > + * @param filter_op
> > + *   The operation taken to assigned filter.
> 
> Rewording: "Type of operation"
> 
OK, will change.

> > + * @param arg
> > + *   A pointer to arguments defined specifically for the operation.
> 
> Actually, arg is specific to the filter type.
> Could it be also specific to the operation. Maybe.
> I think we will have to explicitly specify which operations can be used with
> each structure (in its comments).
> 
Yes, each owner who define structures here need to point out the specific filter type and operation in its comments.

> > + * @return
> > + *   - (0) if successful.
> > + *   - (-ENOTSUP) if hardware doesn't support.
> > + *   - (-ENODEV) if *port_id* invalid.
> > + *   - others depends on the specific operations implementation.
> > + */
> > +int rte_eth_dev_filter_ctrl(uint8_t port_id, enum rte_filter_type filter_type,
> > +			enum rte_filter_op filter_op, void *arg);
> 
> Could we add rx in the name? rte_eth_dev_rx_filter_ctrl
> If you agree with this naming, it should be added in several other places.
> 
We name it as rte_eth_dev_rx_filter_ctrl before. But we have no
Idea whether we have tx_filter, or even have in future, we can share the API. 
So decided to name it as rte_eth_dev_filter_ctrl to make the name brief.

> This API is quite simple (which is a good thing).
> Let's see how it fits when integrating filtering features.
> If something appears to be wrongly designed when integrating a feature
> or when implementing it in a driver, feel free to fix the API.
>
We think so too.

> Thanks
> --
> Thomas
Thanks
Jingjing

Re: [PATCH] lib/librte_ether: new filter APIs definition

[Wu, Jingjing]

> -----Original Message-----
> From: Wu, Jingjing
> Sent: Saturday, October 18, 2014 12:02 AM
> To: Thomas Monjalon
> Cc: dev-VfR2kkLFssw@public.gmane.org
> Subject: RE: [dpdk-dev] [PATCH] lib/librte_ether: new filter APIs definition
> 
> 
> 
> > -----Original Message-----
> > From: Thomas Monjalon [mailto:thomas.monjalon-pdR9zngts4EAvxtiuMwx3w@public.gmane.org]
> > Sent: Friday, October 17, 2014 5:08 PM
> > To: Wu, Jingjing
> > Cc: dev-VfR2kkLFssw@public.gmane.org
> > Subject: Re: [dpdk-dev] [PATCH] lib/librte_ether: new filter APIs
> > definition
> >
> > 2014-10-17 07:29, Jingjing Wu:
> > > Define new APIs to support configure multi-kind filters using same
> > > APIs, instead of creating each API set for each kind of filter.
> > >  - rte_eth_dev_filter_supported
> > >  - rte_eth_dev_filter_ctrl
> > >
> > > Filter types, operations, and structures are defined specifically in
> > > new header file lib/librte_eth/rte_dev_ctrl.h.
> > >
> > > As to the implementation discussion, please refer to
> > > http://dpdk.org/ml/archives/dev/2014-September/005179.html
> > [...]
> > > --- /dev/null
> > > +++ b/lib/librte_ether/rte_eth_ctrl.h
> >
> > Why this name? I think we can reserve this file for filtering API.
> > So rte_eth_rx_filter.h would be more appropriate.
> >
> Yes, the name rte_eth_ctrl.h can be extensible for XX_ctrl APIs.
> While the rte_eth_rx_filter.h only can be used for rx filter features.
> 
> > > +/**
> > > + * All generic operations to filters  */
> >
> > rewording: "Generic operations on filters"
> OK.
> > Could you elaborate on "generic"? What would mean "specific"?
> >
> OK, will add more description.

Generic operations means: the operations filter usually have, like add/delete/set ...
Specific means: different filter type may have different attributes, for example the filter's input set. The Specific stuffs are contained in the structure definition. Structure definition is not included in this patch. It need to be added in rte_eth_ctrl.h header file by owner who contributes to each kind of filter.
> 
> > > +enum rte_filter_op {
> > > +	RTE_ETH_FILTER_OP_NONE = 0,
> > > +	/**< used to check whether the type filter is supported */
> > > +	RTE_ETH_FILTER_OP_ADD,      /**< add filter entry */
> > > +	RTE_ETH_FILTER_OP_UPDATE,   /**< update filter entry */
> > > +	RTE_ETH_FILTER_OP_DELETE,   /**< delete filter entry */
> > > +	RTE_ETH_FILTER_OP_FLUSH,    /**< flush all entries */
> > > +	RTE_ETH_FILTER_OP_GET,      /**< get filter entry */
> > > +	RTE_ETH_FILTER_OP_SET,      /**< configurations */
> > > +	RTE_ETH_FILTER_OP_GET_INFO,
> >
> > Could we remove "OP", except for OP_NONE and OP_MAX?
> >
> Fine, ADD/UPDATE/... is operation definitely, OP may be redundant.
> 
> > > +	/**< get information of filter, such as status or statistics */
> > > +	RTE_ETH_FILTER_OP_MAX,
> > > +};
> >
> > > +int
> > > +rte_eth_dev_filter_supported(uint8_t port_id, enum rte_filter_type
> > > +filter_type)
> >
> > This function is really important for compatibility. Good
> >
> > > +/**
> > > + * Take operations to assigned filter type on an Ethernet device.
> > > + * All the supported operations and filter types are defined in
> 'rte_eth_ctrl.h'.
> > > + *
> > > + * @param port_id
> > > + *   The port identifier of the Ethernet device.
> > > + * @param filter_type
> > > + *   filter type.
> > > + * @param filter_op
> > > + *   The operation taken to assigned filter.
> >
> > Rewording: "Type of operation"
> >
> OK, will change.
> 
> > > + * @param arg
> > > + *   A pointer to arguments defined specifically for the operation.
> >
> > Actually, arg is specific to the filter type.
> > Could it be also specific to the operation. Maybe.
> > I think we will have to explicitly specify which operations can be
> > used with each structure (in its comments).
> >
> Yes, each owner who define structures here need to point out the specific
> filter type and operation in its comments.
> 
> > > + * @return
> > > + *   - (0) if successful.
> > > + *   - (-ENOTSUP) if hardware doesn't support.
> > > + *   - (-ENODEV) if *port_id* invalid.
> > > + *   - others depends on the specific operations implementation.
> > > + */
> > > +int rte_eth_dev_filter_ctrl(uint8_t port_id, enum rte_filter_type
> filter_type,
> > > +			enum rte_filter_op filter_op, void *arg);
> >
> > Could we add rx in the name? rte_eth_dev_rx_filter_ctrl If you agree
> > with this naming, it should be added in several other places.
> >
> We name it as rte_eth_dev_rx_filter_ctrl before. But we have no Idea
> whether we have tx_filter, or even have in future, we can share the API.
> So decided to name it as rte_eth_dev_filter_ctrl to make the name brief.
> 
> > This API is quite simple (which is a good thing).
> > Let's see how it fits when integrating filtering features.
> > If something appears to be wrongly designed when integrating a feature
> > or when implementing it in a driver, feel free to fix the API.
> >
> We think so too.
> 
> > Thanks
> > --
> > Thomas
> Thanks
> Jingjing

Re: [PATCH] lib/librte_ether: new filter APIs definition

[Liu, Jijiang]

As to this API implementation in i40e, there are some common codes should be included in this patch.

> -----Original Message-----
> From: dev [mailto:dev-bounces-VfR2kkLFssw@public.gmane.org] On Behalf Of Thomas Monjalon
> Sent: Friday, October 17, 2014 5:08 PM
> To: Wu, Jingjing
> Cc: dev-VfR2kkLFssw@public.gmane.org
> Subject: Re: [dpdk-dev] [PATCH] lib/librte_ether: new filter APIs definition
> 
> 2014-10-17 07:29, Jingjing Wu:
> > Define new APIs to support configure multi-kind filters using same
> > APIs, instead of creating each API set for each kind of filter.
> >  - rte_eth_dev_filter_supported
> >  - rte_eth_dev_filter_ctrl
> >
> > Filter types, operations, and structures are defined specifically in
> > new header file lib/librte_eth/rte_dev_ctrl.h.
> >
> > As to the implementation discussion, please refer to
> > http://dpdk.org/ml/archives/dev/2014-September/005179.html
> [...]
> > --- /dev/null
> > +++ b/lib/librte_ether/rte_eth_ctrl.h
> 
> Why this name? I think we can reserve this file for filtering API.
> So rte_eth_rx_filter.h would be more appropriate.
> 
> > +/**
> > + * All generic operations to filters
> > + */
> 
> rewording: "Generic operations on filters"
> Could you elaborate on "generic"? What would mean "specific"?
> 
> > +enum rte_filter_op {
> > +	RTE_ETH_FILTER_OP_NONE = 0,
> > +	/**< used to check whether the type filter is supported */
> > +	RTE_ETH_FILTER_OP_ADD,      /**< add filter entry */
> > +	RTE_ETH_FILTER_OP_UPDATE,   /**< update filter entry */
> > +	RTE_ETH_FILTER_OP_DELETE,   /**< delete filter entry */
> > +	RTE_ETH_FILTER_OP_FLUSH,    /**< flush all entries */
> > +	RTE_ETH_FILTER_OP_GET,      /**< get filter entry */
> > +	RTE_ETH_FILTER_OP_SET,      /**< configurations */
> > +	RTE_ETH_FILTER_OP_GET_INFO,
> 
> Could we remove "OP", except for OP_NONE and OP_MAX?
> 
> > +	/**< get information of filter, such as status or statistics */
> > +	RTE_ETH_FILTER_OP_MAX,
> > +};
> 
> > +int
> > +rte_eth_dev_filter_supported(uint8_t port_id, enum rte_filter_type
> > +filter_type)
> 
> This function is really important for compatibility. Good
> 
> > +/**
> > + * Take operations to assigned filter type on an Ethernet device.
> > + * All the supported operations and filter types are defined in
> 'rte_eth_ctrl.h'.
> > + *
> > + * @param port_id
> > + *   The port identifier of the Ethernet device.
> > + * @param filter_type
> > + *   filter type.
> > + * @param filter_op
> > + *   The operation taken to assigned filter.
> 
> Rewording: "Type of operation"
> 
> > + * @param arg
> > + *   A pointer to arguments defined specifically for the operation.
> 
> Actually, arg is specific to the filter type.
> Could it be also specific to the operation. Maybe.
> I think we will have to explicitly specify which operations can be used with
> each structure (in its comments).
> 
> > + * @return
> > + *   - (0) if successful.
> > + *   - (-ENOTSUP) if hardware doesn't support.
> > + *   - (-ENODEV) if *port_id* invalid.
> > + *   - others depends on the specific operations implementation.
> > + */
> > +int rte_eth_dev_filter_ctrl(uint8_t port_id, enum rte_filter_type
> filter_type,
> > +			enum rte_filter_op filter_op, void *arg);
> 
> Could we add rx in the name? rte_eth_dev_rx_filter_ctrl If you agree with
> this naming, it should be added in several other places.
> 
> This API is quite simple (which is a good thing).
> Let's see how it fits when integrating filtering features.
> If something appears to be wrongly designed when integrating a feature or
> when implementing it in a driver, feel free to fix the API.
> 
> Thanks
> --
> Thomas

[PATCH v2 0/2] new filter APIs definition

[Jingjing Wu]

new filter APIs definition in ethdev
define filter_ctrl ops in i40e driver

v2 changes:
  remove OP from the name of filter opeartions
  add API implementation in i40e.
  correct comments

Jingjing Wu (2):
  librte_ether: new filter APIs definition
  i40e: define filter_ctrl ops in i40e driver

 lib/librte_ether/Makefile         |  1 +
 lib/librte_ether/rte_eth_ctrl.h   | 80 +++++++++++++++++++++++++++++++++++++++
 lib/librte_ether/rte_ethdev.c     | 32 ++++++++++++++++
 lib/librte_ether/rte_ethdev.h     | 44 +++++++++++++++++++++
 lib/librte_pmd_i40e/i40e_ethdev.c | 63 ++++++++++++++++++++++++++++++
 5 files changed, 220 insertions(+)
 create mode 100644 lib/librte_ether/rte_eth_ctrl.h

-- 
1.8.1.4

[PATCH v2 1/2] librte_ether: new filter APIs definition

[Jingjing Wu]

Define new APIs to support configure multi-kind filters using same APIs,
instead of creating each API set for each kind of filter.
 - rte_eth_dev_filter_supported
 - rte_eth_dev_filter_ctrl

Filter types, operations, and structures are defined specifically in new
header file lib/librte_eth/rte_dev_ctrl.h.

As to the implementation discussion, please refer to
http://dpdk.org/ml/archives/dev/2014-September/005179.html

Signed-off-by: Jingjing Wu <jingjing.wu-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 lib/librte_ether/Makefile       |  1 +
 lib/librte_ether/rte_eth_ctrl.h | 80 +++++++++++++++++++++++++++++++++++++++++
 lib/librte_ether/rte_ethdev.c   | 32 +++++++++++++++++
 lib/librte_ether/rte_ethdev.h   | 44 +++++++++++++++++++++++
 4 files changed, 157 insertions(+)
 create mode 100644 lib/librte_ether/rte_eth_ctrl.h

diff --git a/lib/librte_ether/Makefile b/lib/librte_ether/Makefile
index b310f8b..a461c31 100644
--- a/lib/librte_ether/Makefile
+++ b/lib/librte_ether/Makefile
@@ -46,6 +46,7 @@ SRCS-y += rte_ethdev.c
 #
 SYMLINK-y-include += rte_ether.h
 SYMLINK-y-include += rte_ethdev.h
+SYMLINK-y-include += rte_eth_ctrl.h
 
 # this lib depends upon:
 DEPDIRS-y += lib/librte_eal lib/librte_mempool lib/librte_ring lib/librte_mbuf
diff --git a/lib/librte_ether/rte_eth_ctrl.h b/lib/librte_ether/rte_eth_ctrl.h
new file mode 100644
index 0000000..4aedbe7
--- /dev/null
+++ b/lib/librte_ether/rte_eth_ctrl.h
@@ -0,0 +1,80 @@
+/*-
+ *   BSD LICENSE
+ *
+ *   Copyright(c) 2010-2014 Intel Corporation. All rights reserved.
+ *   All rights reserved.
+ *
+ *   Redistribution and use in source and binary forms, with or without
+ *   modification, are permitted provided that the following conditions
+ *   are met:
+ *
+ *     * Redistributions of source code must retain the above copyright
+ *       notice, this list of conditions and the following disclaimer.
+ *     * Redistributions in binary form must reproduce the above copyright
+ *       notice, this list of conditions and the following disclaimer in
+ *       the documentation and/or other materials provided with the
+ *       distribution.
+ *     * Neither the name of Intel Corporation nor the names of its
+ *       contributors may be used to endorse or promote products derived
+ *       from this software without specific prior written permission.
+ *
+ *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#ifndef _RTE_ETH_CTRL_H_
+#define _RTE_ETH_CTRL_H_
+
+/**
+ * @file
+ *
+ * Ethernet device features and related data structures used
+ * by control APIs should be defined in this file.
+ *
+ */
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+/**
+ * Feature filter types
+ */
+enum rte_filter_type {
+	RTE_ETH_FILTER_NONE = 0,
+	RTE_ETH_FILTER_HASH,
+	RTE_ETH_FILTER_FDIR,
+	RTE_ETH_FILTER_MAX,
+};
+
+/**
+ * Generic operations on filters
+ */
+enum rte_filter_op {
+	RTE_ETH_FILTER_OP_NONE = 0,
+	/**< used to check whether the type filter is supported */
+	RTE_ETH_FILTER_ADD,      /**< add filter entry */
+	RTE_ETH_FILTER_UPDATE,   /**< update filter entry */
+	RTE_ETH_FILTER_DELETE,   /**< delete filter entry */
+	RTE_ETH_FILTER_FLUSH,    /**< flush all entries */
+	RTE_ETH_FILTER_GET,      /**< get filter entry */
+	RTE_ETH_FILTER_SET,      /**< configurations */
+	RTE_ETH_FILTER_GET_INFO,
+	/**< get information of filter, such as status or statistics */
+	RTE_ETH_FILTER_OP_MAX,
+};
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif /* _RTE_ETH_CTRL_H_ */
diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index 1659340..1edc816 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -3148,3 +3148,35 @@ rte_eth_dev_get_flex_filter(uint8_t port_id, uint16_t index,
 	return (*dev->dev_ops->get_flex_filter)(dev, index, filter,
 						rx_queue);
 }
+
+int
+rte_eth_dev_filter_supported(uint8_t port_id, enum rte_filter_type filter_type)
+{
+	struct rte_eth_dev *dev;
+
+	if (port_id >= nb_ports) {
+		PMD_DEBUG_TRACE("Invalid port_id=%d\n", port_id);
+		return -ENODEV;
+	}
+
+	dev = &rte_eth_devices[port_id];
+	FUNC_PTR_OR_ERR_RET(*dev->dev_ops->filter_ctrl, -ENOTSUP);
+	return (*dev->dev_ops->filter_ctrl)(dev, filter_type,
+				RTE_ETH_FILTER_OP_NONE, NULL);
+}
+
+int
+rte_eth_dev_filter_ctrl(uint8_t port_id, enum rte_filter_type filter_type,
+		       enum rte_filter_op filter_op, void *arg)
+{
+	struct rte_eth_dev *dev;
+
+	if (port_id >= nb_ports) {
+		PMD_DEBUG_TRACE("Invalid port_id=%d\n", port_id);
+		return -ENODEV;
+	}
+
+	dev = &rte_eth_devices[port_id];
+	FUNC_PTR_OR_ERR_RET(*dev->dev_ops->filter_ctrl, -ENOTSUP);
+	return (*dev->dev_ops->filter_ctrl)(dev, filter_type, filter_op, arg);
+}
diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index 13be711..9a83e4d 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -177,6 +177,7 @@ extern "C" {
 #include <rte_pci.h>
 #include <rte_mbuf.h>
 #include "rte_ether.h"
+#include "rte_eth_ctrl.h"
 
 /**
  * A structure used to retrieve statistics for an Ethernet port.
@@ -1385,6 +1386,12 @@ typedef int (*eth_get_flex_filter_t)(struct rte_eth_dev *dev,
 			uint16_t *rx_queue);
 /**< @internal Get a flex filter rule on an Ethernet device */
 
+typedef int (*eth_filter_ctrl_t)(struct rte_eth_dev *dev,
+				 enum rte_filter_type filter_type,
+				 enum rte_filter_op filter_op,
+				 void *arg);
+/**< @internal Take operations to assigned filter type on an Ethernet device */
+
 /**
  * @internal A structure containing the functions exported by an Ethernet driver.
  */
@@ -1493,6 +1500,7 @@ struct eth_dev_ops {
 	eth_add_flex_filter_t          add_flex_filter;      /**< add flex filter. */
 	eth_remove_flex_filter_t       remove_flex_filter;   /**< remove flex filter. */
 	eth_get_flex_filter_t          get_flex_filter;      /**< get flex filter. */
+	eth_filter_ctrl_t              filter_ctrl;          /**< common filter control*/
 };
 
 /**
@@ -3622,6 +3630,42 @@ int rte_eth_dev_remove_flex_filter(uint8_t port_id, uint16_t index);
 int rte_eth_dev_get_flex_filter(uint8_t port_id, uint16_t index,
 			struct rte_flex_filter *filter, uint16_t *rx_queue);
 
+/**
+ * Check whether the filter type is supported on an Ethernet device.
+ * All the supported filter types are defined in 'rte_eth_ctrl.h'.
+ *
+ * @param port_id
+ *   The port identifier of the Ethernet device.
+ * @param filter_type
+ *   Filter type.
+ * @return
+ *   - (0) if successful.
+ *   - (-ENOTSUP) if hardware doesn't support this filter type.
+ *   - (-ENODEV) if *port_id* invalid.
+ */
+int rte_eth_dev_filter_supported(uint8_t port_id, enum rte_filter_type filter_type);
+
+/**
+ * Take operations to assigned filter type on an Ethernet device.
+ * All the supported operations and filter types are defined in 'rte_eth_ctrl.h'.
+ *
+ * @param port_id
+ *   The port identifier of the Ethernet device.
+ * @param filter_type
+ *   Filter type.
+ * @param filter_op
+ *   Type of operation.
+ * @param arg
+ *   A pointer to arguments defined specifically for the operation.
+ * @return
+ *   - (0) if successful.
+ *   - (-ENOTSUP) if hardware doesn't support.
+ *   - (-ENODEV) if *port_id* invalid.
+ *   - others depends on the specific operations implementation.
+ */
+int rte_eth_dev_filter_ctrl(uint8_t port_id, enum rte_filter_type filter_type,
+			enum rte_filter_op filter_op, void *arg);
+
 #ifdef __cplusplus
 }
 #endif
-- 
1.8.1.4

[PATCH v2 2/2] i40e: define filter_ctrl ops in i40e driver

[Jingjing Wu]

Provides filter_ctrl ops in i40e driver.
 - i40e_dev_filter_ctrl
For kinds of filter, only provides empty functions.
Contributors can enrich them based on filter features on fortville.

Signed-off-by: Jingjing Wu <jingjing.wu-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 lib/librte_pmd_i40e/i40e_ethdev.c | 63 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

diff --git a/lib/librte_pmd_i40e/i40e_ethdev.c b/lib/librte_pmd_i40e/i40e_ethdev.c
index dbf231f..395ca0e 100644
--- a/lib/librte_pmd_i40e/i40e_ethdev.c
+++ b/lib/librte_pmd_i40e/i40e_ethdev.c
@@ -216,6 +216,16 @@ static int i40e_dev_rss_hash_update(struct rte_eth_dev *dev,
 				    struct rte_eth_rss_conf *rss_conf);
 static int i40e_dev_rss_hash_conf_get(struct rte_eth_dev *dev,
 				      struct rte_eth_rss_conf *rss_conf);
+static int i40e_hash_filter_ctrl(struct rte_eth_dev *dev,
+				 enum rte_filter_op filter_op,
+				 void *arg);
+static int i40e_fdir_filter_ctrl(struct rte_eth_dev *dev,
+				 enum rte_filter_op filter_op,
+				 void *arg);
+static int i40e_dev_filter_ctrl(struct rte_eth_dev *dev,
+				enum rte_filter_type filter_type,
+				enum rte_filter_op filter_op,
+				void *arg);
 
 /* Default hash key buffer for RSS */
 static uint32_t rss_key_default[I40E_PFQF_HKEY_MAX_INDEX + 1];
@@ -267,6 +277,7 @@ static struct eth_dev_ops i40e_eth_dev_ops = {
 	.reta_query                   = i40e_dev_rss_reta_query,
 	.rss_hash_update              = i40e_dev_rss_hash_update,
 	.rss_hash_conf_get            = i40e_dev_rss_hash_conf_get,
+	.filter_ctrl                  = i40e_dev_filter_ctrl,
 };
 
 static struct eth_driver rte_i40e_pmd = {
@@ -4169,3 +4180,55 @@ i40e_pf_config_mq_rx(struct i40e_pf *pf)
 
 	return 0;
 }
+
+/* Operations for hash function */
+static int
+i40e_hash_filter_ctrl(__rte_unused struct rte_eth_dev *dev,
+		      __rte_unused enum rte_filter_op filter_op,
+		      __rte_unused void *arg)
+{
+	PMD_INIT_FUNC_TRACE();
+	return -ENOSYS;
+}
+
+/* Operations for flow director */
+static int
+i40e_fdir_filter_ctrl(__rte_unused struct rte_eth_dev *dev,
+		      __rte_unused enum rte_filter_op filter_op,
+		      __rte_unused void *arg)
+{
+	PMD_INIT_FUNC_TRACE();
+	return -ENOSYS;
+}
+
+static int
+i40e_dev_filter_ctrl(struct rte_eth_dev *dev,
+		     enum rte_filter_type filter_type,
+		     enum rte_filter_op filter_op,
+		     void *arg)
+{
+	int ret = 0;
+
+	if (dev == NULL)
+		return -EINVAL;
+
+	switch (filter_type) {
+	case RTE_ETH_FILTER_HASH:
+		/* Hash filter processing */
+		/* add empty function here to avoid compile error */
+		ret = i40e_hash_filter_ctrl(dev, filter_op, arg);
+		break;
+	case RTE_ETH_FILTER_FDIR:
+		/* FDIR filter processing */
+		/* add empty function here to avoid compile error */
+		ret = i40e_fdir_filter_ctrl(dev, filter_op, arg);
+		break;
+	default:
+		PMD_DRV_LOG(WARNING, "Filter type (%d) not supported",
+							filter_type);
+		ret = -EINVAL;
+		break;
+	}
+
+	return ret;
+}
-- 
1.8.1.4

Re: [PATCH v2 0/2] new filter APIs definition

[Thomas Monjalon]

2014-10-20 13:40, Jingjing Wu:
> new filter APIs definition in ethdev
> define filter_ctrl ops in i40e driver
> 
> v2 changes:
>   remove OP from the name of filter opeartions
>   add API implementation in i40e.
>   correct comments
> 
> Jingjing Wu (2):
>   librte_ether: new filter APIs definition
>   i40e: define filter_ctrl ops in i40e driver

As Bruce suggested, RTE_ETH_FILTER_NOP is simpler than RTE_ETH_FILTER_OP_NONE.
And I think RTE_ETH_FILTER_GET_INFO could be RTE_ETH_FILTER_INFO.
Last comment: filtering features (fdir, hash) should not be defined at this stage.

I know that we want this patch integrated with high priority, so I made the above
changes by myself. Hope you'll agree.

Acked-by: Thomas Monjalon <thomas.monjalon-pdR9zngts4EAvxtiuMwx3w@public.gmane.org>

Applied

Thank you for your patience
-- 
Thomas