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

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

[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>
---
 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) {
 		*proxy_port_id = port_id;
 		return 0;
 	}
-- 
2.20.1

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

[Thomas Monjalon]

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

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

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

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

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

[Thomas Monjalon]

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.

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

[Ivan Malov]

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?

-- 
Ivan M

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

[Thomas Monjalon]

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?

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

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

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

[Thomas Monjalon]

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.

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

[Andrew Rybchenko]

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.

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

[David Marchand]

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.


-- 
David Marchand

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

[Ori Kam]

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

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

[Ferruh Yigit]

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

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

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

--
Ivan M.

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

[Thomas Monjalon]

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.

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

[Ori Kam]

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

[PATCH] app/testpmd: fix flow transfer proxy port handling

[Ivan Malov]

The current approach detects the proxy port on each port (re-)plug and
may spam the log with error messages if the PMD does not support flows.
As testpmd is a debug tool, it must not do such implicit port handling.
Instead, the new API should be called only when the user requests that.

Revoke the existing code. Implement an explicit command-line primitive
to let the user find the proxy port themselves. Provide relevant hints.

Fixes: 1179f05cc9a0 ("ethdev: query proxy port to manage transfer flows")

Signed-off-by: Ivan Malov <ivan.malov@oktetlabs.ru>
Reviewed-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
---
 app/test-pmd/cmdline.c                      |  75 ++++++++++++
 app/test-pmd/config.c                       | 121 +++-----------------
 app/test-pmd/testpmd.c                      |  20 ----
 app/test-pmd/testpmd.h                      |   4 -
 app/test-pmd/util.c                         |   5 +-
 doc/guides/testpmd_app_ug/testpmd_funcs.rst |  13 +++
 6 files changed, 107 insertions(+), 131 deletions(-)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 4f51b259fe..a8c199f0ee 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -252,6 +252,9 @@ static void cmd_help_long_parsed(void *parsed_result,
 			"show port (port_id) macs|mcast_macs"
 			"       Display list of mac addresses added to port.\n\n"
 
+			"show port (port_id) flow transfer proxy\n"
+			"	Display proxy port to manage transfer flows\n\n"
+
 			"show port (port_id) fec capabilities"
 			"	Show fec capabilities of a port.\n\n"
 
@@ -17588,6 +17591,77 @@ cmdline_parse_inst_t cmd_showport_macs = {
 	},
 };
 
