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

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

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).

> 
> 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?

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

> 
> Thanks
> 

-- 
Ivan M