[dpdk-dev,02/18] net/ixgbe: store flow director filter
Checks
Commit Message
From: wei zhao1 <wei.zhao1@intel.com>
Add support for storing flow director filter in SW.
Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
Signed-off-by: wei zhao1 <wei.zhao1@intel.com>
---
drivers/net/ixgbe/ixgbe_ethdev.c | 48 ++++++++++++++++++
drivers/net/ixgbe/ixgbe_ethdev.h | 19 ++++++-
drivers/net/ixgbe/ixgbe_fdir.c | 105 ++++++++++++++++++++++++++++++++++++++-
3 files changed, 169 insertions(+), 3 deletions(-)
Comments
On 12/2/2016 10:42 AM, Wei Zhao wrote:
> From: wei zhao1 <wei.zhao1@intel.com>
>
> Add support for storing flow director filter in SW.
>
> Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
> Signed-off-by: wei zhao1 <wei.zhao1@intel.com>
> ---
> drivers/net/ixgbe/ixgbe_ethdev.c | 48 ++++++++++++++++++
> drivers/net/ixgbe/ixgbe_ethdev.h | 19 ++++++-
> drivers/net/ixgbe/ixgbe_fdir.c | 105 ++++++++++++++++++++++++++++++++++++++-
> 3 files changed, 169 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
> index 7f10cca..f8e5fe1 100644
> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
<...>
> @@ -1289,6 +1302,25 @@ eth_ixgbe_dev_init(struct rte_eth_dev *eth_dev)
>
> /* initialize SYN filter */
> filter_info->syn_info = 0;
> + /* initialize flow director filter list & hash */
> + TAILQ_INIT(&fdir_info->fdir_list);
> + snprintf(fdir_hash_name, RTE_HASH_NAMESIZE,
> + "fdir_%s", eth_dev->data->name);
> + fdir_info->hash_handle = rte_hash_create(&fdir_hash_params);
Do we really create a hash table in device init? Is there a way to do
this if we know user will use it?
> + if (!fdir_info->hash_handle) {
> + PMD_INIT_LOG(ERR, "Failed to create fdir hash table!");
> + return -EINVAL;
And should we exit here? What if user will not use flow director at all?
> + }
> + fdir_info->hash_map = rte_zmalloc("ixgbe",
> + sizeof(struct ixgbe_fdir_filter *) *
> + IXGBE_MAX_FDIR_FILTER_NUM,
> + 0);
> + if (!fdir_info->hash_map) {
> + PMD_INIT_LOG(ERR,
> + "Failed to allocate memory for fdir hash map!");
> + return -ENOMEM;
> + }
> +
Can you please extract these into functions, to have more clear
init/uninit functions?
> return 0;
> }
>
> @@ -1297,6 +1329,9 @@ eth_ixgbe_dev_uninit(struct rte_eth_dev *eth_dev)
> {
> struct rte_pci_device *pci_dev;
> struct ixgbe_hw *hw;
> + struct ixgbe_hw_fdir_info *fdir_info =
> + IXGBE_DEV_PRIVATE_TO_FDIR_INFO(eth_dev->data->dev_private);
> + struct ixgbe_fdir_filter *fdir_filter;
>
> PMD_INIT_FUNC_TRACE();
>
> @@ -1330,6 +1365,19 @@ eth_ixgbe_dev_uninit(struct rte_eth_dev *eth_dev)
> rte_free(eth_dev->data->hash_mac_addrs);
> eth_dev->data->hash_mac_addrs = NULL;
>
> + /* remove all the fdir filters & hash */
> + if (fdir_info->hash_map)
> + rte_free(fdir_info->hash_map);
> + if (fdir_info->hash_handle)
> + rte_hash_free(fdir_info->hash_handle);
All rte_hash_xxx() APIs gives build error for shared library build, [1]
needs to be added into Makefile.
But, this makes hash library a dependency to the PMD, do you think is
there a way to escape from this?
[1]
+DEPDIRS-$(CONFIG_RTE_LIBRTE_IXGBE_PMD) += lib/librte_net lib/librte_hash
> +
> + while ((fdir_filter = TAILQ_FIRST(&fdir_info->fdir_list))) {
> + TAILQ_REMOVE(&fdir_info->fdir_list,
> + fdir_filter,
> + entries);
> + rte_free(fdir_filter);
> + }
> +
> return 0;
> }
>
<...>
Hi, Ferruh
> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Wednesday, December 21, 2016 12:58 AM
> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>
> Subject: Re: [dpdk-dev] [PATCH 02/18] net/ixgbe: store flow director filter
>
> On 12/2/2016 10:42 AM, Wei Zhao wrote:
> > From: wei zhao1 <wei.zhao1@intel.com>
> >
> > Add support for storing flow director filter in SW.
> >
> > Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
> > Signed-off-by: wei zhao1 <wei.zhao1@intel.com>
> > ---
> > drivers/net/ixgbe/ixgbe_ethdev.c | 48 ++++++++++++++++++
> > drivers/net/ixgbe/ixgbe_ethdev.h | 19 ++++++-
> > drivers/net/ixgbe/ixgbe_fdir.c | 105
> ++++++++++++++++++++++++++++++++++++++-
> > 3 files changed, 169 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
> > b/drivers/net/ixgbe/ixgbe_ethdev.c
> > index 7f10cca..f8e5fe1 100644
> > --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> > +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> <...>
> > @@ -1289,6 +1302,25 @@ eth_ixgbe_dev_init(struct rte_eth_dev
> *eth_dev)
> >
> > /* initialize SYN filter */
> > filter_info->syn_info = 0;
> > + /* initialize flow director filter list & hash */
> > + TAILQ_INIT(&fdir_info->fdir_list);
> > + snprintf(fdir_hash_name, RTE_HASH_NAMESIZE,
> > + "fdir_%s", eth_dev->data->name);
> > + fdir_info->hash_handle = rte_hash_create(&fdir_hash_params);
>
> Do we really create a hash table in device init? Is there a way to do this if we
> know user will use it?
By now, we have no idea about whether user wiil use flow director or not.
So, our strategy is init all types of filter and give option to users.
>
> > + if (!fdir_info->hash_handle) {
> > + PMD_INIT_LOG(ERR, "Failed to create fdir hash table!");
> > + return -EINVAL;
>
> And should we exit here? What if user will not use flow director at all?
Maybe, we can delete "return -EINVAL;" and only print error log as a warning.
But some uer maybe do not pay attention to warning error log sometimes, so we chose to
exit if any init fail.
>
> > + }
> > + fdir_info->hash_map = rte_zmalloc("ixgbe",
> > + sizeof(struct ixgbe_fdir_filter *) *
> > + IXGBE_MAX_FDIR_FILTER_NUM,
> > + 0);
> > + if (!fdir_info->hash_map) {
> > + PMD_INIT_LOG(ERR,
> > + "Failed to allocate memory for fdir hash map!");
> > + return -ENOMEM;
> > + }
> > +
>
> Can you please extract these into functions, to have more clear init/uninit
> functions?
ok, that seems like a good idea to creat two new functions to do initialize of flow director and l2 tunnel fliter.
That will make dev init function more clear. I wiil add two init function for this purpose in v2.
>
> > return 0;
> > }
> >
> > @@ -1297,6 +1329,9 @@ eth_ixgbe_dev_uninit(struct rte_eth_dev
> > *eth_dev) {
> > struct rte_pci_device *pci_dev;
> > struct ixgbe_hw *hw;
> > + struct ixgbe_hw_fdir_info *fdir_info =
> > + IXGBE_DEV_PRIVATE_TO_FDIR_INFO(eth_dev->data-
> >dev_private);
> > + struct ixgbe_fdir_filter *fdir_filter;
> >
> > PMD_INIT_FUNC_TRACE();
> >
> > @@ -1330,6 +1365,19 @@ eth_ixgbe_dev_uninit(struct rte_eth_dev
> *eth_dev)
> > rte_free(eth_dev->data->hash_mac_addrs);
> > eth_dev->data->hash_mac_addrs = NULL;
> >
> > + /* remove all the fdir filters & hash */
> > + if (fdir_info->hash_map)
> > + rte_free(fdir_info->hash_map);
> > + if (fdir_info->hash_handle)
> > + rte_hash_free(fdir_info->hash_handle);
>
> All rte_hash_xxx() APIs gives build error for shared library build, [1] needs to
> be added into Makefile.
>
> But, this makes hash library a dependency to the PMD, do you think is there
> a way to escape from this?
>
> [1]
> +DEPDIRS-$(CONFIG_RTE_LIBRTE_IXGBE_PMD) += lib/librte_net
> +lib/librte_hash
>
Ok, I will change as your suggestion in v2.
> > +
> > + while ((fdir_filter = TAILQ_FIRST(&fdir_info->fdir_list))) {
> > + TAILQ_REMOVE(&fdir_info->fdir_list,
> > + fdir_filter,
> > + entries);
> > + rte_free(fdir_filter);
> > + }
> > +
> > return 0;
> > }
> >
> <...>
@@ -60,6 +60,7 @@
#include <rte_malloc.h>
#include <rte_random.h>
#include <rte_dev.h>
+#include <rte_hash_crc.h>
#include "ixgbe_logs.h"
#include "base/ixgbe_api.h"
@@ -1094,10 +1095,22 @@ eth_ixgbe_dev_init(struct rte_eth_dev *eth_dev)
IXGBE_DEV_PRIVATE_TO_DCB_CFG(eth_dev->data->dev_private);
struct ixgbe_filter_info *filter_info =
IXGBE_DEV_PRIVATE_TO_FILTER_INFO(eth_dev->data->dev_private);
+ struct ixgbe_hw_fdir_info *fdir_info =
+ IXGBE_DEV_PRIVATE_TO_FDIR_INFO(eth_dev->data->dev_private);
uint32_t ctrl_ext;
uint16_t csum;
int diag, i;
+ char fdir_hash_name[RTE_HASH_NAMESIZE];
+ struct rte_hash_parameters fdir_hash_params = {
+ .name = fdir_hash_name,
+ .entries = IXGBE_MAX_FDIR_FILTER_NUM,
+ .key_len = sizeof(union ixgbe_atr_input),
+ .hash_func = rte_hash_crc,
+ .hash_func_init_val = 0,
+ .socket_id = rte_socket_id(),
+ };
+
PMD_INIT_FUNC_TRACE();
eth_dev->dev_ops = &ixgbe_eth_dev_ops;
@@ -1289,6 +1302,25 @@ eth_ixgbe_dev_init(struct rte_eth_dev *eth_dev)
/* initialize SYN filter */
filter_info->syn_info = 0;
+ /* initialize flow director filter list & hash */
+ TAILQ_INIT(&fdir_info->fdir_list);
+ snprintf(fdir_hash_name, RTE_HASH_NAMESIZE,
+ "fdir_%s", eth_dev->data->name);
+ fdir_info->hash_handle = rte_hash_create(&fdir_hash_params);
+ if (!fdir_info->hash_handle) {
+ PMD_INIT_LOG(ERR, "Failed to create fdir hash table!");
+ return -EINVAL;
+ }
+ fdir_info->hash_map = rte_zmalloc("ixgbe",
+ sizeof(struct ixgbe_fdir_filter *) *
+ IXGBE_MAX_FDIR_FILTER_NUM,
+ 0);
+ if (!fdir_info->hash_map) {
+ PMD_INIT_LOG(ERR,
+ "Failed to allocate memory for fdir hash map!");
+ return -ENOMEM;
+ }
+
return 0;
}
@@ -1297,6 +1329,9 @@ eth_ixgbe_dev_uninit(struct rte_eth_dev *eth_dev)
{
struct rte_pci_device *pci_dev;
struct ixgbe_hw *hw;
+ struct ixgbe_hw_fdir_info *fdir_info =
+ IXGBE_DEV_PRIVATE_TO_FDIR_INFO(eth_dev->data->dev_private);
+ struct ixgbe_fdir_filter *fdir_filter;
PMD_INIT_FUNC_TRACE();
@@ -1330,6 +1365,19 @@ eth_ixgbe_dev_uninit(struct rte_eth_dev *eth_dev)
rte_free(eth_dev->data->hash_mac_addrs);
eth_dev->data->hash_mac_addrs = NULL;
+ /* remove all the fdir filters & hash */
+ if (fdir_info->hash_map)
+ rte_free(fdir_info->hash_map);
+ if (fdir_info->hash_handle)
+ rte_hash_free(fdir_info->hash_handle);
+
+ while ((fdir_filter = TAILQ_FIRST(&fdir_info->fdir_list))) {
+ TAILQ_REMOVE(&fdir_info->fdir_list,
+ fdir_filter,
+ entries);
+ rte_free(fdir_filter);
+ }
+
return 0;
}
@@ -38,6 +38,7 @@
#include "base/ixgbe_dcb_82598.h"
#include "ixgbe_bypass.h"
#include <rte_time.h>
+#include <rte_hash.h>
/* need update link, bit flag */
#define IXGBE_FLAG_NEED_LINK_UPDATE (uint32_t)(1 << 0)
@@ -130,10 +131,11 @@
#define IXGBE_MISC_VEC_ID RTE_INTR_VEC_ZERO_OFFSET
#define IXGBE_RX_VEC_START RTE_INTR_VEC_RXTX_OFFSET
+#define IXGBE_MAX_FDIR_FILTER_NUM (1024 * 32)
+
/*
* Information about the fdir mode.
*/
-
struct ixgbe_hw_fdir_mask {
uint16_t vlan_tci_mask;
uint32_t src_ipv4_mask;
@@ -148,6 +150,17 @@ struct ixgbe_hw_fdir_mask {
uint8_t tunnel_type_mask;
};
+struct ixgbe_fdir_filter {
+ TAILQ_ENTRY(ixgbe_fdir_filter) entries;
+ union ixgbe_atr_input ixgbe_fdir; /* key of fdir filter*/
+ uint32_t fdirflags; /* drop or forward */
+ uint32_t fdirhash; /* hash value for fdir */
+ uint8_t queue; /* assigned rx queue */
+};
+
+/* list of fdir filters */
+TAILQ_HEAD(ixgbe_fdir_filter_list, ixgbe_fdir_filter);
+
struct ixgbe_hw_fdir_info {
struct ixgbe_hw_fdir_mask mask;
uint8_t flex_bytes_offset;
@@ -159,6 +172,10 @@ struct ixgbe_hw_fdir_info {
uint64_t remove;
uint64_t f_add;
uint64_t f_remove;
+ struct ixgbe_fdir_filter_list fdir_list; /* filter list*/
+ /* store the pointers of the filters, index is the hash value. */
+ struct ixgbe_fdir_filter **hash_map;
+ struct rte_hash *hash_handle; /* cuckoo hash handler */
};
/* structure for interrupt relative data */
@@ -43,6 +43,7 @@
#include <rte_pci.h>
#include <rte_ether.h>
#include <rte_ethdev.h>
+#include <rte_malloc.h>
#include "ixgbe_logs.h"
#include "base/ixgbe_api.h"
@@ -1075,6 +1076,65 @@ fdir_erase_filter_82599(struct ixgbe_hw *hw, uint32_t fdirhash)
}
+static inline struct ixgbe_fdir_filter *
+ixgbe_fdir_filter_lookup(struct ixgbe_hw_fdir_info *fdir_info,
+ union ixgbe_atr_input *key)
+{
+ int ret = 0;
+
+ ret = rte_hash_lookup(fdir_info->hash_handle, (const void *)key);
+ if (ret < 0)
+ return NULL;
+
+ return fdir_info->hash_map[ret];
+}
+
+static inline int
+ixgbe_insert_fdir_filter(struct ixgbe_hw_fdir_info *fdir_info,
+ struct ixgbe_fdir_filter *fdir_filter)
+{
+ int ret = 0;
+
+ ret = rte_hash_add_key(fdir_info->hash_handle,
+ &fdir_filter->ixgbe_fdir);
+
+ if (ret < 0) {
+ PMD_DRV_LOG(ERR,
+ "Failed to insert fdir filter to hash table %d!",
+ ret);
+ return ret;
+ }
+
+ fdir_info->hash_map[ret] = fdir_filter;
+
+ TAILQ_INSERT_TAIL(&fdir_info->fdir_list, fdir_filter, entries);
+
+ return 0;
+}
+
+static inline int
+ixgbe_remove_fdir_filter(struct ixgbe_hw_fdir_info *fdir_info,
+ union ixgbe_atr_input *key)
+{
+ int ret = 0;
+ struct ixgbe_fdir_filter *fdir_filter;
+
+ ret = rte_hash_del_key(fdir_info->hash_handle, key);
+
+ if (ret < 0) {
+ PMD_DRV_LOG(ERR, "No such fdir filter to delete %d!", ret);
+ return ret;
+ }
+
+ fdir_filter = fdir_info->hash_map[ret];
+ fdir_info->hash_map[ret] = NULL;
+
+ TAILQ_REMOVE(&fdir_info->fdir_list, fdir_filter, entries);
+ rte_free(fdir_filter);
+
+ return 0;
+}
+
/*
* ixgbe_add_del_fdir_filter - add or remove a flow diretor filter.
* @dev: pointer to the structure rte_eth_dev
@@ -1098,6 +1158,8 @@ ixgbe_add_del_fdir_filter(struct rte_eth_dev *dev,
struct ixgbe_hw_fdir_info *info =
IXGBE_DEV_PRIVATE_TO_FDIR_INFO(dev->data->dev_private);
enum rte_fdir_mode fdir_mode = dev->data->dev_conf.fdir_conf.mode;
+ struct ixgbe_fdir_filter *node;
+ bool add_node = FALSE;
if (fdir_mode == RTE_FDIR_MODE_NONE)
return -ENOTSUP;
@@ -1148,6 +1210,10 @@ ixgbe_add_del_fdir_filter(struct rte_eth_dev *dev,
dev->data->dev_conf.fdir_conf.pballoc);
if (del) {
+ err = ixgbe_remove_fdir_filter(info, &input);
+ if (err < 0)
+ return err;
+
err = fdir_erase_filter_82599(hw, fdirhash);
if (err < 0)
PMD_DRV_LOG(ERR, "Fail to delete FDIR filter!");
@@ -1172,6 +1238,37 @@ ixgbe_add_del_fdir_filter(struct rte_eth_dev *dev,
else
return -EINVAL;
+ node = ixgbe_fdir_filter_lookup(info, &input);
+ if (node) {
+ if (update) {
+ node->fdirflags = fdircmd_flags;
+ node->fdirhash = fdirhash;
+ node->queue = queue;
+ } else {
+ PMD_DRV_LOG(ERR, "Conflict with existing fdir filter!");
+ return -EINVAL;
+ }
+ } else {
+ add_node = TRUE;
+ node = rte_zmalloc("ixgbe_fdir",
+ sizeof(struct ixgbe_fdir_filter),
+ 0);
+ if (!node)
+ return -ENOMEM;
+ (void)rte_memcpy(&node->ixgbe_fdir,
+ &input,
+ sizeof(union ixgbe_atr_input));
+ node->fdirflags = fdircmd_flags;
+ node->fdirhash = fdirhash;
+ node->queue = queue;
+
+ err = ixgbe_insert_fdir_filter(info, node);
+ if (err < 0) {
+ rte_free(node);
+ return err;
+ }
+ }
+
if (is_perfect) {
err = fdir_write_perfect_filter_82599(hw, &input, queue,
fdircmd_flags, fdirhash,
@@ -1180,10 +1277,14 @@ ixgbe_add_del_fdir_filter(struct rte_eth_dev *dev,
err = fdir_add_signature_filter_82599(hw, &input, queue,
fdircmd_flags, fdirhash);
}
- if (err < 0)
+ if (err < 0) {
PMD_DRV_LOG(ERR, "Fail to add FDIR filter!");
- else
+
+ if (add_node)
+ (void)ixgbe_remove_fdir_filter(info, &input);
+ } else {
PMD_DRV_LOG(DEBUG, "Success to add FDIR filter");
+ }
return err;
}