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

[Thomas Monjalon <thomas@monjalon.net>]

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.

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

> > 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.
A driver can introduce a malfunction with this API while
we don't use rte_flow at all in the test scenario.