Re: [dpdk-dev] [PATCH v2 15/17] net/i40e: add flow flush function

[Tiwei Bie <tiwei.bie@intel.com>]

On Tue, Dec 27, 2016 at 02:26:22PM +0800, Beilei Xing wrote:
> This patch adds i40e_flow_flush function to flush all
> filters for users. And flow director flush function
> is involved first.
> 
> Signed-off-by: Beilei Xing <beilei.xing@intel.com>
> ---
>  drivers/net/i40e/i40e_ethdev.h |  3 +++
>  drivers/net/i40e/i40e_fdir.c   |  8 ++------
>  drivers/net/i40e/i40e_flow.c   | 46 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 51 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/i40e/i40e_ethdev.h b/drivers/net/i40e/i40e_ethdev.h
> index b8c7d41..0b736d5 100644
> --- a/drivers/net/i40e/i40e_ethdev.h
> +++ b/drivers/net/i40e/i40e_ethdev.h
> @@ -786,6 +786,9 @@ i40e_sw_tunnel_filter_lookup(struct i40e_tunnel_rule *tunnel_rule,
>  			     const struct i40e_tunnel_filter_input *input);
>  int i40e_sw_tunnel_filter_del(struct i40e_pf *pf,
>  			      struct i40e_tunnel_filter *tunnel_filter);
> +int i40e_sw_fdir_filter_del(struct i40e_pf *pf,
> +			    struct i40e_fdir_filter *filter);
> +int i40e_fdir_flush(struct rte_eth_dev *dev);
>  

Why don't declare them as the global functions at the beginning?

