diff mbox series

ethdev: fine tune error reporting in pick transfer proxy API

Message ID 20211027090003.14556-1-ivan.malov@oktetlabs.ru (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers
Series ethdev: fine tune error reporting in pick transfer proxy API |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK
ci/github-robot: build success github build: passed
ci/intel-Testing success Testing PASS
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-x86_64-unit-testing success Testing PASS
ci/iol-x86_64-compile-testing success Testing PASS
ci/iol-aarch64-compile-testing success Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-aarch64-unit-testing success Testing PASS

Commit Message

Ivan Malov Oct. 27, 2021, 9 a.m. UTC 
  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>
---
 lib/ethdev/rte_flow.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

Comments

Thomas Monjalon Oct. 27, 2021, 9:46 a.m. UTC | #1 
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.
You could add a comment to say that the current port is the default.

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?

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?

Thanks
Ivan Malov Oct. 27, 2021, 9:55 a.m. UTC | #2 
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
>
Thomas Monjalon Oct. 27, 2021, 10:57 a.m. UTC | #3 
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.
Ivan Malov Oct. 28, 2021, 4:24 p.m. UTC | #4 
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?
Thomas Monjalon Oct. 29, 2021, 8:11 a.m. UTC | #5 
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?
Andrew Rybchenko Nov. 1, 2021, 9:41 a.m. UTC | #6 
On 10/27/21 12:00 PM, Ivan Malov wrote:
> 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>
> ---
>   lib/ethdev/rte_flow.c | 5 +----
>   1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/lib/ethdev/rte_flow.c b/lib/ethdev/rte_flow.c
> index d268784532..9d98d2d716 100644
> --- 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) {

First of all I think that the patch is wrong and origin code is better.
If flow API is not supported at all (ops == NULL), what's the point
to return some proxy port?

>   		*proxy_port_id = port_id;
>   		return 0;
>   	}
> 

IMHO, spamming of testpmd logs in described case should be fixed
in testpmd itself to avoid logs in the case of ENOTSUP. That's it.
Thomas Monjalon Nov. 2, 2021, 3:45 p.m. UTC | #7 
01/11/2021 10:41, Andrew Rybchenko:
> On 10/27/21 12:00 PM, Ivan Malov wrote:
> > 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>
> > ---
> >   lib/ethdev/rte_flow.c | 5 +----
> >   1 file changed, 1 insertion(+), 4 deletions(-)
> > 
> > diff --git a/lib/ethdev/rte_flow.c b/lib/ethdev/rte_flow.c
> > index d268784532..9d98d2d716 100644
> > --- 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) {
> 
> First of all I think that the patch is wrong and origin code is better.
> If flow API is not supported at all (ops == NULL), what's the point
> to return some proxy port?
> 
> >   		*proxy_port_id = port_id;
> >   		return 0;
> >   	}
> > 
> 
> IMHO, spamming of testpmd logs in described case should be fixed
> in testpmd itself to avoid logs in the case of ENOTSUP. That's it.

I think we should not call this API in testpmd
if not doing rte_flow transfer rule.
Andrew Rybchenko Nov. 2, 2021, 3:58 p.m. UTC | #8 
On 11/2/21 6:45 PM, Thomas Monjalon wrote:
> 01/11/2021 10:41, Andrew Rybchenko:
>> On 10/27/21 12:00 PM, Ivan Malov wrote:
>>> 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>
>>> ---
>>>    lib/ethdev/rte_flow.c | 5 +----
>>>    1 file changed, 1 insertion(+), 4 deletions(-)
>>>
>>> diff --git a/lib/ethdev/rte_flow.c b/lib/ethdev/rte_flow.c
>>> index d268784532..9d98d2d716 100644
>>> --- 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) {
>>
>> First of all I think that the patch is wrong and origin code is better.
>> If flow API is not supported at all (ops == NULL), what's the point
>> to return some proxy port?
>>
>>>    		*proxy_port_id = port_id;
>>>    		return 0;
>>>    	}
>>>
>>
>> IMHO, spamming of testpmd logs in described case should be fixed
>> in testpmd itself to avoid logs in the case of ENOTSUP. That's it.
> 
> I think we should not call this API in testpmd
> if not doing rte_flow transfer rule.
> 

Since testpmd does not pursue top flow insertion rate etc,
I tend to agree. For debugging purposes (testpmd main goal)
the best way is to pick proxy just before usage without any
caching etc.
David Marchand Nov. 2, 2021, 5:04 p.m. UTC | #9 
On Tue, Nov 2, 2021 at 4:59 PM Andrew Rybchenko
<andrew.rybchenko@oktetlabs.ru> wrote:
> >> IMHO, spamming of testpmd logs in described case should be fixed
> >> in testpmd itself to avoid logs in the case of ENOTSUP. That's it.
> >
> > I think we should not call this API in testpmd
> > if not doing rte_flow transfer rule.
> >
>
> Since testpmd does not pursue top flow insertion rate etc,
> I tend to agree. For debugging purposes (testpmd main goal)
> the best way is to pick proxy just before usage without any
> caching etc.