+/* *** show flow transfer proxy port ID for the given port *** */
+struct cmd_show_port_flow_transfer_proxy_result {
+	cmdline_fixed_string_t show;
+	cmdline_fixed_string_t port;
+	portid_t port_id;
+	cmdline_fixed_string_t flow;
+	cmdline_fixed_string_t transfer;
+	cmdline_fixed_string_t proxy;
+};
+
+cmdline_parse_token_string_t cmd_show_port_flow_transfer_proxy_show =
+	TOKEN_STRING_INITIALIZER
+		(struct cmd_show_port_flow_transfer_proxy_result,
+		 show, "show");
+cmdline_parse_token_string_t cmd_show_port_flow_transfer_proxy_port =
+	TOKEN_STRING_INITIALIZER
+		(struct cmd_show_port_flow_transfer_proxy_result,
+		 port, "port");
+cmdline_parse_token_num_t cmd_show_port_flow_transfer_proxy_port_id =
+	TOKEN_NUM_INITIALIZER
+		(struct cmd_show_port_flow_transfer_proxy_result,
+		 port_id, RTE_UINT16);
+cmdline_parse_token_string_t cmd_show_port_flow_transfer_proxy_flow =
+	TOKEN_STRING_INITIALIZER
+		(struct cmd_show_port_flow_transfer_proxy_result,
+		 flow, "flow");
+cmdline_parse_token_string_t cmd_show_port_flow_transfer_proxy_transfer =
+	TOKEN_STRING_INITIALIZER
+		(struct cmd_show_port_flow_transfer_proxy_result,
+		 transfer, "transfer");
+cmdline_parse_token_string_t cmd_show_port_flow_transfer_proxy_proxy =
+	TOKEN_STRING_INITIALIZER
+		(struct cmd_show_port_flow_transfer_proxy_result,
+		 proxy, "proxy");
+
+static void
+cmd_show_port_flow_transfer_proxy_parsed(void *parsed_result,
+					 __rte_unused struct cmdline *cl,
+					 __rte_unused void *data)
+{
+	struct cmd_show_port_flow_transfer_proxy_result *res = parsed_result;
+	portid_t proxy_port_id;
+	int ret;
+
+	printf("\n");
+
+	ret = rte_flow_pick_transfer_proxy(res->port_id, &proxy_port_id, NULL);
+	if (ret != 0) {
+		fprintf(stderr, "Failed to pick transfer proxy: %s\n",
+			rte_strerror(-ret));
+		return;
+	}
+
+	printf("Transfer proxy port ID: %u\n\n", proxy_port_id);
+}
+
+cmdline_parse_inst_t cmd_show_port_flow_transfer_proxy = {
+	.f = cmd_show_port_flow_transfer_proxy_parsed,
+	.data = NULL,
+	.help_str = "show port <port_id> flow transfer proxy",
+	.tokens = {
+		(void *)&cmd_show_port_flow_transfer_proxy_show,
+		(void *)&cmd_show_port_flow_transfer_proxy_port,
+		(void *)&cmd_show_port_flow_transfer_proxy_port_id,
+		(void *)&cmd_show_port_flow_transfer_proxy_flow,
+		(void *)&cmd_show_port_flow_transfer_proxy_transfer,
+		(void *)&cmd_show_port_flow_transfer_proxy_proxy,
+		NULL,
+	}
+};
+
 /* ******************************************************************************** */
 
 /* list of instructions */
@@ -17716,6 +17790,7 @@ cmdline_parse_ctx_t main_ctx[] = {
 	(cmdline_parse_inst_t *)&cmd_config_rss_reta,
 	(cmdline_parse_inst_t *)&cmd_showport_reta,
 	(cmdline_parse_inst_t *)&cmd_showport_macs,
+	(cmdline_parse_inst_t *)&cmd_show_port_flow_transfer_proxy,
 	(cmdline_parse_inst_t *)&cmd_config_burst,
 	(cmdline_parse_inst_t *)&cmd_config_thresh,
 	(cmdline_parse_inst_t *)&cmd_config_threshold,
diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index 26cadf39f7..0477e02abe 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -1449,6 +1449,21 @@ port_flow_complain(struct rte_flow_error *error)
 					 error->cause), buf) : "",
 		error->message ? error->message : "(no stated reason)",
 		rte_strerror(err));
+
+	switch (error->type) {
+	case RTE_FLOW_ERROR_TYPE_ATTR_TRANSFER:
+		fprintf(stderr, "The status suggests the use of \"transfer\" "
+				"as the possible cause of the failure. Make "
+				"sure that the flow in question and its "
+				"indirect components (if any) are managed "
+				"via \"transfer\" proxy port. Use command "
+				"\"show port (port_id) flow transfer proxy\" "
+				"to figure out the proxy port ID\n");
+		break;
+	default:
+		break;
+	}
+
 	return -err;
 }
 