>  /* I40E_DEV_PRIVATE_TO */
>  #define I40E_DEV_PRIVATE_TO_PF(adapter) \
> diff --git a/drivers/net/i40e/i40e_fdir.c b/drivers/net/i40e/i40e_fdir.c
> index 6c1bb18..f10aeee 100644
> --- a/drivers/net/i40e/i40e_fdir.c
> +++ b/drivers/net/i40e/i40e_fdir.c
> @@ -119,8 +119,6 @@ static int i40e_fdir_filter_programming(struct i40e_pf *pf,
>  			enum i40e_filter_pctype pctype,
>  			const struct rte_eth_fdir_filter *filter,
>  			bool add);
> -static int i40e_fdir_flush(struct rte_eth_dev *dev);
> -
>  static int i40e_fdir_filter_convert(const struct rte_eth_fdir_filter *input,
>  			 struct i40e_fdir_filter *filter);
>  static struct i40e_fdir_filter *
> @@ -128,8 +126,6 @@ i40e_sw_fdir_filter_lookup(struct i40e_fdir_info *fdir_info,
>  			const struct rte_eth_fdir_input *input);
>  static int i40e_sw_fdir_filter_insert(struct i40e_pf *pf,
>  				   struct i40e_fdir_filter *filter);
> -static int i40e_sw_fdir_filter_del(struct i40e_pf *pf,
> -				struct i40e_fdir_filter *filter);
>  
>  static int
>  i40e_fdir_rx_queue_init(struct i40e_rx_queue *rxq)
> @@ -1070,7 +1066,7 @@ i40e_sw_fdir_filter_insert(struct i40e_pf *pf, struct i40e_fdir_filter *filter)
>  }
>  
>  /* Delete a flow director filter from the SW list */
> -static int
> +int
>  i40e_sw_fdir_filter_del(struct i40e_pf *pf, struct i40e_fdir_filter *filter)
>  {
>  	struct i40e_fdir_info *fdir_info = &pf->fdir;
> @@ -1318,7 +1314,7 @@ i40e_fdir_filter_programming(struct i40e_pf *pf,
>   * i40e_fdir_flush - clear all filters of Flow Director table
>   * @pf: board private structure
>   */
> -static int
> +int
>  i40e_fdir_flush(struct rte_eth_dev *dev)
>  {
>  	struct i40e_pf *pf = I40E_DEV_PRIVATE_TO_PF(dev->data->dev_private);
> diff --git a/drivers/net/i40e/i40e_flow.c b/drivers/net/i40e/i40e_flow.c
> index 4c7856c..1d9f603 100644
> --- a/drivers/net/i40e/i40e_flow.c
> +++ b/drivers/net/i40e/i40e_flow.c
> @@ -68,6 +68,8 @@ static struct rte_flow *i40e_flow_create(struct rte_eth_dev *dev,
>  					 const struct rte_flow_item pattern[],
>  					 const struct rte_flow_action actions[],
>  					 struct rte_flow_error *error);
> +static int i40e_flow_flush(struct rte_eth_dev *dev,
> +			   struct rte_flow_error *error);
>  static int i40e_flow_destroy(struct rte_eth_dev *dev,
>  			     struct rte_flow *flow,
>  			     struct rte_flow_error *error);
> @@ -95,11 +97,13 @@ static int i40e_dev_destroy_ethertype_filter(struct i40e_pf *pf,
>  				     struct i40e_ethertype_filter *filter);
>  static int i40e_dev_destroy_tunnel_filter(struct i40e_pf *pf,
>  					  struct i40e_tunnel_filter *filter);
> +static int i40e_fdir_filter_flush(struct i40e_pf *pf);
>  
>  const struct rte_flow_ops i40e_flow_ops = {
>  	.validate = i40e_flow_validate,
>  	.create = i40e_flow_create,
>  	.destroy = i40e_flow_destroy,
> +	.flush = i40e_flow_flush,
>  };
>  
>  enum rte_filter_type cons_filter_type = RTE_ETH_FILTER_NONE;
> @@ -1603,3 +1607,45 @@ i40e_dev_destroy_tunnel_filter(struct i40e_pf *pf,
>  
>  	return ret;
>  }
> +
> +static int
> +i40e_flow_flush(struct rte_eth_dev *dev, struct rte_flow_error *error)
> +{
> +	struct i40e_pf *pf = I40E_DEV_PRIVATE_TO_PF(dev->data->dev_private);
> +	int ret = 0;
> +

Meaningless initialization.

> +	ret = i40e_fdir_filter_flush(pf);
> +	if (!ret)
> +		rte_flow_error_set(error, EINVAL,
> +				   RTE_FLOW_ERROR_TYPE_HANDLE, NULL,
> +				   "Failed to flush FDIR flows.");

Just curious. What's the relationship between `ret' and the code (EINVAL)
passed to rte_flow_error_set()? Is `-ret' acceptable as the parameter?
Because the `-ret' which is actually returned by i40e_fdir_flush() is also
some standard UNIX errno. When error occurs, user should use which one to
figure out the failure reason? `-ret' or `rte_errno'?

> +
> +	return ret;
> +}
> +
> +static int
> +i40e_fdir_filter_flush(struct i40e_pf *pf)
> +{
> +	struct rte_eth_dev *dev = pf->adapter->eth_dev;
> +	struct i40e_fdir_info *fdir_info = &pf->fdir;
> +	struct i40e_fdir_filter *fdir_filter;
> +	struct i40e_flow *flow;
> +	int ret = 0;
> +

Meaningless initialization.

> +	ret = i40e_fdir_flush(dev);
> +	if (!ret) {
> +		/* Delete FDIR filters in FDIR list. */
> +		while ((fdir_filter = TAILQ_FIRST(&fdir_info->fdir_list)))
> +			i40e_sw_fdir_filter_del(pf, fdir_filter);
> +

The i40e_sw_fdir_filter_del() may fail, in which case fdir_filter won't
be removed from fdir_info->fdir_list. Will it lead to an infinite loop?
Should you check the retval of i40e_sw_fdir_filter_del() and break the
loop when it fails?

Best regards,
Tiwei Bie