+1.
Ori Kam Nov. 3, 2021, 2:38 p.m. UTC | #10 
> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Thomas Monjalon
> Sent: Tuesday, November 2, 2021 5:46 PM
> To: Ivan Malov <ivan.malov@oktetlabs.ru>; Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Cc: dev@dpdk.org; David Marchand <david.marchand@redhat.com>; Ferruh Yigit
> <ferruh.yigit@intel.com>; Ori Kam <orika@nvidia.com>
> Subject: Re: [dpdk-dev] [PATCH] ethdev: fine tune error reporting in pick transfer proxy API
> 
> 01/11/2021 10:41, Andrew Rybchenko:
> > On 10/27/21 12:00 PM, Ivan Malov wrote:
> > > 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>
> > > ---
> > >   lib/ethdev/rte_flow.c | 5 +----
> > >   1 file changed, 1 insertion(+), 4 deletions(-)
> > >
> > > diff --git a/lib/ethdev/rte_flow.c b/lib/ethdev/rte_flow.c
> > > index d268784532..9d98d2d716 100644
> > > --- 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) {
> >
> > First of all I think that the patch is wrong and origin code is better.
> > If flow API is not supported at all (ops == NULL), what's the point
> > to return some proxy port?
> >
> > >   		*proxy_port_id = port_id;
> > >   		return 0;
> > >   	}
> > >
> >
> > IMHO, spamming of testpmd logs in described case should be fixed
> > in testpmd itself to avoid logs in the case of ENOTSUP. That's it.
> 
> I think we should not call this API in testpmd
> if not doing rte_flow transfer rule.
> 

+1 too the two points above.

Best,
Ori
Ferruh Yigit Nov. 10, 2021, 2:21 p.m. UTC | #11 
On 11/2/2021 5:04 PM, David Marchand wrote:
> On Tue, Nov 2, 2021 at 4:59 PM Andrew Rybchenko
> <andrew.rybchenko@oktetlabs.ru> wrote:
>>>> IMHO, spamming of testpmd logs in described case should be fixed
>>>> in testpmd itself to avoid logs in the case of ENOTSUP. That's it.
>>>
>>> I think we should not call this API in testpmd
>>> if not doing rte_flow transfer rule.
>>>
>>
>> Since testpmd does not pursue top flow insertion rate etc,
>> I tend to agree. For debugging purposes (testpmd main goal)
>> the best way is to pick proxy just before usage without any
>> caching etc.
> 
> +1.
> 

+1 to not call this API when not doing rte_flow transfer rule,
and get rid of this testpmd logs..
Ivan Malov Nov. 15, 2021, 2:15 p.m. UTC | #12 
On Wed, 10 Nov 2021, Ferruh Yigit wrote:

> On 11/2/2021 5:04 PM, David Marchand wrote:
>> On Tue, Nov 2, 2021 at 4:59 PM Andrew Rybchenko
>> <andrew.rybchenko@oktetlabs.ru> wrote:
>>>>> IMHO, spamming of testpmd logs in described case should be fixed
>>>>> in testpmd itself to avoid logs in the case of ENOTSUP. That's it.
>>>> 
>>>> I think we should not call this API in testpmd
>>>> if not doing rte_flow transfer rule.
>>>> 
>>> 
>>> Since testpmd does not pursue top flow insertion rate etc,
>>> I tend to agree. For debugging purposes (testpmd main goal)
>>> the best way is to pick proxy just before usage without any
>>> caching etc.
>> 
>> +1.
>> 
>
> +1 to not call this API when not doing rte_flow transfer rule,
> and get rid of this testpmd logs..
>

Hi all,

I apologise for the delay. These arguments make sense. Thanks.

However, the idea to hide the proxy port request inside flow
command handlers might be wrong in fact. It is too much code
churn. And it is easy to overlook this part when adding new
indirect action handlers that are also suited for use in
transfer flows. To code maintainers, it is very vague.

Now you mention it, testpmd is indeed scenario-dependent. Its
workings should be explicitly controllable by the user. That
means, the proxy thing should be exposed via an explicit
command: "show port <port_id> flow_transfer_proxy".

As testpmd is a debug tool, it should simply do what the user
says. Suppose, the user wants to create transfer flows. They
realise that doing so may require proxy. Hence, they request
the proxy and then use the resulting port ID in all commands
which have something to do with transfer flows. Very robust.

And, of course, this way, the request is done only when the
user needs it, and spamming the log is also gone. Let the
user query the proxy themselves, and things become simple.

Please vote in favor of my motion. It will not break anything.
Right now, in this release cycle, nobody supports the proxy
thing, so the existing flow use cases should work as normal.

Opinions are welcome.

