Re: [dpdk-dev] [PATCH 1/2] ethdev: replace callback getting filter operations

[Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>]

On 3/12/21 1:17 AM, Thomas Monjalon wrote:
> Since rte_flow is the only API for filtering operations,
> the legacy driver interface filter_ctrl was too much complicated
> for the simple task of getting the struct rte_flow_ops.
> 
> The filter type RTE_ETH_FILTER_GENERIC and
> the filter operarion RTE_ETH_FILTER_GET are removed.
> The new driver callback flow_ops_get replaces filter_ctrl.
> 
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>

A couple of minor notes below. Other than that:

Reviewed-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>

[snip]

> diff --git a/doc/guides/rel_notes/release_20_11.rst b/doc/guides/rel_notes/release_20_11.rst
> index 7405a9864f..1260539a21 100644
> --- a/doc/guides/rel_notes/release_20_11.rst
> +++ b/doc/guides/rel_notes/release_20_11.rst
> @@ -571,7 +571,7 @@ API Changes
>    a TC is greater than 256.
>  
>  * ethdev: Removed the legacy filter API, including
> -  ``rte_eth_dev_filter_supported()`` and ``rte_eth_dev_filter_ctrl()``.
> +  ``rte_eth_dev_filter_supported()`` and ``rte_eth_dev_flow_ops_get()``.

Mass substitution is dangerous. I guess this one is not
intended.

>  
>  * ethdev: Removed the legacy L2 tunnel configuration API, including
>    ``rte_eth_dev_l2_tunnel_eth_type_conf()`` and
> diff --git a/doc/guides/rel_notes/release_2_2.rst b/doc/guides/rel_notes/release_2_2.rst
> index cea5c8746d..f7deeac34b 100644
> --- a/doc/guides/rel_notes/release_2_2.rst
> +++ b/doc/guides/rel_notes/release_2_2.rst
> @@ -501,7 +501,7 @@ API Changes
>  -----------
>  
>  * The deprecated flow director API is removed.
> -  It was replaced by ``rte_eth_dev_filter_ctrl()``.
> +  It was replaced by ``rte_eth_dev_flow_ops_get()``.

As well as this one.

>  
>  * The ``dcb_queue`` is renamed to ``dcb_tc`` in following dcb configuration
>    structures: ``rte_eth_dcb_rx_conf``, ``rte_eth_dcb_tx_conf``,

[snip]

> diff --git a/lib/librte_ethdev/ethdev_driver.h b/lib/librte_ethdev/ethdev_driver.h
> index 57fdedaa1a..1c6592ec23 100644
> --- a/lib/librte_ethdev/ethdev_driver.h
> +++ b/lib/librte_ethdev/ethdev_driver.h
> @@ -465,34 +465,10 @@ typedef int (*eth_get_module_eeprom_t)(struct rte_eth_dev *dev,
>  				       struct rte_dev_eeprom_info *info);
>  /**< @internal Retrieve plugin module eeprom data */
>  
> -/**
> - * Feature filter types
> - */
> -enum rte_filter_type {
> -	RTE_ETH_FILTER_NONE = 0,
> -	RTE_ETH_FILTER_ETHERTYPE,
> -	RTE_ETH_FILTER_FLEXIBLE,
> -	RTE_ETH_FILTER_SYN,
> -	RTE_ETH_FILTER_NTUPLE,
> -	RTE_ETH_FILTER_TUNNEL,
> -	RTE_ETH_FILTER_FDIR,
> -	RTE_ETH_FILTER_HASH,
> -	RTE_ETH_FILTER_L2_TUNNEL,
> -	RTE_ETH_FILTER_GENERIC,
> -};
> -
> -/**
> - * Generic operations on filters
> - */
> -enum rte_filter_op {
> -	RTE_ETH_FILTER_GET,      /**< get flow API ops */
> -};
> -
> -typedef int (*eth_filter_ctrl_t)(struct rte_eth_dev *dev,
> -				 enum rte_filter_type filter_type,
> -				 enum rte_filter_op filter_op,
> -				 void *arg);
> -/**< @internal Take operations to assigned filter type on an Ethernet device */
> +struct rte_flow_ops;
> +typedef int (*eth_flow_ops_get_t)(struct rte_eth_dev *dev,
> +				  const struct rte_flow_ops **ops);
> +/**< @internal Get flow operations */

