> -----Original Message-----
> From: Bie, Tiwei
> Sent: Wednesday, December 28, 2016 11:39 AM
> To: Xing, Beilei <beilei.x...@intel.com>
> Cc: Wu, Jingjing <jingjing...@intel.com>; Zhang, Helin
> <helin.zh...@intel.com>; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2 03/17] net/i40e: store flow director filter
>
> On Tue, Dec 27, 2016 at 02:26:10PM +0800, Beilei Xing wrote:
> > Currently there's no flow director filter stored in SW. This patch
> > stores flow director filters in SW with cuckoo hash, also adds
> > protection if a flow director filter has been added.
> >
> > Signed-off-by: Beilei Xing <beilei.x...@intel.com>
> > ---
> > drivers/net/i40e/i40e_ethdev.c | 48 +++++++++++++++++++++
> > drivers/net/i40e/i40e_ethdev.h | 12 ++++++
> > drivers/net/i40e/i40e_fdir.c | 98
> ++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 158 insertions(+)
> >
> > diff --git a/drivers/net/i40e/i40e_ethdev.c
> > b/drivers/net/i40e/i40e_ethdev.c index c012d5d..427ebdc 100644
> > --- a/drivers/net/i40e/i40e_ethdev.c
> > +++ b/drivers/net/i40e/i40e_ethdev.c
> [...]
> > @@ -1342,6 +1379,17 @@ eth_i40e_dev_uninit(struct rte_eth_dev *dev)
> > rte_free(p_tunnel);
> > }
> >
> > + /* Remove all flow director rules and hash */
> > + if (fdir_info->hash_map)
> > + rte_free(fdir_info->hash_map);
> > + if (fdir_info->hash_table)
> > + rte_hash_free(fdir_info->hash_table);
> > +
> > + while ((p_fdir = TAILQ_FIRST(&fdir_info->fdir_list))) {
>
> There is a redundant pair of parentheses, or you should compare with NULL.
I think the another parentheses is used to compare with NULL. In fact there's
similar using in PMD.
>
> > + TAILQ_REMOVE(&fdir_info->fdir_list, p_fdir, rules);
> > + rte_free(p_fdir);
> > + }
> > +
> > dev->dev_ops = NULL;
> > dev->rx_pkt_burst = NULL;
> > dev->tx_pkt_burst = NULL;
> [...]
> > diff --git a/drivers/net/i40e/i40e_fdir.c
> > b/drivers/net/i40e/i40e_fdir.c index 335bf15..faa2495 100644
> > --- a/drivers/net/i40e/i40e_fdir.c
> > +++ b/drivers/net/i40e/i40e_fdir.c
> [...]
> > +/* Check if there exists the flow director filter */ static struct
> > +i40e_fdir_filter * i40e_sw_fdir_filter_lookup(struct i40e_fdir_info
> > +*fdir_info,
> > + const struct rte_eth_fdir_input *input) {
> > + int ret = 0;
> > +
>
> The initialization is meaningless, as it will be written by the below
> assignment unconditionally.
Yes, you're right.
>
> > + ret = rte_hash_lookup(fdir_info->hash_table, (const void *)input);
> > + if (ret < 0)
> > + return NULL;
> > +
> > + return fdir_info->hash_map[ret];
> > +}
> > +
> > +/* Add a flow director filter into the SW list */
> > +static int
> > +i40e_sw_fdir_filter_insert(struct i40e_pf *pf, struct i40e_fdir_filter
> > *filter)
> > +{
> > + struct i40e_fdir_info *fdir_info = &pf->fdir;
> > + int ret = 0;
> > +
>
> Same here.
>
> > + ret = rte_hash_add_key(fdir_info->hash_table,
> > + &filter->fdir.input);
> > + if (ret < 0)
> > + PMD_DRV_LOG(ERR,
> > + "Failed to insert fdir filter to hash table %d!",
> > + ret);
>
> Function should return when ret < 0.
Thanks for catching it.
>
> > + fdir_info->hash_map[ret] = filter;
> > +
> > + TAILQ_INSERT_TAIL(&fdir_info->fdir_list, filter, rules);
> > +
> > + return 0;
> > +}
> > +
> > +/* Delete a flow director filter from the SW list */
> > +static int
> > +i40e_sw_fdir_filter_del(struct i40e_pf *pf, struct i40e_fdir_filter
> > *filter)
> > +{
> > + struct i40e_fdir_info *fdir_info = &pf->fdir;
> > + int ret = 0;
> > +
>
> Same here.
>
> > + ret = rte_hash_del_key(fdir_info->hash_table,
> > + &filter->fdir.input);
> > + if (ret < 0)
> > + PMD_DRV_LOG(ERR,
> > + "Failed to delete fdir filter to hash table %d!",
> > + ret);
>
> Function should return when ret < 0.
>
> > + fdir_info->hash_map[ret] = NULL;
> > +
> > + TAILQ_REMOVE(&fdir_info->fdir_list, filter, rules);
> > + rte_free(filter);
> > +
> > + return 0;
> > +}
> > +
> > /*
> > * i40e_add_del_fdir_filter - add or remove a flow director filter.
> > * @pf: board private structure
> > @@ -1032,6 +1105,8 @@ i40e_add_del_fdir_filter(struct rte_eth_dev *dev,
> > struct i40e_pf *pf = I40E_DEV_PRIVATE_TO_PF(dev->data-
> >dev_private);
> > unsigned char *pkt = (unsigned char *)pf->fdir.prg_pkt;
> > enum i40e_filter_pctype pctype;
> > + struct i40e_fdir_info *fdir_info = &pf->fdir;
> > + struct i40e_fdir_filter *fdir_filter, *node;
> > int ret = 0;
> >
> > if (dev->data->dev_conf.fdir_conf.mode !=
> RTE_FDIR_MODE_PERFECT) {
> > @@ -1054,6 +1129,21 @@ i40e_add_del_fdir_filter(struct rte_eth_dev
> *dev,
> > return -EINVAL;
> > }
> >
> > + fdir_filter = rte_zmalloc("fdir_filter", sizeof(*fdir_filter), 0);
> > + i40e_fdir_filter_convert(filter, fdir_filter);
> > + node = i40e_sw_fdir_filter_lookup(fdir_info, &fdir_filter->fdir.input);
> > + if (add && node) {
> > + PMD_DRV_LOG(ERR,
> > + "Conflict with existing flow director rules!");
> > + rte_free(fdir_filter);
> > + return -EINVAL;
> > + } else if (!add && !node) {
>
> When `if (add && node)' is true, function will return. There is no need
> to use `else' here.
>
> Best regards,
> Tiwei Bie