--
Ivan M.
Thomas Monjalon Nov. 15, 2021, 3:09 p.m. UTC | #13 
15/11/2021 15:15, Ivan Malov:
> On Wed, 10 Nov 2021, Ferruh Yigit wrote:
> 
> > On 11/2/2021 5:04 PM, David Marchand wrote:
> >> On Tue, Nov 2, 2021 at 4:59 PM Andrew Rybchenko
> >> <andrew.rybchenko@oktetlabs.ru> wrote:
> >>>>> IMHO, spamming of testpmd logs in described case should be fixed
> >>>>> in testpmd itself to avoid logs in the case of ENOTSUP. That's it.
> >>>> 
> >>>> I think we should not call this API in testpmd
> >>>> if not doing rte_flow transfer rule.
> >>>> 
> >>> 
> >>> Since testpmd does not pursue top flow insertion rate etc,
> >>> I tend to agree. For debugging purposes (testpmd main goal)
> >>> the best way is to pick proxy just before usage without any
> >>> caching etc.
> >> 
> >> +1.
> >> 
> >
> > +1 to not call this API when not doing rte_flow transfer rule,
> > and get rid of this testpmd logs..
> >
> 
> Hi all,
> 
> I apologise for the delay. These arguments make sense. Thanks.
> 
> However, the idea to hide the proxy port request inside flow
> command handlers might be wrong in fact. It is too much code
> churn. And it is easy to overlook this part when adding new
> indirect action handlers that are also suited for use in
> transfer flows. To code maintainers, it is very vague.
> 
> Now you mention it, testpmd is indeed scenario-dependent. Its
> workings should be explicitly controllable by the user. That
> means, the proxy thing should be exposed via an explicit
> command: "show port <port_id> flow_transfer_proxy".
> 
> As testpmd is a debug tool, it should simply do what the user
> says. Suppose, the user wants to create transfer flows. They
> realise that doing so may require proxy. Hence, they request
> the proxy and then use the resulting port ID in all commands
> which have something to do with transfer flows. Very robust.
> 
> And, of course, this way, the request is done only when the
> user needs it, and spamming the log is also gone. Let the
> user query the proxy themselves, and things become simple.
> 
> Please vote in favor of my motion. It will not break anything.
> Right now, in this release cycle, nobody supports the proxy
> thing, so the existing flow use cases should work as normal.
> 
> Opinions are welcome.

I'm fine with this proposal.
Ori Kam Nov. 15, 2021, 3:30 p.m. UTC | #14 
> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Monday, November 15, 2021 5:09 PM
> Subject: Re: [dpdk-dev] [PATCH] ethdev: fine tune error reporting in pick transfer proxy API
> 
> 15/11/2021 15:15, Ivan Malov:
> > On Wed, 10 Nov 2021, Ferruh Yigit wrote:
> >
> > > On 11/2/2021 5:04 PM, David Marchand wrote:
> > >> On Tue, Nov 2, 2021 at 4:59 PM Andrew Rybchenko
> > >> <andrew.rybchenko@oktetlabs.ru> wrote:
> > >>>>> IMHO, spamming of testpmd logs in described case should be fixed
> > >>>>> in testpmd itself to avoid logs in the case of ENOTSUP. That's it.
> > >>>>
> > >>>> I think we should not call this API in testpmd
> > >>>> if not doing rte_flow transfer rule.
> > >>>>
> > >>>
> > >>> Since testpmd does not pursue top flow insertion rate etc,
> > >>> I tend to agree. For debugging purposes (testpmd main goal)
> > >>> the best way is to pick proxy just before usage without any
> > >>> caching etc.
> > >>
> > >> +1.
> > >>
> > >
> > > +1 to not call this API when not doing rte_flow transfer rule,
> > > and get rid of this testpmd logs..
> > >
> >
> > Hi all,
> >
> > I apologise for the delay. These arguments make sense. Thanks.
> >
> > However, the idea to hide the proxy port request inside flow
> > command handlers might be wrong in fact. It is too much code
> > churn. And it is easy to overlook this part when adding new
> > indirect action handlers that are also suited for use in
> > transfer flows. To code maintainers, it is very vague.
> >
> > Now you mention it, testpmd is indeed scenario-dependent. Its
> > workings should be explicitly controllable by the user. That
> > means, the proxy thing should be exposed via an explicit
> > command: "show port <port_id> flow_transfer_proxy".
> >
> > As testpmd is a debug tool, it should simply do what the user
> > says. Suppose, the user wants to create transfer flows. They
> > realise that doing so may require proxy. Hence, they request
> > the proxy and then use the resulting port ID in all commands
> > which have something to do with transfer flows. Very robust.
> >
> > And, of course, this way, the request is done only when the
> > user needs it, and spamming the log is also gone. Let the
> > user query the proxy themselves, and things become simple.
> >
> > Please vote in favor of my motion. It will not break anything.
> > Right now, in this release cycle, nobody supports the proxy
> > thing, so the existing flow use cases should work as normal.
> >
> > Opinions are welcome.
> 
> I'm fine with this proposal.
> 
+1

Ori
diff mbox series

Patch

diff --git a/lib/ethdev/rte_flow.c b/lib/ethdev/rte_flow.c
index d268784532..9d98d2d716 100644
--- 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;
 	}