[dpdk-dev,v4,2/2] ethdev: get the supported pools for a port

  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>
---
v3 --> v4:
- Replaced __preferred_pool() with
  rte_eth_dev_pools_ops_supported() (suggested by Olivier)

History, Refer [1].
[1] http://dpdk.org/dev/patchwork/patch/27610/

 lib/librte_ether/rte_ethdev.c           | 24 ++++++++++++++++++++++++
 lib/librte_ether/rte_ethdev.h           | 24 ++++++++++++++++++++++++
 lib/librte_ether/rte_ethdev_version.map |  7 +++++++
 3 files changed, 55 insertions(+)
  

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
  

Hi Olivier,


On Monday 25 September 2017 08:37 AM, Olivier MATZ wrote:
> 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?

ok.

>> +{
>> +	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.

But that will be only for ops not supported case and in that case,
function _must_ make sure that if inputted param is _default_ops_name
then function should return ops supported correct info (whether 
returning '0' : Best ops or '1': ops does support 
, this part is arguable.. meaning One can say that default_ops ='handle-name' is best possible handle Or 
one of handle which platform supports).

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

Return 1 says: PMD support this ops.. 

So if ops is not supported and func returns with 1, then which ops application will use?
If that ops is default_ops.. then How application will distinguish when to use default ops or
param ops?.. as because in both cases func will return with value 1.

The approach in the patch takes care of that condition and func will return -ENOTSUP 
if (ops not support || inputted param not matching with default ops) otherwise will return
0 or 1.

At application side; 
For error case: In case of -ENOTSUP, its upto application to use _default_ops or exit.
For good case: 0 or 1 case, func gaurantee that handle is either best handle for pool or pool supports
that handle.. However in your suggestion if ops not supported case returns 1 then application is not 
sure which ops to use.. default_ops Or input ops given to func.

make sense? 

>> +	}
>> +
>> +	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

ok, v5.

>> + *   Maximum length of pool handle name is RTE_MEMPOOL_OPS_NAMESIZE.
> I don't think we should keep this
>
Ok

>
> Thanks,
> Olivier
  

Hi Olivier,


