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?
