Re: [PATCH v4 2/2] ethdev: get the supported pools for a port

[Olivier MATZ <olivier.matz@6wind.com>]

Hi,

On Mon, Sep 11, 2017 at 08:48:37PM +0530, Santosh Shukla wrote:
> Now that dpdk supports more than one mempool drivers and
> each mempool driver works best for specific PMD, example:
> - sw ring based mempool for Intel PMD drivers.
> - dpaa2 HW mempool manager for dpaa2 PMD driver.
> - fpa HW mempool manager for Octeontx PMD driver.
> 
> Application would like to know the best mempool handle
> for any port.
> 
> Introducing rte_eth_dev_pools_ops_supported() API,
> which allows PMD driver to advertise
> his supported pools capability to the application.
> 
> Supported pools are categorized in below priority:-
> - Best mempool handle for this port (Highest priority '0')
> - Port supports this mempool handle (Priority '1')
> 
> Signed-off-by: Santosh Shukla <santosh.shukla@caviumnetworks.com>
>
> [...]
>
> +int
> +rte_eth_dev_pools_ops_supported(uint8_t port_id, const char *pool)

pools -> pool?

> +{
> +	struct rte_eth_dev *dev;
> +	const char *tmp;
> +
> +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> +
> +	if (pool == NULL)
> +		return -EINVAL;
> +
> +	dev = &rte_eth_devices[port_id];
> +
> +	if (*dev->dev_ops->pools_ops_supported == NULL) {
> +		tmp = rte_eal_mbuf_default_mempool_ops();
> +		if (!strcmp(tmp, pool))
> +			return 0;
> +		else
> +			return -ENOTSUP;

I don't understand why we are comparing with
rte_eal_mbuf_default_mempool_ops().

It means that the result of the function would be influenced
by the parameter given by the user.

I think that a PMD that does not implement ->pools_ops_supported
should always return 1 (mempool is supported).


> +	}
> +
> +	return (*dev->dev_ops->pools_ops_supported)(dev, pool);
> +}
> diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
> index 0adf3274a..d90029b1e 100644
> --- a/lib/librte_ether/rte_ethdev.h
> +++ b/lib/librte_ether/rte_ethdev.h
> @@ -1425,6 +1425,10 @@ typedef int (*eth_get_dcb_info)(struct rte_eth_dev *dev,
>  				 struct rte_eth_dcb_info *dcb_info);
>  /**< @internal Get dcb information on an Ethernet device */
>  
> +typedef int (*eth_pools_ops_supported_t)(struct rte_eth_dev *dev,
> +						const char *pool);
> +/**< @internal Get the supported pools for a port */
> +

The comment should be something like:
Test if a port supports specific mempool ops.

>  /**
>   * @internal A structure containing the functions exported by an Ethernet driver.
>   */
> @@ -1544,6 +1548,8 @@ struct eth_dev_ops {
>  
>  	eth_tm_ops_get_t tm_ops_get;
>  	/**< Get Traffic Management (TM) operations. */
> +	eth_pools_ops_supported_t pools_ops_supported;
> +	/**< Get the supported pools for a port */

Same

>  };
>  
>  /**
> @@ -4436,6 +4442,24 @@ int rte_eth_dev_adjust_nb_rx_tx_desc(uint8_t port_id,
>  				     uint16_t *nb_rx_desc,
>  				     uint16_t *nb_tx_desc);
>  
> +
> +/**
> + * Get the supported pools for a port

Same

> + *
> + * @param port_id
> + *   Port identifier of the Ethernet device.
> + * @param [in] pool
> + *   The supported pool handle for this port.

The name of the pool operations to test

> + *   Maximum length of pool handle name is RTE_MEMPOOL_OPS_NAMESIZE.

I don't think we should keep this



Thanks,
Olivier