On Monday 25 September 2017 10:52 PM, santosh wrote:
> Hi Olivier,
>
>
> On Monday 25 September 2017 08:37 AM, Olivier MATZ wrote:
>>> +{
>>> +	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.
> But that will be only for ops not supported case and in that case,
> function _must_ make sure that if inputted param is _default_ops_name
> then function should return ops supported correct info (whether 
> returning '0' : Best ops or '1': ops does support 
> , this part is arguable.. meaning One can say that default_ops ='handle-name' is best possible handle Or 
> one of handle which platform supports).
>
>> I think that a PMD that does not implement ->pools_ops_supported
>> should always return 1 (mempool is supported).
> Return 1 says: PMD support this ops.. 
>
> So if ops is not supported and func returns with 1, then which ops application will use?
> If that ops is default_ops.. then How application will distinguish when to use default ops or
> param ops?.. as because in both cases func will return with value 1.
>
> The approach in the patch takes care of that condition and func will return -ENOTSUP 
> if (ops not support || inputted param not matching with default ops) otherwise will return
> 0 or 1.
>
> At application side; 
> For error case: In case of -ENOTSUP, its upto application to use _default_ops or exit.
> For good case: 0 or 1 case, func gaurantee that handle is either best handle for pool or pool supports
> that handle.. However in your suggestion if ops not supported case returns 1 then application is not 
> sure which ops to use.. default_ops Or input ops given to func.
>
> make sense? 

Ping? Are you fine with said explanation if not then would you
share your thought in case if ops not supported returns simply
1 if then how application differentiate whether to use
default_ops Or inputted param ops.. as return value 1 holds true
for both cases, so how application will pick and choose which ops
to use?

Can we pl. close on this? need to send v5 for -rc1 release.

Thanks.
  

On Mon, Sep 25, 2017 at 10:52:31PM +0100, santosh wrote:
> Hi Olivier,
> 
> 
> On Monday 25 September 2017 08:37 AM, Olivier MATZ wrote:
> > 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?
> 
> ok.
> 
> >> +{
> >> +	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.
> 
> But that will be only for ops not supported case and in that case,
> function _must_ make sure that if inputted param is _default_ops_name
> then function should return ops supported correct info (whether 
> returning '0' : Best ops or '1': ops does support 
> , this part is arguable.. meaning One can say that default_ops ='handle-name' is best possible handle Or 
> one of handle which platform supports).
> 
> > I think that a PMD that does not implement ->pools_ops_supported
> > should always return 1 (mempool is supported).
> 

I don't agree.
The result of this API (mempool ops supported or not by a PMD)
should not depend on what user asks for.

> Return 1 says: PMD support this ops.. 
> 
> So if ops is not supported and func returns with 1, then which ops application will use?
> If that ops is default_ops.. then How application will distinguish when to use default ops or
> param ops?.. as because in both cases func will return with value 1.
> 
> The approach in the patch takes care of that condition and func will return -ENOTSUP 
> if (ops not support || inputted param not matching with default ops) otherwise will return
> 0 or 1.
> 
> At application side; 
> For error case: In case of -ENOTSUP, its upto application to use _default_ops or exit.
> For good case: 0 or 1 case, func gaurantee that handle is either best handle for pool or pool supports
> that handle.. However in your suggestion if ops not supported case returns 1 then application is not 
> sure which ops to use.. default_ops Or input ops given to func.
> 
> make sense? 

My proposition is:

- what a PMD returns does not depend on used parameter:
  - 0: best support
  - 1: support
  - -ENOTSUP: not supported

- if a PMD does not implement the _ops_supported() API, assume all pools
  are supported (returns 1)

- if the user does not pass a specific mempool ops, the application asks
  the PMDs, finds the best mempool ops, and use it. This could even be
  done in rte_pktmbuf_pool_create() as discussed at the summit.

- if the user passes a specific mempool ops, we don't need to call the
  _ops_supported() api, we just try to use this pool.


The _ops_supported() returns a property of a PMD, in my opinion it
should not be impacted by a user argument.
  

Hi Olivier,


On Friday 29 September 2017 09:32 AM, Olivier MATZ wrote:
> On Mon, Sep 25, 2017 at 10:52:31PM +0100, santosh wrote:
>> Hi Olivier,
>>
>>
>> On Monday 25 September 2017 08:37 AM, Olivier MATZ wrote:
>>
>>>> +{
>>>> +	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.
>> But that will be only for ops not supported case and in that case,
>> function _must_ make sure that if inputted param is _default_ops_name
>> then function should return ops supported correct info (whether 
>> returning '0' : Best ops or '1': ops does support 
>> , this part is arguable.. meaning One can say that default_ops ='handle-name' is best possible handle Or 
>> one of handle which platform supports).
>>
>>> I think that a PMD that does not implement ->pools_ops_supported
>>> should always return 1 (mempool is supported).
> I don't agree.
> The result of this API (mempool ops supported or not by a PMD)
> should not depend on what user asks for.
>
>> Return 1 says: PMD support this ops.. 
>>
>> So if ops is not supported and func returns with 1, then which ops application will use?
>> If that ops is default_ops.. then How application will distinguish when to use default ops or
>> param ops?.. as because in both cases func will return with value 1.
>>
>> The approach in the patch takes care of that condition and func will return -ENOTSUP 
>> if (ops not support || inputted param not matching with default ops) otherwise will return
>> 0 or 1.
>>
>> At application side; 
>> For error case: In case of -ENOTSUP, its upto application to use _default_ops or exit.
>> For good case: 0 or 1 case, func gaurantee that handle is either best handle for pool or pool supports
>> that handle.. However in your suggestion if ops not supported case returns 1 then application is not 
>> sure which ops to use.. default_ops Or input ops given to func.
>>
>> make sense? 
> My proposition is:
>
> - what a PMD returns does not depend on used parameter:
>   - 0: best support
>   - 1: support
>   - -ENOTSUP: not supported
>
> - if a PMD does not implement the _ops_supported() API, assume all pools
>   are supported (returns 1)

If API returns 1 for PMD does not implement _ops_supported() case,
then do we really need -ENOTSUP?

If so then in which case API will return -ENOTSUP error, wondering.

> - if the user does not pass a specific mempool ops, the application asks
>   the PMDs, finds the best mempool ops, and use it. This could even be
>   done in rte_pktmbuf_pool_create() as discussed at the summit.

Right.

> - if the user passes a specific mempool ops, we don't need to call the
>   _ops_supported() api, we just try to use this pool.
>
>
> The _ops_supported() returns a property of a PMD, in my opinion it
> should not be impacted by a user argument.
>
Thanks.
  

On Friday 29 September 2017 11:16 AM, santosh wrote:
> Hi Olivier,
>
>
> On Friday 29 September 2017 09:32 AM, Olivier MATZ wrote:
>> On Mon, Sep 25, 2017 at 10:52:31PM +0100, santosh wrote:
>>> Hi Olivier,
>>>
>>>
>>> On Monday 25 September 2017 08:37 AM, Olivier MATZ wrote:
>>>
>>>>> +{
>>>>> +	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.
>>> But that will be only for ops not supported case and in that case,
>>> function _must_ make sure that if inputted param is _default_ops_name
>>> then function should return ops supported correct info (whether 
>>> returning '0' : Best ops or '1': ops does support 
>>> , this part is arguable.. meaning One can say that default_ops ='handle-name' is best possible handle Or 
>>> one of handle which platform supports).
>>>
>>>> I think that a PMD that does not implement ->pools_ops_supported
>>>> should always return 1 (mempool is supported).
>> I don't agree.
>> The result of this API (mempool ops supported or not by a PMD)
>> should not depend on what user asks for.
>>
>>> Return 1 says: PMD support this ops.. 
>>>
>>> So if ops is not supported and func returns with 1, then which ops application will use?
>>> If that ops is default_ops.. then How application will distinguish when to use default ops or
>>> param ops?.. as because in both cases func will return with value 1.
>>>
>>> The approach in the patch takes care of that condition and func will return -ENOTSUP 
>>> if (ops not support || inputted param not matching with default ops) otherwise will return
>>> 0 or 1.
>>>
>>> At application side; 
>>> For error case: In case of -ENOTSUP, its upto application to use _default_ops or exit.
>>> For good case: 0 or 1 case, func gaurantee that handle is either best handle for pool or pool supports
>>> that handle.. However in your suggestion if ops not supported case returns 1 then application is not 
>>> sure which ops to use.. default_ops Or input ops given to func.
>>>
>>> make sense? 
>> My proposition is:
>>
>> - what a PMD returns does not depend on used parameter:
>>   - 0: best support
>>   - 1: support
>>   - -ENOTSUP: not supported
>>
>> - if a PMD does not implement the _ops_supported() API, assume all pools
>>   are supported (returns 1)
> If API returns 1 for PMD does not implement _ops_supported() case,
> then do we really need -ENOTSUP?
>
> If so then in which case API will return -ENOTSUP error, wondering.

To summarize return value of API (specially for error case):
- 0: best support
- 1: pool support ops or _that_ PMD did not implement _ops_supported().
- -ENODEV: Invalid port id.
- -EINVAL: pool is null. 

So to me, We don't need -ENOTSUP error code as return value 1 per your input, 
address both case ie.. (Pool does support this ops Or _that_ PMD does not implement _ops_supported()).

is above API return value and their case explanation make sense to you?

If not then could you pl. be more explicit on your error code case detailing and 
suggest more about return value '1' case? 

As per me: Return value 1 is wearing two hats,
addressing two cases. Correct me if I misunderstood your proposition?

Thanks.
  

On Fri, Sep 29, 2017 at 11:16:20AM +0100, santosh wrote:
> Hi Olivier,
> 
> 
> On Friday 29 September 2017 09:32 AM, Olivier MATZ wrote:
> > On Mon, Sep 25, 2017 at 10:52:31PM +0100, santosh wrote:
> >> Hi Olivier,
> >>
> >>
> >> On Monday 25 September 2017 08:37 AM, Olivier MATZ wrote:
> >>
> >>>> +{
> >>>> +	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.
> >> But that will be only for ops not supported case and in that case,
> >> function _must_ make sure that if inputted param is _default_ops_name
> >> then function should return ops supported correct info (whether 
> >> returning '0' : Best ops or '1': ops does support 
> >> , this part is arguable.. meaning One can say that default_ops ='handle-name' is best possible handle Or 
> >> one of handle which platform supports).
> >>
> >>> I think that a PMD that does not implement ->pools_ops_supported
> >>> should always return 1 (mempool is supported).
> > I don't agree.
> > The result of this API (mempool ops supported or not by a PMD)
> > should not depend on what user asks for.
> >
> >> Return 1 says: PMD support this ops.. 
> >>
> >> So if ops is not supported and func returns with 1, then which ops application will use?
> >> If that ops is default_ops.. then How application will distinguish when to use default ops or
> >> param ops?.. as because in both cases func will return with value 1.
> >>
> >> The approach in the patch takes care of that condition and func will return -ENOTSUP 
> >> if (ops not support || inputted param not matching with default ops) otherwise will return
> >> 0 or 1.
> >>
> >> At application side; 
> >> For error case: In case of -ENOTSUP, its upto application to use _default_ops or exit.
> >> For good case: 0 or 1 case, func gaurantee that handle is either best handle for pool or pool supports
> >> that handle.. However in your suggestion if ops not supported case returns 1 then application is not 
> >> sure which ops to use.. default_ops Or input ops given to func.
> >>
> >> make sense? 
> > My proposition is:
> >
> > - what a PMD returns does not depend on used parameter:
> >   - 0: best support
> >   - 1: support
> >   - -ENOTSUP: not supported
> >
> > - if a PMD does not implement the _ops_supported() API, assume all pools
> >   are supported (returns 1)
> 
> If API returns 1 for PMD does not implement _ops_supported() case,
> then do we really need -ENOTSUP?

When the PMD explicitly does not support a mempool ops.

Not implementing the API means, like today: "I don't care what
the mempool ops is, so I a priori support all the ones available,
without preference".
  

On Friday 29 September 2017 12:23 PM, Olivier MATZ wrote:
> On Fri, Sep 29, 2017 at 11:16:20AM +0100, santosh wrote:
>> Hi Olivier,
>>
>>
>> On Friday 29 September 2017 09:32 AM, Olivier MATZ wrote:
>>> On Mon, Sep 25, 2017 at 10:52:31PM +0100, santosh wrote:
>>>> Hi Olivier,
>>>>
>>>>
>>>> On Monday 25 September 2017 08:37 AM, Olivier MATZ wrote:
>>>>
>>>>>> +{
>>>>>> +	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.
>>>> But that will be only for ops not supported case and in that case,
>>>> function _must_ make sure that if inputted param is _default_ops_name
>>>> then function should return ops supported correct info (whether 
>>>> returning '0' : Best ops or '1': ops does support 
>>>> , this part is arguable.. meaning One can say that default_ops ='handle-name' is best possible handle Or 
>>>> one of handle which platform supports).
>>>>
>>>>> I think that a PMD that does not implement ->pools_ops_supported
>>>>> should always return 1 (mempool is supported).
>>> I don't agree.
>>> The result of this API (mempool ops supported or not by a PMD)
>>> should not depend on what user asks for.
>>>
>>>> Return 1 says: PMD support this ops.. 
>>>>
>>>> So if ops is not supported and func returns with 1, then which ops application will use?
>>>> If that ops is default_ops.. then How application will distinguish when to use default ops or
>>>> param ops?.. as because in both cases func will return with value 1.
>>>>
>>>> The approach in the patch takes care of that condition and func will return -ENOTSUP 
>>>> if (ops not support || inputted param not matching with default ops) otherwise will return
>>>> 0 or 1.
>>>>
>>>> At application side; 
>>>> For error case: In case of -ENOTSUP, its upto application to use _default_ops or exit.
>>>> For good case: 0 or 1 case, func gaurantee that handle is either best handle for pool or pool supports
>>>> that handle.. However in your suggestion if ops not supported case returns 1 then application is not 
>>>> sure which ops to use.. default_ops Or input ops given to func.
>>>>
>>>> make sense? 
>>> My proposition is:
>>>
>>> - what a PMD returns does not depend on used parameter:
>>>   - 0: best support
>>>   - 1: support
>>>   - -ENOTSUP: not supported
>>>
>>> - if a PMD does not implement the _ops_supported() API, assume all pools
>>>   are supported (returns 1)
>> If API returns 1 for PMD does not implement _ops_supported() case,
>> then do we really need -ENOTSUP?
> When the PMD explicitly does not support a mempool ops.
>
> Not implementing the API means, like today: "I don't care what
> the mempool ops is, so I a priori support all the ones available,
> without preference".
>
More confusing to me.

If PMD does not implement mempool_ops then
if (_ops_supported() == NULL)
	return 1; /// per your proposition.

so I don't see why we need -ENOTSUP error code in API?