I'm OK with the callback prototype. I think there is no point
to optimize it and make ops a return value. It is called in
just one place. Nothing to optimize.

>  
>  typedef int (*eth_tm_ops_get_t)(struct rte_eth_dev *dev, void *ops);
>  /**< @internal Get Traffic Management (TM) operations on an Ethernet device */
> @@ -880,7 +856,7 @@ struct eth_dev_ops {
>  	eth_get_module_eeprom_t    get_module_eeprom;
>  	/** Get plugin module eeprom data. */
>  
> -	eth_filter_ctrl_t          filter_ctrl; /**< common filter control. */
> +	eth_flow_ops_get_t         flow_ops_get; /**< Get flow operations. */
>  
>  	eth_get_dcb_info           get_dcb_info; /** Get DCB information. */
>  
> @@ -1377,6 +1353,18 @@ rte_eth_hairpin_queue_peer_unbind(uint16_t cur_port, uint16_t cur_queue,
>   * Legacy ethdev API used internally by drivers.
>   */
>  
> +enum rte_filter_type {
> +	RTE_ETH_FILTER_NONE = 0,
> +	RTE_ETH_FILTER_ETHERTYPE,
> +	RTE_ETH_FILTER_FLEXIBLE,
> +	RTE_ETH_FILTER_SYN,
> +	RTE_ETH_FILTER_NTUPLE,
> +	RTE_ETH_FILTER_TUNNEL,
> +	RTE_ETH_FILTER_FDIR,
> +	RTE_ETH_FILTER_HASH,
> +	RTE_ETH_FILTER_L2_TUNNEL,
> +};
> +
>  /**
>   * Define all structures for Ethertype Filter type.
>   */
> diff --git a/lib/librte_ethdev/rte_eth_ctrl.h b/lib/librte_ethdev/rte_eth_ctrl.h
> index 8a50dbfef9..42652f9cce 100644
> --- a/lib/librte_ethdev/rte_eth_ctrl.h
> +++ b/lib/librte_ethdev/rte_eth_ctrl.h
> @@ -339,7 +339,7 @@ struct rte_eth_fdir_action {
>  };
>  
>  /**
> - * A structure used to define the flow director filter entry by filter_ctrl API.
> + * A structure used to define the flow director filter entry.
>   */
>  struct rte_eth_fdir_filter {
>  	uint32_t soft_id;
> diff --git a/lib/librte_ethdev/rte_flow.c b/lib/librte_ethdev/rte_flow.c
> index 241af6c4ca..ed9f826b85 100644
> --- a/lib/librte_ethdev/rte_flow.c
> +++ b/lib/librte_ethdev/rte_flow.c
> @@ -255,12 +255,9 @@ rte_flow_ops_get(uint16_t port_id, struct rte_flow_error *error)
>  
>  	if (unlikely(!rte_eth_dev_is_valid_port(port_id)))
>  		code = ENODEV;
> -	else if (unlikely(!dev->dev_ops->filter_ctrl ||
> -			  dev->dev_ops->filter_ctrl(dev,
> -						    RTE_ETH_FILTER_GENERIC,
> -						    RTE_ETH_FILTER_GET,
> -						    &ops) ||
> -			  !ops))
> +	else if (unlikely(!dev->dev_ops->flow_ops_get ||
> +			  dev->dev_ops->flow_ops_get(dev, &ops) ||

If the callback return error code, may be it should be used
and forwarded here?

> +			  ops == NULL))
>  		code = ENOSYS;
>  	else
>  		return ops;

[snip]