[dpdk-dev] [PATCH 8/8] net/bnxt: add support for flow filter ops

Ferruh Yigit ferruh.yigit at intel.com
Tue Aug 29 18:16:14 CEST 2017


On 8/24/2017 5:29 PM, Ajit Khaparde wrote:
> This patch adds support for flow validate/create/destroy/flush ops.

Can you please update feature file [1] to document flow API support and
for supported filters.

Also I believe this worth mentioning in release notes [2].

[1]
doc/guides/nics/features/bnxt.ini

[2]
doc/guides/rel_notes/release_17_11.rst

> Signed-off-by: Ajit Khaparde <ajit.khaparde at broadcom.com>

<...>

> --- a/drivers/net/bnxt/bnxt_ethdev.c
> +++ b/drivers/net/bnxt/bnxt_ethdev.c
> @@ -610,7 +610,7 @@ static void bnxt_mac_addr_remove_op(struct rte_eth_dev *eth_dev,
>  				if (filter->mac_index == index) {
>  					STAILQ_REMOVE(&vnic->filter, filter,
>  						      bnxt_filter_info, next);
> -					bnxt_hwrm_clear_filter(bp, filter);
> +					bnxt_hwrm_clear_l2_filter(bp, filter);

Is it possible to extract these function name changes into different patch?

<...>

> +		default:
> +			RTE_LOG(ERR, PMD, "Unknown Flow type");
> +			use_ntuple |= 1;
> +			//return -1;

Please avoid dead code. "break" ?

> +		}
> +		item++;
> +	}
> +	return use_ntuple;
> +}
> +

<...>

> +		switch (item->type) {
> +		case RTE_FLOW_ITEM_TYPE_ETH:
> +			//filter_TYPE = EM

can be removed.

<...>

> +			} /*
> +			   * else {
> +			   *  RTE_LOG(ERR, PMD, "Handle this condition\n");
> +			   * }
> +			   */

Please remove unsued code.

<...>

> +	rc = bnxt_flow_parse_attr(attr, error);
> +	if (rc != 0)
> +		goto ret;
> +	//Since we support ingress attribute only - right now.

Please prefer /* */ comments.

<...>

> +			filter->flags |=
> +				HWRM_CFA_NTUPLE_FILTER_ALLOC_INPUT_FLAGS_DROP;
> +		//HWRM_CFA_L2_FILTER_CFG_INPUT_FLAGS_DROP;
> +		//HWRM_CFA_L2_FILTER_ALLOC_INPUT_FLAGS_DROP;
> +		//HWRM_CFA_NTUPLE_FILTER_ALLOC_INPUT_FLAGS_DROP
> +		//HWRM_CFA_EM_FLOW_ALLOC_INPUT_FLAGS_DROP
> +		//HWRM_CFA_FLOW_ALLOC_INPUT_ACTION_FLAGS_DROP

These can go.

<...>

> +free_flow:
> +	RTE_LOG(ERR, PMD, "Failed to create flow.\n");
> +	rte_flow_error_set(error, -ret,
> +			   RTE_FLOW_ERROR_TYPE_HANDLE, NULL,
> +			   "Failed to create flow.");
> +	rte_free(flow);
> +	flow = NULL;
> +	return flow;

return NULL; ?

> +static int
> +bnxt_flow_destroy(struct rte_eth_dev *dev,
> +		  struct rte_flow *flow,
> +		  struct rte_flow_error *error)
> +{
> +	struct bnxt *bp = (struct bnxt *)dev->data->dev_private;
> +	struct bnxt_filter_info *filter = flow->filter;
> +	int ret = 0;
> +
> +	if (filter->filter_type == HWRM_CFA_EM_FILTER)
> +		ret = bnxt_hwrm_clear_em_filter(bp, filter);
> +	if (filter->filter_type == HWRM_CFA_NTUPLE_FILTER)
> +		ret = bnxt_hwrm_clear_ntuple_filter(bp, filter);
> +
> +	if (!ret) {
> +		TAILQ_REMOVE(&bp->flow_list, flow, node);
> +		rte_free(flow);
> +	} else {
> +		rte_flow_error_set(error, -ret,
> +				   RTE_FLOW_ERROR_TYPE_HANDLE, NULL,
> +				   "Failed to destroy flow.");
> +	}
> +
> +	return ret;
> +}
> +
> +static int
> +bnxt_flow_flush(struct rte_eth_dev *dev, struct rte_flow_error *error)
> +{
> +	struct bnxt *bp = (struct bnxt *)dev->data->dev_private;
> +	struct rte_flow *flow;
> +	int ret = 0;
> +	void *temp;
> +
> +	TAILQ_FOREACH_SAFE(flow, &bp->flow_list, node, temp) {
> +		struct bnxt_filter_info *filter = flow->filter;
> +
> +		if (filter->filter_type == HWRM_CFA_EM_FILTER)
> +			ret = bnxt_hwrm_clear_em_filter(bp, filter);
> +		if (filter->filter_type == HWRM_CFA_NTUPLE_FILTER)
> +			ret = bnxt_hwrm_clear_ntuple_filter(bp, filter);
> +
> +		if (ret) {
> +			rte_flow_error_set(error, -ret,
> +					   RTE_FLOW_ERROR_TYPE_HANDLE, NULL,
> +					   "Failed to flush flow in HW.");
> +			return -rte_errno;
> +		}
> +
> +		TAILQ_REMOVE(&bp->flow_list, flow, node);
> +		rte_free(flow);

This part looks like duplication of bnxt_flow_destroy()

> +	}
> +
> +	return ret;
> +}

