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

[santosh]

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.