Re: [dpdk-dev] [PATCH] ethdev: fine tune error reporting in pick transfer proxy API

[Ivan Malov <Ivan.Malov@oktetlabs.ru>]

Good day to you, Thomas.

On 27/10/2021 13:57, Thomas Monjalon wrote:
> 27/10/2021 11:55, Ivan Malov:
>> Hi Thomas,
>>
>> On 27/10/2021 12:46, Thomas Monjalon wrote:
>>> 27/10/2021 11:00, Ivan Malov:
>>>> There are PMDs which do not support flow offloads at all.
>>>> In such cases, the API in question returns ENOTSUP. This
>>>> is too loud. Restructure the code to avoid spamming logs.
>>>>
>>>> Fixes: 1179f05cc9a0 ("ethdev: query proxy port to manage transfer flows")
>>>>
>>>> Signed-off-by: Ivan Malov <ivan.malov@oktetlabs.ru>
>>>> ---
>>>> --- a/lib/ethdev/rte_flow.c
>>>> +++ b/lib/ethdev/rte_flow.c
>>>> @@ -1335,10 +1335,7 @@ rte_flow_pick_transfer_proxy(uint16_t port_id, uint16_t *proxy_port_id,
>>>>    	const struct rte_flow_ops *ops = rte_flow_ops_get(port_id, error);
>>>>    	struct rte_eth_dev *dev;
>>>>    
>>>> -	if (unlikely(ops == NULL))
>>>> -		return -rte_errno;
>>>> -
>>>> -	if (ops->pick_transfer_proxy == NULL) {
>>>> +	if (ops == NULL || ops->pick_transfer_proxy == NULL) {
>>>>    		*proxy_port_id = port_id;
>>>>    		return 0;
>>>>    	}
>>>
>>> I prefer this logic.
>>
>> Thank you.
>>
>>> You could add a comment to say that the current port is the default.
>>
>> As far as I remember, the comment ("note") is already in place (rte_flow.h).
> 
> I meant adding a comment in the implementation above.

Technically, I don't object adding it. But isn't the
idea expressed clear enough by the code itself?

> 
>>> There is also this logic in testpmd:
>>>
>>>       port->flow_transfer_proxy = port_id;
>>>       if (!is_proc_primary())
>>>           return;
>>>
>>> Could we manage secondary process case inside the API?
>>
>> Shouldn't we manage secondary process in *all* flow APIs then?
> 
> Hmm, yes logically we should not care about secondary process at all in rte_flow.
> OK to leave it as is.

Thank you.

> 
>>> One more comment, for testpmd,
>>> we are calling rte_flow_pick_transfer_proxy even if we do not config any transfer flow.
>>> It is called always in init_config_port_offloads().
>>> It looks wrong. Can we call it only when needed?
>>
>> In which way does it look wrong? Does it inflict error(s), malfunction,
>> performance drops? Please elaborate.
> 
> It is testing a function that we don't intend to test in a basic use case.

Not really. The original idea is to invoke this API only once, on
port (re-)plug and remember the proxy port ID to be used on each
flow create invocation. Theoretically, when the new asynchronous
flow API arrives, this approach will be even more to the point.

> A driver can introduce a malfunction with this API while
> we don't use rte_flow at all in the test scenario.

Fat chance. Even if that happens, it will draw attention. It is
the duty of test-pmd to detect such malfunction after all. If
the current code comes across a bug in some driver, it should
be good, shouldn't it? Test coverage gets extended, right?

-- 
Ivan M