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

[Andrew Rybchenko]

On 10/23/20 6:32 AM, Hyong Youb Kim (hyonkim) wrote:
>> -----Original Message-----
>> From: Andrew Rybchenko <arybchenko at solarflare.com>
>> Sent: Thursday, October 22, 2020 6:43 PM
>> To: Wenzhuo Lu <wenzhuo.lu at intel.com>; Beilei Xing
>> <beilei.xing at intel.com>; Bernard Iremonger
>> <bernard.iremonger at intel.com>; Jeff Guo <jia.guo at intel.com>; Ray Kinsella
>> <mdr at ashroe.eu>; Neil Horman <nhorman at tuxdriver.com>; Ajit Khaparde
>> <ajit.khaparde at broadcom.com>; Somnath Kotur
>> <somnath.kotur at broadcom.com>; John Daley (johndale)
>> <johndale at cisco.com>; Hyong Youb Kim (hyonkim) <hyonkim at cisco.com>;
>> Haiyue Wang <haiyue.wang at intel.com>; Matan Azrad <matan at nvidia.com>;
>> Shahaf Shuler <shahafs at nvidia.com>; Viacheslav Ovsiienko
>> <viacheslavo at nvidia.com>; Rasesh Mody <rmody at marvell.com>; Shahed
>> Shaikh <shshaikh at marvell.com>; Andrew Rybchenko
>> <andrew.rybchenko at oktetlabs.ru>; Thomas Monjalon
>> <thomas at monjalon.net>; Ferruh Yigit <ferruh.yigit at intel.com>
>> Cc: dev at 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 at solarflare.com>
>> Acked-by: Ajit Khaparde <ajit.khaparde at broadcom.com>
>> Acked-by: Haiyue Wang <haiyue.wang at 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 at 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.

I've tried to trace back and in some cases remove variables and
structure fields used for FDIR only, but it was not always easy
and I've not dig too deep. That's true. It is much easier for
driver maintainers. Thanks.