Re: [dpdk-dev] [PATCH v2 12/14] ethdev: remove legacy FDIR filter type support

["Hyong Youb Kim (hyonkim)" <hyonkim@cisco.com>]

> -----Original Message-----
> From: Andrew Rybchenko <arybchenko@solarflare.com>
> Sent: Thursday, October 22, 2020 6:43 PM
> To: Wenzhuo Lu <wenzhuo.lu@intel.com>; Beilei Xing
> <beilei.xing@intel.com>; Bernard Iremonger
> <bernard.iremonger@intel.com>; Jeff Guo <jia.guo@intel.com>; Ray Kinsella
> <mdr@ashroe.eu>; Neil Horman <nhorman@tuxdriver.com>; Ajit Khaparde
> <ajit.khaparde@broadcom.com>; Somnath Kotur
> <somnath.kotur@broadcom.com>; John Daley (johndale)
> <johndale@cisco.com>; Hyong Youb Kim (hyonkim) <hyonkim@cisco.com>;
> Haiyue Wang <haiyue.wang@intel.com>; Matan Azrad <matan@nvidia.com>;
> Shahaf Shuler <shahafs@nvidia.com>; Viacheslav Ovsiienko
> <viacheslavo@nvidia.com>; Rasesh Mody <rmody@marvell.com>; Shahed
> Shaikh <shshaikh@marvell.com>; Andrew Rybchenko
> <andrew.rybchenko@oktetlabs.ru>; Thomas Monjalon
> <thomas@monjalon.net>; Ferruh Yigit <ferruh.yigit@intel.com>
> Cc: dev@dpdk.org
> Subject: [PATCH v2 12/14] ethdev: remove legacy FDIR filter type support
> 
> Instead of FDIR filters RTE flow API should be used.
> 
> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
> Acked-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
> Acked-by: Haiyue Wang <haiyue.wang@intel.com>
> ---
>  - cleanup testpmd user guide
>  - partially cleanup net/i40e documentation, please, review and
>    suggest how to fix remaining flow_director_filter command
[...]
>  drivers/net/enic/enic.h                     |   7 -
>  drivers/net/enic/enic_clsf.c                | 168 ----
>  drivers/net/enic/enic_ethdev.c              |  48 --
[...]
> diff --git a/drivers/net/enic/enic.h b/drivers/net/enic/enic.h
> index 5f0ae395da..079f194275 100644
> --- a/drivers/net/enic/enic.h
> +++ b/drivers/net/enic/enic.h
> @@ -394,12 +394,6 @@ enic_ring_incr(uint32_t n_descriptors, uint32_t idx)
>  }
> 
>  int dev_is_enic(struct rte_eth_dev *dev);
> -void enic_fdir_stats_get(struct enic *enic,
> -			 struct rte_eth_fdir_stats *stats);
> -int enic_fdir_add_fltr(struct enic *enic,
> -		       struct rte_eth_fdir_filter *params);
> -int enic_fdir_del_fltr(struct enic *enic,
> -		       struct rte_eth_fdir_filter *params);
>  void enic_free_wq(void *txq);
>  int enic_alloc_intr_resources(struct enic *enic);
>  int enic_setup_finish(struct enic *enic);
> @@ -464,7 +458,6 @@ bool enic_use_vector_rx_handler(struct rte_eth_dev
> *eth_dev);
>  void enic_pick_rx_handler(struct rte_eth_dev *eth_dev);
>  void enic_pick_tx_handler(struct rte_eth_dev *eth_dev);
>  void enic_fdir_info(struct enic *enic);
> -void enic_fdir_info_get(struct enic *enic, struct rte_eth_fdir_info *stats);
>  int enic_vf_representor_init(struct rte_eth_dev *eth_dev, void
> *init_params);
>  int enic_vf_representor_uninit(struct rte_eth_dev *ethdev);
>  int enic_fm_allocate_switch_domain(struct enic *pf);
> diff --git a/drivers/net/enic/enic_clsf.c b/drivers/net/enic/enic_clsf.c
> index e206123ba5..1c837a4d09 100644
> --- a/drivers/net/enic/enic_clsf.c
> +++ b/drivers/net/enic/enic_clsf.c
> @@ -42,17 +42,6 @@ static void copy_fltr_v2(struct filter_v2 *fltr,
>  		const struct rte_eth_fdir_input *input,
>  		const struct rte_eth_fdir_masks *masks);
> 
> -void enic_fdir_stats_get(struct enic *enic, struct rte_eth_fdir_stats *stats)
> -{
> -	*stats = enic->fdir.stats;
> -}
> -
> -void enic_fdir_info_get(struct enic *enic, struct rte_eth_fdir_info *info)
> -{
> -	info->mode = (enum rte_fdir_mode)enic->fdir.modes;
> -	info->flow_types_mask[0] = enic->fdir.types_mask;
> -}
> -
>  void enic_fdir_info(struct enic *enic)
>  {
>  	enic->fdir.modes = (uint32_t)RTE_FDIR_MODE_PERFECT;
> @@ -305,163 +294,6 @@ copy_fltr_v2(struct filter_v2 *fltr, const struct
> rte_eth_fdir_input *input,
>  	}
>  }
> 
> -int enic_fdir_del_fltr(struct enic *enic, struct rte_eth_fdir_filter *params)
> -{
> -	int32_t pos;
> -	struct enic_fdir_node *key;
> -	/* See if the key is in the table */
> -	pos = rte_hash_del_key(enic->fdir.hash, params);
> -	switch (pos) {
> -	case -EINVAL:
> -	case -ENOENT:
> -		enic->fdir.stats.f_remove++;
> -		return -EINVAL;
> -	default:
> -		/* The entry is present in the table */
> -		key = enic->fdir.nodes[pos];
> -
> -		/* Delete the filter */
> -		vnic_dev_classifier(enic->vdev, CLSF_DEL,
> -			&key->fltr_id, NULL, NULL);
> -		rte_free(key);
> -		enic->fdir.nodes[pos] = NULL;
> -		enic->fdir.stats.free++;
> -		enic->fdir.stats.remove++;
> -		break;
> -	}
> -	return 0;
> -}
> -
> -int enic_fdir_add_fltr(struct enic *enic, struct rte_eth_fdir_filter *params)
> -{
> -	struct enic_fdir_node *key;
> -	struct filter_v2 fltr;
> -	int32_t pos;
> -	uint8_t do_free = 0;
> -	uint16_t old_fltr_id = 0;
> -	uint32_t flowtype_supported;
> -	uint16_t flex_bytes;
> -	uint16_t queue;
> -	struct filter_action_v2 action;
> -
> -	memset(&fltr, 0, sizeof(fltr));
> -	memset(&action, 0, sizeof(action));
> -	flowtype_supported = enic->fdir.types_mask
> -			     & (1 << params->input.flow_type);
> -
> -	flex_bytes = ((params->input.flow_ext.flexbytes[1] << 8 & 0xFF00) |
> -		(params->input.flow_ext.flexbytes[0] & 0xFF));
> -
> -	if (!enic->fdir.hash ||
> -		(params->input.flow_ext.vlan_tci & 0xFFF) ||
> -		!flowtype_supported || flex_bytes ||
> -		params->action.behavior /* drop */) {
> -		enic->fdir.stats.f_add++;
> -		return -ENOTSUP;
> -	}
> -
> -	/* Get the enicpmd RQ from the DPDK Rx queue */
> -	queue = enic_rte_rq_idx_to_sop_idx(params->action.rx_queue);
> -
> -	if (!enic->rq[queue].in_use)
> -		return -EINVAL;
> -
> -	/* See if the key is already there in the table */
> -	pos = rte_hash_del_key(enic->fdir.hash, params);
> -	switch (pos) {
> -	case -EINVAL:
> -		enic->fdir.stats.f_add++;
> -		return -EINVAL;
> -	case -ENOENT:
> -		/* Add a new classifier entry */
> -		if (!enic->fdir.stats.free) {
> -			enic->fdir.stats.f_add++;
> -			return -ENOSPC;
> -		}
> -		key = rte_zmalloc("enic_fdir_node",
> -				  sizeof(struct enic_fdir_node), 0);
> -		if (!key) {
> -			enic->fdir.stats.f_add++;
> -			return -ENOMEM;
> -		}
> -		break;
> -	default:
> -		/* The entry is already present in the table.
> -		 * Check if there is a change in queue
> -		 */
> -		key = enic->fdir.nodes[pos];
> -		enic->fdir.nodes[pos] = NULL;
> -		if (unlikely(key->rq_index == queue)) {
> -			/* Nothing to be done */
> -			enic->fdir.stats.f_add++;
> -			pos = rte_hash_add_key(enic->fdir.hash, params);
> -			if (pos < 0) {
> -				dev_err(enic, "Add hash key failed\n");
> -				return pos;
> -			}
> -			enic->fdir.nodes[pos] = key;
> -			dev_warning(enic,
> -				"FDIR rule is already present\n");
> -			return 0;
> -		}
> -
> -		if (likely(enic->fdir.stats.free)) {
> -			/* Add the filter and then delete the old one.
> -			 * This is to avoid packets from going into the
> -			 * default queue during the window between
> -			 * delete and add
> -			 */
> -			do_free = 1;
> -			old_fltr_id = key->fltr_id;
> -		} else {
> -			/* No free slots in the classifier.
> -			 * Delete the filter and add the modified one later
> -			 */
> -			vnic_dev_classifier(enic->vdev, CLSF_DEL,
> -				&key->fltr_id, NULL, NULL);
> -			enic->fdir.stats.free++;
> -		}
> -
> -		break;
> -	}
> -
> -	key->filter = *params;
> -	key->rq_index = queue;
> -
> -	enic->fdir.copy_fltr_fn(&fltr, &params->input,
> -				&enic->rte_dev->data-
> >dev_conf.fdir_conf.mask);
> -	action.type = FILTER_ACTION_RQ_STEERING;
> -	action.rq_idx = queue;
> -
> -	if (!vnic_dev_classifier(enic->vdev, CLSF_ADD, &queue, &fltr,
> -	    &action)) {
> -		key->fltr_id = queue;
> -	} else {
> -		dev_err(enic, "Add classifier entry failed\n");
> -		enic->fdir.stats.f_add++;
> -		rte_free(key);
> -		return -1;
> -	}
> -
> -	if (do_free)
> -		vnic_dev_classifier(enic->vdev, CLSF_DEL, &old_fltr_id, NULL,
> -				    NULL);
> -	else{
> -		enic->fdir.stats.free--;
> -		enic->fdir.stats.add++;
> -	}
> -
> -	pos = rte_hash_add_key(enic->fdir.hash, params);
> -	if (pos < 0) {
> -		enic->fdir.stats.f_add++;
> -		dev_err(enic, "Add hash key failed\n");
> -		return pos;
> -	}
> -
> -	enic->fdir.nodes[pos] = key;
> -	return 0;
> -}
> -
>  void enic_clsf_destroy(struct enic *enic)
>  {
>  	uint32_t index;

Hi,

For enic:

Acked-by: Hyong Youb Kim <hyonkim@cisco.com>

Looks like you mechanically removed the things that caused compilation
errors. That is fine. Once this patch series lands, I will post a
follow-up cleanup patch for enic: remove the entire enic_clsf.c and
remaining traces of fdir in enic.h.

Thanks.
-Hyong