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

[Thomas Monjalon <thomas@monjalon.net>]

28/10/2021 18:24, Ivan Malov:
> On 27/10/2021 13:57, Thomas Monjalon wrote:
> > 27/10/2021 11:55, Ivan Malov:
> >> On 27/10/2021 12:46, Thomas Monjalon wrote:
> >>> 27/10/2021 11:00, Ivan Malov:
> >>>> -	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?

In general I like having a global idea as comment
to make clear it is the intent, but no strong opinion.

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

I understand, but this one-time call could be done only
when configuring the first transfer flow.

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

testpmd duty is to test some precise scenarios.
We don't test metering if not requested for example.

What others think?