@@ -1588,25 +1603,10 @@ port_action_handle_create(portid_t port_id, uint32_t id,
 	struct port_indirect_action *pia;
 	int ret;
 	struct rte_flow_error error;
-	struct rte_port *port;
-
-	if (port_id_is_invalid(port_id, ENABLED_WARN) ||
-	    port_id == (portid_t)RTE_PORT_ALL)
-		return -EINVAL;
 
 	ret = action_alloc(port_id, id, &pia);
 	if (ret)
 		return ret;
-
-	port = &ports[port_id];
-
-	if (conf->transfer)
-		port_id = port->flow_transfer_proxy;
-
-	if (port_id_is_invalid(port_id, ENABLED_WARN) ||
-	    port_id == (portid_t)RTE_PORT_ALL)
-		return -EINVAL;
-
 	if (action->type == RTE_FLOW_ACTION_TYPE_AGE) {
 		struct rte_flow_action_age *age =
 			(struct rte_flow_action_age *)(uintptr_t)(action->conf);
@@ -1629,7 +1629,6 @@ port_action_handle_create(portid_t port_id, uint32_t id,
 		return port_flow_complain(&error);
 	}
 	pia->type = action->type;
-	pia->transfer = conf->transfer;
 	printf("Indirect action #%u created\n", pia->id);
 	return 0;
 }
@@ -1656,18 +1655,9 @@ port_action_handle_destroy(portid_t port_id,
 		for (i = 0; i != n; ++i) {
 			struct rte_flow_error error;
 			struct port_indirect_action *pia = *tmp;
-			portid_t port_id_eff = port_id;
 
 			if (actions[i] != pia->id)
 				continue;
-
-			if (pia->transfer)
-				port_id_eff = port->flow_transfer_proxy;
-
-			if (port_id_is_invalid(port_id_eff, ENABLED_WARN) ||
-			    port_id_eff == (portid_t)RTE_PORT_ALL)
-				return -EINVAL;
-
 			/*
 			 * Poisoning to make sure PMDs update it in case
 			 * of error.
@@ -1675,7 +1665,7 @@ port_action_handle_destroy(portid_t port_id,
 			memset(&error, 0x33, sizeof(error));
 
 			if (pia->handle && rte_flow_action_handle_destroy(
-					port_id_eff, pia->handle, &error)) {
+					port_id, pia->handle, &error)) {
 				ret = port_flow_complain(&error);
 				continue;
 			}
@@ -1710,15 +1700,8 @@ port_action_handle_update(portid_t port_id, uint32_t id,
 	struct rte_flow_error error;
 	struct rte_flow_action_handle *action_handle;
 	struct port_indirect_action *pia;
-	struct rte_port *port;
 	const void *update;
 
-	if (port_id_is_invalid(port_id, ENABLED_WARN) ||
-	    port_id == (portid_t)RTE_PORT_ALL)
-		return -EINVAL;
-
-	port = &ports[port_id];
-
 	action_handle = port_action_handle_get_by_id(port_id, id);
 	if (!action_handle)
 		return -EINVAL;
@@ -1733,14 +1716,6 @@ port_action_handle_update(portid_t port_id, uint32_t id,
 		update = action;
 		break;
 	}
-
-	if (pia->transfer)
-		port_id = port->flow_transfer_proxy;
-
-	if (port_id_is_invalid(port_id, ENABLED_WARN) ||
-	    port_id == (portid_t)RTE_PORT_ALL)
-		return -EINVAL;
-
 	if (rte_flow_action_handle_update(port_id, action_handle, update,
 					  &error)) {
 		return port_flow_complain(&error);
@@ -1759,14 +1734,6 @@ port_action_handle_query(portid_t port_id, uint32_t id)
 		struct rte_flow_query_age age;
 		struct rte_flow_action_conntrack ct;
 	} query;
-	portid_t port_id_eff = port_id;
-	struct rte_port *port;
-
-	if (port_id_is_invalid(port_id, ENABLED_WARN) ||
-	    port_id == (portid_t)RTE_PORT_ALL)
-		return -EINVAL;
-
-	port = &ports[port_id];
 
 	pia = action_get_by_id(port_id, id);
 	if (!pia)
@@ -1781,19 +1748,10 @@ port_action_handle_query(portid_t port_id, uint32_t id)
 			id, pia->type, port_id);
 		return -ENOTSUP;
 	}
-
-	if (pia->transfer)
-		port_id_eff = port->flow_transfer_proxy;
-
-	if (port_id_is_invalid(port_id_eff, ENABLED_WARN) ||
-	    port_id_eff == (portid_t)RTE_PORT_ALL)
-		return -EINVAL;
-
 	/* Poisoning to make sure PMDs update it in case of error. */
 	memset(&error, 0x55, sizeof(error));
 	memset(&query, 0, sizeof(query));
-	if (rte_flow_action_handle_query(port_id_eff, pia->handle, &query,
-					 &error))
+	if (rte_flow_action_handle_query(port_id, pia->handle, &query, &error))
 		return port_flow_complain(&error);
 	switch (pia->type) {
 	case RTE_FLOW_ACTION_TYPE_AGE:
@@ -2012,20 +1970,6 @@ port_flow_validate(portid_t port_id,
 {
 	struct rte_flow_error error;
 	struct port_flow_tunnel *pft = NULL;
-	struct rte_port *port;
-
-	if (port_id_is_invalid(port_id, ENABLED_WARN) ||
-	    port_id == (portid_t)RTE_PORT_ALL)
-		return -EINVAL;
-
-	port = &ports[port_id];
-
-	if (attr->transfer)
-		port_id = port->flow_transfer_proxy;
-
-	if (port_id_is_invalid(port_id, ENABLED_WARN) ||
-	    port_id == (portid_t)RTE_PORT_ALL)
-		return -EINVAL;
 
 	/* Poisoning to make sure PMDs update it in case of error. */
 	memset(&error, 0x11, sizeof(error));
@@ -2079,19 +2023,7 @@ port_flow_create(portid_t port_id,
 	struct port_flow_tunnel *pft = NULL;
 	struct rte_flow_action_age *age = age_action_get(actions);
 
-	if (port_id_is_invalid(port_id, ENABLED_WARN) ||
-	    port_id == (portid_t)RTE_PORT_ALL)
-		return -EINVAL;
-
 	port = &ports[port_id];
-
-	if (attr->transfer)
-		port_id = port->flow_transfer_proxy;
-
-	if (port_id_is_invalid(port_id, ENABLED_WARN) ||
-	    port_id == (portid_t)RTE_PORT_ALL)
-		return -EINVAL;
-
 	if (port->flow_list) {
 		if (port->flow_list->id == UINT32_MAX) {
 			fprintf(stderr,
@@ -2155,7 +2087,6 @@ port_flow_destroy(portid_t port_id, uint32_t n, const uint32_t *rule)
 		uint32_t i;
 
 		for (i = 0; i != n; ++i) {
-			portid_t port_id_eff = port_id;
 			struct rte_flow_error error;
 			struct port_flow *pf = *tmp;
 
@@ -2166,15 +2097,7 @@ port_flow_destroy(portid_t port_id, uint32_t n, const uint32_t *rule)
 			 * of error.
 			 */
 			memset(&error, 0x33, sizeof(error));
-
-			if (pf->rule.attr->transfer)
-				port_id_eff = port->flow_transfer_proxy;
-
-			if (port_id_is_invalid(port_id_eff, ENABLED_WARN) ||
-			    port_id_eff == (portid_t)RTE_PORT_ALL)
-				return -EINVAL;
-
-			if (rte_flow_destroy(port_id_eff, pf->flow, &error)) {
+			if (rte_flow_destroy(port_id, pf->flow, &error)) {
 				ret = port_flow_complain(&error);
 				continue;
 			}
@@ -2308,14 +2231,6 @@ port_flow_query(portid_t port_id, uint32_t rule,
 		fprintf(stderr, "Flow rule #%u not found\n", rule);
 		return -ENOENT;
 	}
-
-	if (pf->rule.attr->transfer)
-		port_id = port->flow_transfer_proxy;
-
-	if (port_id_is_invalid(port_id, ENABLED_WARN) ||
-	    port_id == (portid_t)RTE_PORT_ALL)
-		return -EINVAL;
-
 	ret = rte_flow_conv(RTE_FLOW_CONV_OP_ACTION_NAME_PTR,
 			    &name, sizeof(name),
 			    (void *)(uintptr_t)action->type, &error);
diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index c18942279a..37c67180de 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -578,25 +578,6 @@ eth_rx_metadata_negotiate_mp(uint16_t port_id)
 	}
 }
 
-static void
-flow_pick_transfer_proxy_mp(uint16_t port_id)
-{
-	struct rte_port *port = &ports[port_id];
-	int ret;
-
-	port->flow_transfer_proxy = port_id;
-
-	if (!is_proc_primary())
-		return;
-
-	ret = rte_flow_pick_transfer_proxy(port_id, &port->flow_transfer_proxy,
-					   NULL);
-	if (ret != 0) {
-		fprintf(stderr, "Error picking flow transfer proxy for port %u: %s - ignore\n",
-			port_id, rte_strerror(-ret));
-	}
-}
-
 static int
 eth_dev_configure_mp(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
 		      const struct rte_eth_conf *dev_conf)
@@ -1569,7 +1550,6 @@ init_config_port_offloads(portid_t pid, uint32_t socket_id)
 	int i;
 
 	eth_rx_metadata_negotiate_mp(pid);
-	flow_pick_transfer_proxy_mp(pid);
 
 	port->dev_conf.txmode = tx_mode;
 	port->dev_conf.rxmode = rx_mode;
diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
index 669ce1e87d..3492ce8217 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -177,8 +177,6 @@ struct port_indirect_action {
 	enum rte_flow_action_type type; /**< Action type. */
 	struct rte_flow_action_handle *handle;	/**< Indirect action handle. */
 	enum age_action_context_type age_type; /**< Age action context type. */
-	/** If true, the action applies to "transfer" flows, and vice versa */
-	bool transfer;
 };
 
 struct port_flow_tunnel {
@@ -251,8 +249,6 @@ struct rte_port {
 	/**< dynamic flags. */
 	uint64_t		mbuf_dynf;
 	const struct rte_eth_rxtx_callback *tx_set_dynf_cb[RTE_MAX_QUEUES_PER_PORT+1];
-	/** Associated port which is supposed to handle "transfer" flows */
-	portid_t		flow_transfer_proxy;
 	struct xstat_display_info xstats_info;
 };
 
diff --git a/app/test-pmd/util.c b/app/test-pmd/util.c
index 8020f999d6..fd98e8b51d 100644
--- a/app/test-pmd/util.c
+++ b/app/test-pmd/util.c
@@ -98,7 +98,6 @@ dump_pkt_burst(uint16_t port_id, uint16_t queue, struct rte_mbuf *pkts[],
 		int ret;
 		struct rte_flow_error error;
 		struct rte_flow_restore_info info = { 0, };
-		struct rte_port *port = &ports[port_id];
 
 		mb = pkts[i];
 		if (rxq_share > 0)
@@ -108,9 +107,7 @@ dump_pkt_burst(uint16_t port_id, uint16_t queue, struct rte_mbuf *pkts[],
 		eth_type = RTE_BE_TO_CPU_16(eth_hdr->ether_type);
 		packet_type = mb->packet_type;
 		is_encapsulation = RTE_ETH_IS_TUNNEL_PKT(packet_type);
-
-		ret = rte_flow_get_restore_info(port->flow_transfer_proxy,
-						mb, &info, &error);
+		ret = rte_flow_get_restore_info(port_id, mb, &info, &error);
 		if (!ret) {
 			MKDUMPSTR(print_buf, buf_size, cur_len,
 				  "restore info:");
diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
index d8d50539e2..44228cd7d2 100644
--- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
+++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
@@ -527,6 +527,15 @@ Show multicast mac addresses added for a specific port::
 
    testpmd> show port (port_id) mcast_macs
 
+show flow transfer proxy port ID for the given port
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+Show proxy port ID to use as the 1st argument in commands to
+manage ``transfer`` flows and their indirect components.
+::
+
+   testpmd> show port (port_id) flow transfer proxy
+
 show device info
 ~~~~~~~~~~~~~~~~
 
@@ -3477,6 +3486,10 @@ specified before the ``pattern`` token.
 - ``egress``: rule applies to egress traffic.
 - ``transfer``: apply rule directly to endpoints found in pattern.
 
+Please note that use of ``transfer`` attribute requires that the flow and
+its indirect components be managed via so-called ``transfer`` proxy port.
+See `show flow transfer proxy port ID for the given port`_ for details.
+
 Each instance of an attribute specified several times overrides the previous
 value as shown below (group 4 is used)::
 
-- 
2.30.2

RE: [PATCH] app/testpmd: fix flow transfer proxy port handling

[Ori Kam]

Hi Ivan,

> -----Original Message-----
> From: Ivan Malov <ivan.malov@oktetlabs.ru>
> Sent: Tuesday, November 16, 2021 5:38 PM
> Subject: [PATCH] app/testpmd: fix flow transfer proxy port handling
> 
> The current approach detects the proxy port on each port (re-)plug and
> may spam the log with error messages if the PMD does not support flows.
> As testpmd is a debug tool, it must not do such implicit port handling.
> Instead, the new API should be called only when the user requests that.
> 
> Revoke the existing code. Implement an explicit command-line primitive
> to let the user find the proxy port themselves. Provide relevant hints.
> 
> Fixes: 1179f05cc9a0 ("ethdev: query proxy port to manage transfer flows")
> 
> Signed-off-by: Ivan Malov <ivan.malov@oktetlabs.ru>
> Reviewed-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> ---

Acked-by: Ori Kam <orika@nvidia.com>
Best,
Ori

RE: [PATCH] app/testpmd: fix flow transfer proxy port handling

[Slava Ovsiienko]

> -----Original Message-----
> From: Ivan Malov <ivan.malov@oktetlabs.ru>
> Sent: Tuesday, November 16, 2021 17:38
> To: dev@dpdk.org
> Cc: David Marchand <david.marchand@redhat.com>; Ferruh Yigit
> <ferruh.yigit@intel.com>; NBU-Contact-Thomas Monjalon
> <thomas@monjalon.net>; Andrew Rybchenko
> <andrew.rybchenko@oktetlabs.ru>; Xiaoyun Li <xiaoyun.li@intel.com>; Ori
> Kam <orika@nvidia.com>
> Subject: [PATCH] app/testpmd: fix flow transfer proxy port handling
> 
> The current approach detects the proxy port on each port (re-)plug and may
> spam the log with error messages if the PMD does not support flows.
> As testpmd is a debug tool, it must not do such implicit port handling.
> Instead, the new API should be called only when the user requests that.
> 
> Revoke the existing code. Implement an explicit command-line primitive to let
> the user find the proxy port themselves. Provide relevant hints.
> 
> Fixes: 1179f05cc9a0 ("ethdev: query proxy port to manage transfer flows")
> 
> Signed-off-by: Ivan Malov <ivan.malov@oktetlabs.ru>
> Reviewed-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
Acked-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>

Re: [PATCH] app/testpmd: fix flow transfer proxy port handling

[Ferruh Yigit]

On 11/17/2021 7:41 AM, Slava Ovsiienko wrote:
>> -----Original Message-----
>> From: Ivan Malov <ivan.malov@oktetlabs.ru>
>> Sent: Tuesday, November 16, 2021 17:38
>> To: dev@dpdk.org
>> Cc: David Marchand <david.marchand@redhat.com>; Ferruh Yigit
>> <ferruh.yigit@intel.com>; NBU-Contact-Thomas Monjalon
>> <thomas@monjalon.net>; Andrew Rybchenko
>> <andrew.rybchenko@oktetlabs.ru>; Xiaoyun Li <xiaoyun.li@intel.com>; Ori
>> Kam <orika@nvidia.com>
>> Subject: [PATCH] app/testpmd: fix flow transfer proxy port handling
>>
>> The current approach detects the proxy port on each port (re-)plug and may
>> spam the log with error messages if the PMD does not support flows.
>> As testpmd is a debug tool, it must not do such implicit port handling.
>> Instead, the new API should be called only when the user requests that.
>>
>> Revoke the existing code. Implement an explicit command-line primitive to let
>> the user find the proxy port themselves. Provide relevant hints.
>>
>> Fixes: 1179f05cc9a0 ("ethdev: query proxy port to manage transfer flows")
>>
>> Signed-off-by: Ivan Malov <ivan.malov@oktetlabs.ru>
>> Reviewed-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Acked-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
> 

Applied to dpdk-next-net/main, thanks.