<...>

> diff --git a/drivers/net/bnxt/bnxt_rxq.c b/drivers/net/bnxt/bnxt_rxq.c
> index 0793820..da53e99 100644
> --- a/drivers/net/bnxt/bnxt_rxq.c
> +++ b/drivers/net/bnxt/bnxt_rxq.c
> @@ -98,7 +98,7 @@ int bnxt_mq_rx_configure(struct bnxt *bp)
>  	}
>  
>  	/* Multi-queue mode */
> -	if (dev_conf->rxmode.mq_mode & ETH_MQ_RX_VMDQ_FLAG) {
> +	if (dev_conf->rxmode.mq_mode & ETH_MQ_RX_VMDQ_DCB_RSS) {

Is this change related?

>  		/* VMDq ONLY, VMDq+RSS, VMDq+DCB, VMDq+DCB+RSS */
>  		enum rte_eth_nb_pools pools;
>  
> @@ -113,6 +113,9 @@ int bnxt_mq_rx_configure(struct bnxt *bp)
>  				pools = conf->nb_queue_pools;
>  				break;
>  			}
> +		case ETH_MQ_RX_RSS:
> +			pools = 1;	//bp->rx_cp_nr_rings;
> +			break;
>  		default:
>  			RTE_LOG(ERR, PMD, "Unsupported mq_mod %d\n",
>  				dev_conf->rxmode.mq_mode);
> @@ -203,12 +206,42 @@ int bnxt_mq_rx_configure(struct bnxt *bp)
>  	}
>  	STAILQ_INSERT_TAIL(&vnic->filter, filter, next);
>  
> -	if (dev_conf->rxmode.mq_mode & ETH_MQ_RX_RSS_FLAG)
> -		vnic->hash_type =
> -			HWRM_VNIC_RSS_CFG_INPUT_HASH_TYPE_IPV4 |
> -			HWRM_VNIC_RSS_CFG_INPUT_HASH_TYPE_IPV6;
> -
>  out:
> +	if (dev_conf->rxmode.mq_mode & ETH_MQ_RX_RSS_FLAG) {
> +		struct rte_eth_rss_conf *rss = &dev_conf->rx_adv_conf.rss_conf;
> +		uint16_t hash_type = 0;
> +
> +		if (rss->rss_hf & ETH_RSS_IPV4)
> +			hash_type |= HWRM_VNIC_RSS_CFG_INPUT_HASH_TYPE_IPV4;
> +		if (rss->rss_hf & ETH_RSS_NONFRAG_IPV4_TCP)
> +			hash_type |= HWRM_VNIC_RSS_CFG_INPUT_HASH_TYPE_TCP_IPV4;
> +		if (rss->rss_hf & ETH_RSS_NONFRAG_IPV4_UDP)
> +			hash_type |= HWRM_VNIC_RSS_CFG_INPUT_HASH_TYPE_UDP_IPV4;
> +		if (rss->rss_hf & ETH_RSS_IPV6)
> +			hash_type |= HWRM_VNIC_RSS_CFG_INPUT_HASH_TYPE_IPV6;
> +		if (rss->rss_hf & ETH_RSS_NONFRAG_IPV6_TCP)
> +			hash_type |= HWRM_VNIC_RSS_CFG_INPUT_HASH_TYPE_TCP_IPV6;
> +		if (rss->rss_hf & ETH_RSS_NONFRAG_IPV6_UDP)
> +			hash_type |= HWRM_VNIC_RSS_CFG_INPUT_HASH_TYPE_UDP_IPV6;
> +
> +		for (i = 0; i < bp->nr_vnics; i++) {
> +			STAILQ_FOREACH(vnic, &bp->ff_pool[i], next) {
> +			vnic->hash_type |= hash_type;
> +
> +			/*
> +			 * Use the supplied key if the key length is
> +			 * acceptable and the rss_key is not NULL
> +			 */
> +			if (rss->rss_key &&
> +			    rss->rss_key_len <= HW_HASH_KEY_SIZE)
> +				memcpy(vnic->rss_hash_key,
> +				       rss->rss_key, rss->rss_key_len);
> +			}
> +		}
> +		RTE_LOG(ERR, PMD,
> +			"VNIC rss hash key_len %d\n", rss->rss_key_len);
> +	}
> +

Same for above, these looks like logically unrelated, can be separated
into another patch?

<...>


More information about the dev mailing list