Re: [PATCH] app/testpmd: fix secondary process cannot dump packet

Re: [PATCH] app/testpmd: fix secondary process cannot dump packet

[Andrew Rybchenko]

On 6/23/22 21:15, peng1x.zhang@intel.com wrote:
> From: Peng Zhang <peng1x.zhang@intel.com>
> 
> The origin design is whether testpmd is primary or not, if state of
> receive queue is stop, then packets will not be dumped for show.
> While to secondary process, receive queue will not be set up, and state
> will still be stop even if testpmd is started. So packets of stated
> secondary process cannot be dumped for show.
> 
> The current design is to secondary process state of queue will be set
> to start after testpmd is started. Then packets of started secondary
> process can be dumped for show.
> 
> Fixes: a550baf24af9 ("app/testpmd: support multi-process")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Peng Zhang <peng1x.zhang@intel.com>
> ---
>   app/test-pmd/testpmd.c | 12 ++++++++++++
>   1 file changed, 12 insertions(+)
> 
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> index 205d98ee3d..93ba7e7c9b 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -3007,6 +3007,18 @@ start_port(portid_t pid)
>   			if (setup_hairpin_queues(pi, p_pi, cnt_pi) != 0)
>   				return -1;
>   		}
> +
> +		if (port->need_reconfig_queues > 0 && !is_proc_primary()) {
> +			struct rte_eth_rxconf *rx_conf;
> +			for (qi = 0; qi < nb_rxq; qi++) {
> +				rx_conf = &(port->rxq[qi].conf);
> +				ports[pi].rxq[qi].state =
> +					rx_conf->rx_deferred_start ?
> +					RTE_ETH_QUEUE_STATE_STOPPED :
> +					RTE_ETH_QUEUE_STATE_STARTED;

I'm not sure why it is correct to assume that deferred queue is
not yet started.

> +			}
> +		}
> +
>   		configure_rxtx_dump_callbacks(verbose_level);
>   		if (clear_ptypes) {
>   			diag = rte_eth_dev_set_ptypes(pi, RTE_PTYPE_UNKNOWN,

[PATCH] app/testpmd: fix secondary process cannot dump packet

[peng1x.zhang]

From: Peng Zhang <peng1x.zhang@intel.com>

The origin design is whether testpmd is primary or not, if state of
receive queue is stop, then packets will not be dumped for show.
While to secondary process, receive queue will not be set up, and state
will still be stop even if testpmd is started. So packets of stated
secondary process cannot be dumped for show.

The current design is to secondary process state of queue will be set
to start after testpmd is started. Then packets of started secondary
process can be dumped for show.

Fixes: a550baf24af9 ("app/testpmd: support multi-process")
Cc: stable@dpdk.org

Signed-off-by: Peng Zhang <peng1x.zhang@intel.com>
---
 app/test-pmd/testpmd.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 205d98ee3d..93ba7e7c9b 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -3007,6 +3007,18 @@ start_port(portid_t pid)
 			if (setup_hairpin_queues(pi, p_pi, cnt_pi) != 0)
 				return -1;
 		}
+
+		if (port->need_reconfig_queues > 0 && !is_proc_primary()) {
+			struct rte_eth_rxconf *rx_conf;
+			for (qi = 0; qi < nb_rxq; qi++) {
+				rx_conf = &(port->rxq[qi].conf);
+				ports[pi].rxq[qi].state =
+					rx_conf->rx_deferred_start ?
+					RTE_ETH_QUEUE_STATE_STOPPED :
+					RTE_ETH_QUEUE_STATE_STARTED;
+			}
+		}
+
 		configure_rxtx_dump_callbacks(verbose_level);
 		if (clear_ptypes) {
 			diag = rte_eth_dev_set_ptypes(pi, RTE_PTYPE_UNKNOWN,
-- 
2.25.1

RE: [PATCH] app/testpmd: fix secondary process cannot dump packet

[Zhang, Yuying]

Hi Peng,

> -----Original Message-----
> From: Zhang, Peng1X <peng1x.zhang@intel.com>
> Sent: Friday, June 24, 2022 2:15 AM
> To: dev@dpdk.org
> Cc: Singh, Aman Deep <aman.deep.singh@intel.com>; Zhang, Yuying
> <yuying.zhang@intel.com>; Zhang, Peng1X <peng1x.zhang@intel.com>;
> stable@dpdk.org
> Subject: [PATCH] app/testpmd: fix secondary process cannot dump packet
> 
> From: Peng Zhang <peng1x.zhang@intel.com>
> 
> The origin design is whether testpmd is primary or not, if state of receive
> queue is stop, then packets will not be dumped for show.
> While to secondary process, receive queue will not be set up, and state will
> still be stop even if testpmd is started. So packets of stated secondary
> process cannot be dumped for show.

Current description is confusing. Please refine the commit log to define the issue clearly.

> 
> The current design is to secondary process state of queue will be set to start
> after testpmd is started. Then packets of started secondary process can be
> dumped for show.
> 
> Fixes: a550baf24af9 ("app/testpmd: support multi-process")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Peng Zhang <peng1x.zhang@intel.com>
> ---
>  app/test-pmd/testpmd.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> 205d98ee3d..93ba7e7c9b 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -3007,6 +3007,18 @@ start_port(portid_t pid)
>  			if (setup_hairpin_queues(pi, p_pi, cnt_pi) != 0)
>  				return -1;
>  		}
> +
> +		if (port->need_reconfig_queues > 0 && !is_proc_primary()) {
> +			struct rte_eth_rxconf *rx_conf;
> +			for (qi = 0; qi < nb_rxq; qi++) {
> +				rx_conf = &(port->rxq[qi].conf);
> +				ports[pi].rxq[qi].state =
> +					rx_conf->rx_deferred_start ?
> +					RTE_ETH_QUEUE_STATE_STOPPED :
> +					RTE_ETH_QUEUE_STATE_STARTED;
> +			}
> +		}
> +
>  		configure_rxtx_dump_callbacks(verbose_level);
>  		if (clear_ptypes) {
>  			diag = rte_eth_dev_set_ptypes(pi,
> RTE_PTYPE_UNKNOWN,
> --
> 2.25.1

Re: [PATCH] app/testpmd: fix secondary process cannot dump packet

[lihuisong (C)]

在 2022/6/23 20:10, Andrew Rybchenko 写道:
> On 6/23/22 21:15, peng1x.zhang@intel.com wrote:
>> From: Peng Zhang <peng1x.zhang@intel.com>
>>
>> The origin design is whether testpmd is primary or not, if state of
>> receive queue is stop, then packets will not be dumped for show.
>> While to secondary process, receive queue will not be set up, and state
>> will still be stop even if testpmd is started. So packets of stated
>> secondary process cannot be dumped for show.
>>
>> The current design is to secondary process state of queue will be set
>> to start after testpmd is started. Then packets of started secondary
>> process can be dumped for show.
>>
>> Fixes: a550baf24af9 ("app/testpmd: support multi-process")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Peng Zhang <peng1x.zhang@intel.com>
>> ---
>>   app/test-pmd/testpmd.c | 12 ++++++++++++
>>   1 file changed, 12 insertions(+)
>>
>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
>> index 205d98ee3d..93ba7e7c9b 100644
>> --- a/app/test-pmd/testpmd.c
>> +++ b/app/test-pmd/testpmd.c
>> @@ -3007,6 +3007,18 @@ start_port(portid_t pid)
>>               if (setup_hairpin_queues(pi, p_pi, cnt_pi) != 0)
>>                   return -1;
>>           }
>> +
>> +        if (port->need_reconfig_queues > 0 && !is_proc_primary()) {
>> +            struct rte_eth_rxconf *rx_conf;
>> +            for (qi = 0; qi < nb_rxq; qi++) {
>> +                rx_conf = &(port->rxq[qi].conf);
>> +                ports[pi].rxq[qi].state =
>> +                    rx_conf->rx_deferred_start ?
>> +                    RTE_ETH_QUEUE_STATE_STOPPED :
>> +                    RTE_ETH_QUEUE_STATE_STARTED;
>
> I'm not sure why it is correct to assume that deferred queue is
> not yet started.
+1.

We should also consider whether the queue state can be changed in secondary.
The 'rx_conf->rx_deferred_start' is the data in secondary.
Why not use 'dev->data->rx_queue_state[]'.

In fact, the issue you memtioned was introduced the following patch:
Fixes: 3c4426db54fc ("app/testpmd: do not poll stopped queues")

The root cause of this issue is that the default value of Rx/Tx queue state
maintained by testpmd is 'RTE_ETH_QUEUE_STATE_STOPPED'. As a result,
secondary doesn't start polling thread to receive packets when start packet
forwarding. And now, secondary cannot receive and send any packets.

Could you fix it together?
>
>> +            }
>> +        }
>> +
>>           configure_rxtx_dump_callbacks(verbose_level);
>>           if (clear_ptypes) {
>>               diag = rte_eth_dev_set_ptypes(pi, RTE_PTYPE_UNKNOWN,
>
> .

RE: [PATCH] app/testpmd: fix secondary process cannot dump packet

[Zhang, Yuying]

Hi,

> -----Original Message-----
> From: Zhang, Peng1X <peng1x.zhang@intel.com>
> Sent: Friday, June 24, 2022 2:15 AM
> To: dev@dpdk.org
> Cc: Singh, Aman Deep <aman.deep.singh@intel.com>; Zhang, Yuying
> <yuying.zhang@intel.com>; Zhang, Peng1X <peng1x.zhang@intel.com>;
> stable@dpdk.org
> Subject: [PATCH] app/testpmd: fix secondary process cannot dump packet
> 
> From: Peng Zhang <peng1x.zhang@intel.com>
> 
> The origin design is whether testpmd is primary or not, if state of receive queue
> is stop, then packets will not be dumped for show.
> While to secondary process, receive queue will not be set up, and state will still
> be stop even if testpmd is started. So packets of stated secondary process
> cannot be dumped for show.
> 
> The current design is to secondary process state of queue will be set to start
> after testpmd is started. Then packets of started secondary process can be
> dumped for show.
> 
> Fixes: a550baf24af9 ("app/testpmd: support multi-process")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Peng Zhang <peng1x.zhang@intel.com>

Acked-by: Yuying Zhang <yuying.zhang@intel.com>

> ---
>  app/test-pmd/testpmd.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> 205d98ee3d..93ba7e7c9b 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -3007,6 +3007,18 @@ start_port(portid_t pid)
>  			if (setup_hairpin_queues(pi, p_pi, cnt_pi) != 0)
>  				return -1;
>  		}
> +
> +		if (port->need_reconfig_queues > 0 && !is_proc_primary()) {
> +			struct rte_eth_rxconf *rx_conf;
> +			for (qi = 0; qi < nb_rxq; qi++) {
> +				rx_conf = &(port->rxq[qi].conf);
> +				ports[pi].rxq[qi].state =
> +					rx_conf->rx_deferred_start ?
> +					RTE_ETH_QUEUE_STATE_STOPPED :
> +					RTE_ETH_QUEUE_STATE_STARTED;
> +			}
> +		}
> +
>  		configure_rxtx_dump_callbacks(verbose_level);
>  		if (clear_ptypes) {
>  			diag = rte_eth_dev_set_ptypes(pi,
> RTE_PTYPE_UNKNOWN,
> --
> 2.25.1

RE: [PATCH] app/testpmd: fix secondary process cannot dump packet

[Zhang, Peng1X]

Hi,
In fact, the patch is aim to fix this issue that secondary process cannot dump packet after start testpmd. 
This issue is induced by commit id is 3c4426db54fc ("app/testpmd: do not poll stopped queues"). After 
secondary process start, the default value of Rx/Tx queue state maintained by testpmd is 
'RTE_ETH_QUEUE_STATE_STOPPED', the 'fsm[sm_id]->disabled' flag will set true according to queues 
state, then packet cannot forward and dump. 

The reason why not use 'dev->data->rx_queue_state' is whether queue state is start or stop in primary 
process depend on rx_conf->rx_deferred_start after start testpmd. And after having started testpmd, 
queue state can be controlled by command for example 'port x rxq x start'. 
Should we align with the same behavior of queues state for primary and secondary process after start testpmd?   

> -----Original Message-----
> From: lihuisong (C) <lihuisong@huawei.com>
> Sent: Wednesday, June 29, 2022 10:55 AM
> To: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>; Zhang, Peng1X
> <peng1x.zhang@intel.com>; dev@dpdk.org
> Cc: Singh, Aman Deep <aman.deep.singh@intel.com>; Zhang, Yuying
> <yuying.zhang@intel.com>; stable@dpdk.org
> Subject: Re: [PATCH] app/testpmd: fix secondary process cannot dump packet
> 
> 
> 在 2022/6/23 20:10, Andrew Rybchenko 写道:
> > On 6/23/22 21:15, peng1x.zhang@intel.com wrote:
> >> From: Peng Zhang <peng1x.zhang@intel.com>
> >>
> >> The origin design is whether testpmd is primary or not, if state of
> >> receive queue is stop, then packets will not be dumped for show.
> >> While to secondary process, receive queue will not be set up, and
> >> state will still be stop even if testpmd is started. So packets of
> >> stated secondary process cannot be dumped for show.
> >>
> >> The current design is to secondary process state of queue will be set
> >> to start after testpmd is started. Then packets of started secondary
> >> process can be dumped for show.
> >>
> >> Fixes: a550baf24af9 ("app/testpmd: support multi-process")
> >> Cc: stable@dpdk.org
> >>
> >> Signed-off-by: Peng Zhang <peng1x.zhang@intel.com>
> >> ---
> >>   app/test-pmd/testpmd.c | 12 ++++++++++++
> >>   1 file changed, 12 insertions(+)
> >>
> >> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> >> 205d98ee3d..93ba7e7c9b 100644
> >> --- a/app/test-pmd/testpmd.c
> >> +++ b/app/test-pmd/testpmd.c
> >> @@ -3007,6 +3007,18 @@ start_port(portid_t pid)
> >>               if (setup_hairpin_queues(pi, p_pi, cnt_pi) != 0)
> >>                   return -1;
> >>           }
> >> +
> >> +        if (port->need_reconfig_queues > 0 && !is_proc_primary()) {
> >> +            struct rte_eth_rxconf *rx_conf;
> >> +            for (qi = 0; qi < nb_rxq; qi++) {
> >> +                rx_conf = &(port->rxq[qi].conf);
> >> +                ports[pi].rxq[qi].state =
> >> +                    rx_conf->rx_deferred_start ?
> >> +                    RTE_ETH_QUEUE_STATE_STOPPED :
> >> +                    RTE_ETH_QUEUE_STATE_STARTED;
> >
> > I'm not sure why it is correct to assume that deferred queue is not
> > yet started.
> +1.
> 
> We should also consider whether the queue state can be changed in secondary.
> The 'rx_conf->rx_deferred_start' is the data in secondary.
> Why not use 'dev->data->rx_queue_state[]'.
> 
> In fact, the issue you memtioned was introduced the following patch:
> Fixes: 3c4426db54fc ("app/testpmd: do not poll stopped queues")
> 
> The root cause of this issue is that the default value of Rx/Tx queue state
> maintained by testpmd is 'RTE_ETH_QUEUE_STATE_STOPPED'. As a result,
> secondary doesn't start polling thread to receive packets when start packet
> forwarding. And now, secondary cannot receive and send any packets.
> 
> Could you fix it together?
> >
> >> +            }
> >> +        }
> >> +
> >>           configure_rxtx_dump_callbacks(verbose_level);
> >>           if (clear_ptypes) {
> >>               diag = rte_eth_dev_set_ptypes(pi, RTE_PTYPE_UNKNOWN,
> >
> > .

Re: [PATCH] app/testpmd: fix secondary process cannot dump packet

[lihuisong (C)]

Hi Peng1X,

在 2022/7/1 19:36, Zhang, Peng1X 写道:
> Hi,
> In fact, the patch is aim to fix this issue that secondary process cannot dump packet after start testpmd.
> This issue is induced by commit id is 3c4426db54fc ("app/testpmd: do not poll stopped queues"). After
> secondary process start, the default value of Rx/Tx queue state maintained by testpmd is
> 'RTE_ETH_QUEUE_STATE_STOPPED', the 'fsm[sm_id]->disabled' flag will set true according to queues
> state, then packet cannot forward and dump.
I get your meaning.
However, failing to dump packet isn't the first exception,
and the first one is that testpmd doesn't call
'struct fwd_engine::packet_fwd()' to receive or send packet.
So, I think you should describe and resolve this problem
from this point. This patch cannot completely resolve this
problem. The Tx queue state should also be added here.
>
> The reason why not use 'dev->data->rx_queue_state' is whether queue state is start or stop in primary
> process depend on rx_conf->rx_deferred_start after start testpmd. And after having started testpmd,
> queue state can be controlled by command for example 'port x rxq x start'.
> Should we align with the same behavior of queues state for primary and secondary process after start testpmd?
If primary process stops a queue, but secondary doesn't know.
we have to simplify this queue state problem like you momentioned
if we don't have a good idea.

@Andrew, what do you think?

Thanks,

Huisong

>
>> -----Original Message-----
>> From: lihuisong (C) <lihuisong@huawei.com>
>> Sent: Wednesday, June 29, 2022 10:55 AM
>> To: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>; Zhang, Peng1X
>> <peng1x.zhang@intel.com>; dev@dpdk.org
>> Cc: Singh, Aman Deep <aman.deep.singh@intel.com>; Zhang, Yuying
>> <yuying.zhang@intel.com>; stable@dpdk.org
>> Subject: Re: [PATCH] app/testpmd: fix secondary process cannot dump packet
>>
>>
>> 在 2022/6/23 20:10, Andrew Rybchenko 写道:
>>> On 6/23/22 21:15, peng1x.zhang@intel.com wrote:
>>>> From: Peng Zhang <peng1x.zhang@intel.com>
>>>>
>>>> The origin design is whether testpmd is primary or not, if state of
>>>> receive queue is stop, then packets will not be dumped for show.
>>>> While to secondary process, receive queue will not be set up, and
>>>> state will still be stop even if testpmd is started. So packets of
>>>> stated secondary process cannot be dumped for show.
>>>>
>>>> The current design is to secondary process state of queue will be set
>>>> to start after testpmd is started. Then packets of started secondary
>>>> process can be dumped for show.
>>>>
>>>> Fixes: a550baf24af9 ("app/testpmd: support multi-process")
>>>> Cc: stable@dpdk.org
>>>>
>>>> Signed-off-by: Peng Zhang <peng1x.zhang@intel.com>
>>>> ---
>>>>    app/test-pmd/testpmd.c | 12 ++++++++++++
>>>>    1 file changed, 12 insertions(+)
>>>>
>>>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
>>>> 205d98ee3d..93ba7e7c9b 100644
>>>> --- a/app/test-pmd/testpmd.c
>>>> +++ b/app/test-pmd/testpmd.c
>>>> @@ -3007,6 +3007,18 @@ start_port(portid_t pid)
>>>>                if (setup_hairpin_queues(pi, p_pi, cnt_pi) != 0)
>>>>                    return -1;
>>>>            }
>>>> +
>>>> +        if (port->need_reconfig_queues > 0 && !is_proc_primary()) {
>>>> +            struct rte_eth_rxconf *rx_conf;
>>>> +            for (qi = 0; qi < nb_rxq; qi++) {
>>>> +                rx_conf = &(port->rxq[qi].conf);
>>>> +                ports[pi].rxq[qi].state =
>>>> +                    rx_conf->rx_deferred_start ?
>>>> +                    RTE_ETH_QUEUE_STATE_STOPPED :
>>>> +                    RTE_ETH_QUEUE_STATE_STARTED;
>>> I'm not sure why it is correct to assume that deferred queue is not
>>> yet started.
>> +1.
>>
>> We should also consider whether the queue state can be changed in secondary.
>> The 'rx_conf->rx_deferred_start' is the data in secondary.
>> Why not use 'dev->data->rx_queue_state[]'.
>>
>> In fact, the issue you memtioned was introduced the following patch:
>> Fixes: 3c4426db54fc ("app/testpmd: do not poll stopped queues")
>>
>> The root cause of this issue is that the default value of Rx/Tx queue state
>> maintained by testpmd is 'RTE_ETH_QUEUE_STATE_STOPPED'. As a result,
>> secondary doesn't start polling thread to receive packets when start packet
>> forwarding. And now, secondary cannot receive and send any packets.
>>
>> Could you fix it together?
>>>> +            }
>>>> +        }
>>>> +
>>>>            configure_rxtx_dump_callbacks(verbose_level);
>>>>            if (clear_ptypes) {
>>>>                diag = rte_eth_dev_set_ptypes(pi, RTE_PTYPE_UNKNOWN,
>>> .

Re: [PATCH] app/testpmd: fix secondary process cannot dump packet

[Dmitry Kozlyuk]

[-- Attachment #1: Type: text/plain, Size: 1028 bytes --]

On Mon, Jul 4, 2022 at 5:37 AM lihuisong (C) <lihuisong@huawei.com> wrote:

> [...]
> 在 2022/7/1 19:36, Zhang, Peng1X 写道:
> [...]
> > The reason why not use 'dev->data->rx_queue_state' is whether queue
> state is start or stop in primary
> > process depend on rx_conf->rx_deferred_start after start testpmd. And
> after having started testpmd,
> > queue state can be controlled by command for example 'port x rxq x
> start'.
> > Should we align with the same behavior of queues state for primary and
> secondary process after start testpmd?
> If primary process stops a queue, but secondary doesn't know.
> we have to simplify this queue state problem like you momentioned
> if we don't have a good idea.
>

AFAIU, dev->data->rx_queue_state should be aligned with
rx_conf->rx_deferred_start.
The reason why testpmd manages the queue state itself in the offending patch
is that not all PMDs implement `rte_eth_rx/tx_queue_info_get()`
and ethdev won't return `dev->data->rx_queue_state` in this case.

[-- Attachment #2: Type: text/html, Size: 1443 bytes --]

RE: [PATCH] app/testpmd: fix secondary process cannot dump packet

[Zhang, Peng1X]

Hi Song,
Currently this patch is just fix the issue detected for rx queue on secondary process.
Later patch for tx queue will be submit.

@Andrew, what's your opinion about the solution of this patch?

Thanks,
Peng

> -----Original Message-----
> From: lihuisong (C) <lihuisong@huawei.com>
> Sent: Monday, July 4, 2022 10:37 AM
> To: Zhang, Peng1X <peng1x.zhang@intel.com>; Andrew Rybchenko
> <andrew.rybchenko@oktetlabs.ru>; dev@dpdk.org
> Cc: Singh, Aman Deep <aman.deep.singh@intel.com>; Zhang, Yuying
> <yuying.zhang@intel.com>; stable@dpdk.org
> Subject: Re: [PATCH] app/testpmd: fix secondary process cannot dump packet
> 
> Hi Peng1X,
> 
> 在 2022/7/1 19:36, Zhang, Peng1X 写道:
> > Hi,
> > In fact, the patch is aim to fix this issue that secondary process cannot dump
> packet after start testpmd.
> > This issue is induced by commit id is 3c4426db54fc ("app/testpmd: do
> > not poll stopped queues"). After secondary process start, the default
> > value of Rx/Tx queue state maintained by testpmd is
> > 'RTE_ETH_QUEUE_STATE_STOPPED', the 'fsm[sm_id]->disabled' flag will set
> true according to queues state, then packet cannot forward and dump.
> I get your meaning.
> However, failing to dump packet isn't the first exception, and the first one is
> that testpmd doesn't call 'struct fwd_engine::packet_fwd()' to receive or send
> packet.
> So, I think you should describe and resolve this problem from this point. This
> patch cannot completely resolve this problem. The Tx queue state should also
> be added here.
> >
> > The reason why not use 'dev->data->rx_queue_state' is whether queue
> > state is start or stop in primary process depend on
> > rx_conf->rx_deferred_start after start testpmd. And after having started
> testpmd, queue state can be controlled by command for example 'port x rxq x
> start'.
> > Should we align with the same behavior of queues state for primary and
> secondary process after start testpmd?
> If primary process stops a queue, but secondary doesn't know.
> we have to simplify this queue state problem like you momentioned if we don't
> have a good idea.
> 
> @Andrew, what do you think?
> 
> Thanks,
> 
> Huisong
> 
> >
> >> -----Original Message-----
> >> From: lihuisong (C) <lihuisong@huawei.com>
> >> Sent: Wednesday, June 29, 2022 10:55 AM
> >> To: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>; Zhang, Peng1X
> >> <peng1x.zhang@intel.com>; dev@dpdk.org
> >> Cc: Singh, Aman Deep <aman.deep.singh@intel.com>; Zhang, Yuying
> >> <yuying.zhang@intel.com>; stable@dpdk.org
> >> Subject: Re: [PATCH] app/testpmd: fix secondary process cannot dump
> >> packet
> >>
> >>
> >> 在 2022/6/23 20:10, Andrew Rybchenko 写道:
> >>> On 6/23/22 21:15, peng1x.zhang@intel.com wrote:
> >>>> From: Peng Zhang <peng1x.zhang@intel.com>
> >>>>
> >>>> The origin design is whether testpmd is primary or not, if state of
> >>>> receive queue is stop, then packets will not be dumped for show.
> >>>> While to secondary process, receive queue will not be set up, and
> >>>> state will still be stop even if testpmd is started. So packets of
> >>>> stated secondary process cannot be dumped for show.
> >>>>
> >>>> The current design is to secondary process state of queue will be
> >>>> set to start after testpmd is started. Then packets of started
> >>>> secondary process can be dumped for show.
> >>>>
> >>>> Fixes: a550baf24af9 ("app/testpmd: support multi-process")
> >>>> Cc: stable@dpdk.org
> >>>>
> >>>> Signed-off-by: Peng Zhang <peng1x.zhang@intel.com>
> >>>> ---
> >>>>    app/test-pmd/testpmd.c | 12 ++++++++++++
> >>>>    1 file changed, 12 insertions(+)
> >>>>
> >>>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> >>>> 205d98ee3d..93ba7e7c9b 100644
> >>>> --- a/app/test-pmd/testpmd.c
> >>>> +++ b/app/test-pmd/testpmd.c
> >>>> @@ -3007,6 +3007,18 @@ start_port(portid_t pid)
> >>>>                if (setup_hairpin_queues(pi, p_pi, cnt_pi) != 0)
> >>>>                    return -1;
> >>>>            }
> >>>> +
> >>>> +        if (port->need_reconfig_queues > 0 && !is_proc_primary())
> >>>> +{
> >>>> +            struct rte_eth_rxconf *rx_conf;
> >>>> +            for (qi = 0; qi < nb_rxq; qi++) {
> >>>> +                rx_conf = &(port->rxq[qi].conf);
> >>>> +                ports[pi].rxq[qi].state =
> >>>> +                    rx_conf->rx_deferred_start ?
> >>>> +                    RTE_ETH_QUEUE_STATE_STOPPED :
> >>>> +                    RTE_ETH_QUEUE_STATE_STARTED;
> >>> I'm not sure why it is correct to assume that deferred queue is not
> >>> yet started.
> >> +1.
> >>
> >> We should also consider whether the queue state can be changed in
> secondary.
> >> The 'rx_conf->rx_deferred_start' is the data in secondary.
> >> Why not use 'dev->data->rx_queue_state[]'.
> >>
> >> In fact, the issue you memtioned was introduced the following patch:
> >> Fixes: 3c4426db54fc ("app/testpmd: do not poll stopped queues")
> >>
> >> The root cause of this issue is that the default value of Rx/Tx queue
> >> state maintained by testpmd is 'RTE_ETH_QUEUE_STATE_STOPPED'. As a
> >> result, secondary doesn't start polling thread to receive packets
> >> when start packet forwarding. And now, secondary cannot receive and send
> any packets.
> >>
> >> Could you fix it together?
> >>>> +            }
> >>>> +        }
> >>>> +
> >>>>            configure_rxtx_dump_callbacks(verbose_level);
> >>>>            if (clear_ptypes) {
> >>>>                diag = rte_eth_dev_set_ptypes(pi, RTE_PTYPE_UNKNOWN,
> >>> .

Re: [PATCH] app/testpmd: fix secondary process cannot dump packet

[lihuisong (C)]

在 2022/7/5 18:12, Zhang, Peng1X 写道:
> Hi Song,
> Currently this patch is just fix the issue detected for rx queue on secondary process.
> Later patch for tx queue will be submit.
Firstly, the Rx packet dump failure is not displayed in your commit log.

In addition, even if this patch is aimed to fix the Rx issue on 
secondary process,
it doesn't work well when if testpmd run in 'mac' or 'swap' forwarding 
mode, right?
Because it is also affected by the Tx queue state. Therefore, it is 
better to place
the modification of Rx and Tx queue state in the same patch to fix this 
issue.

@Peng1X, @Andrew and @Thomas
Currently, the secondary process cannot send and receive packets.
It is suggested that this problem should be solved quickly.
After all, 22.07 will be released soon.

> @Andrew, what's your opinion about the solution of this patch?
>
> Thanks,
> Peng
>
>> -----Original Message-----
>> From: lihuisong (C) <lihuisong@huawei.com>
>> Sent: Monday, July 4, 2022 10:37 AM
>> To: Zhang, Peng1X <peng1x.zhang@intel.com>; Andrew Rybchenko
>> <andrew.rybchenko@oktetlabs.ru>; dev@dpdk.org
>> Cc: Singh, Aman Deep <aman.deep.singh@intel.com>; Zhang, Yuying
>> <yuying.zhang@intel.com>; stable@dpdk.org
>> Subject: Re: [PATCH] app/testpmd: fix secondary process cannot dump packet
>>
>> Hi Peng1X,
>>
>> 在 2022/7/1 19:36, Zhang, Peng1X 写道:
>>> Hi,
>>> In fact, the patch is aim to fix this issue that secondary process cannot dump
>> packet after start testpmd.
>>> This issue is induced by commit id is 3c4426db54fc ("app/testpmd: do
>>> not poll stopped queues"). After secondary process start, the default
>>> value of Rx/Tx queue state maintained by testpmd is
>>> 'RTE_ETH_QUEUE_STATE_STOPPED', the 'fsm[sm_id]->disabled' flag will set
>> true according to queues state, then packet cannot forward and dump.
>> I get your meaning.
>> However, failing to dump packet isn't the first exception, and the first one is
>> that testpmd doesn't call 'struct fwd_engine::packet_fwd()' to receive or send
>> packet.
>> So, I think you should describe and resolve this problem from this point. This
>> patch cannot completely resolve this problem. The Tx queue state should also
>> be added here.
>>> The reason why not use 'dev->data->rx_queue_state' is whether queue
>>> state is start or stop in primary process depend on
>>> rx_conf->rx_deferred_start after start testpmd. And after having started
>> testpmd, queue state can be controlled by command for example 'port x rxq x
>> start'.
>>> Should we align with the same behavior of queues state for primary and
>> secondary process after start testpmd?
>> If primary process stops a queue, but secondary doesn't know.
>> we have to simplify this queue state problem like you momentioned if we don't
>> have a good idea.
>>
>> @Andrew, what do you think?
>>
>> Thanks,
>>
>> Huisong
>>
>>>> -----Original Message-----
>>>> From: lihuisong (C) <lihuisong@huawei.com>
>>>> Sent: Wednesday, June 29, 2022 10:55 AM
>>>> To: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>; Zhang, Peng1X
>>>> <peng1x.zhang@intel.com>; dev@dpdk.org
>>>> Cc: Singh, Aman Deep <aman.deep.singh@intel.com>; Zhang, Yuying
>>>> <yuying.zhang@intel.com>; stable@dpdk.org
>>>> Subject: Re: [PATCH] app/testpmd: fix secondary process cannot dump
>>>> packet
>>>>
>>>>
>>>> 在 2022/6/23 20:10, Andrew Rybchenko 写道:
>>>>> On 6/23/22 21:15, peng1x.zhang@intel.com wrote:
>>>>>> From: Peng Zhang <peng1x.zhang@intel.com>
>>>>>>
>>>>>> The origin design is whether testpmd is primary or not, if state of
>>>>>> receive queue is stop, then packets will not be dumped for show.
>>>>>> While to secondary process, receive queue will not be set up, and
>>>>>> state will still be stop even if testpmd is started. So packets of
>>>>>> stated secondary process cannot be dumped for show.
>>>>>>
>>>>>> The current design is to secondary process state of queue will be
>>>>>> set to start after testpmd is started. Then packets of started
>>>>>> secondary process can be dumped for show.
>>>>>>
>>>>>> Fixes: a550baf24af9 ("app/testpmd: support multi-process")
>>>>>> Cc: stable@dpdk.org
>>>>>>
>>>>>> Signed-off-by: Peng Zhang <peng1x.zhang@intel.com>
>>>>>> ---
>>>>>>     app/test-pmd/testpmd.c | 12 ++++++++++++
>>>>>>     1 file changed, 12 insertions(+)
>>>>>>
>>>>>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
>>>>>> 205d98ee3d..93ba7e7c9b 100644
>>>>>> --- a/app/test-pmd/testpmd.c
>>>>>> +++ b/app/test-pmd/testpmd.c
>>>>>> @@ -3007,6 +3007,18 @@ start_port(portid_t pid)
>>>>>>                 if (setup_hairpin_queues(pi, p_pi, cnt_pi) != 0)
>>>>>>                     return -1;
>>>>>>             }
>>>>>> +
>>>>>> +        if (port->need_reconfig_queues > 0 && !is_proc_primary())
>>>>>> +{
>>>>>> +            struct rte_eth_rxconf *rx_conf;
>>>>>> +            for (qi = 0; qi < nb_rxq; qi++) {
>>>>>> +                rx_conf = &(port->rxq[qi].conf);
>>>>>> +                ports[pi].rxq[qi].state =
>>>>>> +                    rx_conf->rx_deferred_start ?
>>>>>> +                    RTE_ETH_QUEUE_STATE_STOPPED :
>>>>>> +                    RTE_ETH_QUEUE_STATE_STARTED;
>>>>> I'm not sure why it is correct to assume that deferred queue is not
>>>>> yet started.
>>>> +1.
>>>>
>>>> We should also consider whether the queue state can be changed in
>> secondary.
>>>> The 'rx_conf->rx_deferred_start' is the data in secondary.
>>>> Why not use 'dev->data->rx_queue_state[]'.
>>>>
>>>> In fact, the issue you memtioned was introduced the following patch:
>>>> Fixes: 3c4426db54fc ("app/testpmd: do not poll stopped queues")
>>>>
>>>> The root cause of this issue is that the default value of Rx/Tx queue
>>>> state maintained by testpmd is 'RTE_ETH_QUEUE_STATE_STOPPED'. As a
>>>> result, secondary doesn't start polling thread to receive packets
>>>> when start packet forwarding. And now, secondary cannot receive and send
>> any packets.
>>>> Could you fix it together?
>>>>>> +            }
>>>>>> +        }
>>>>>> +
>>>>>>             configure_rxtx_dump_callbacks(verbose_level);
>>>>>>             if (clear_ptypes) {
>>>>>>                 diag = rte_eth_dev_set_ptypes(pi, RTE_PTYPE_UNKNOWN,
>>>>> .

Re: [PATCH] app/testpmd: fix secondary process cannot dump packet

[Andrew Rybchenko]

On 7/6/22 05:00, lihuisong (C) wrote:
> 
> 在 2022/7/5 18:12, Zhang, Peng1X 写道:
>> Hi Song,
>> Currently this patch is just fix the issue detected for rx queue on 
>> secondary process.
>> Later patch for tx queue will be submit.
> Firstly, the Rx packet dump failure is not displayed in your commit log.
> 
> In addition, even if this patch is aimed to fix the Rx issue on 
> secondary process,
> it doesn't work well when if testpmd run in 'mac' or 'swap' forwarding 
> mode, right?
> Because it is also affected by the Tx queue state. Therefore, it is 
> better to place
> the modification of Rx and Tx queue state in the same patch to fix this 
> issue.

If the problem is generic, it is better to find generic solution
rather than concentrate on one aspect of the problem.

> @Peng1X, @Andrew and @Thomas
> Currently, the secondary process cannot send and receive packets.
> It is suggested that this problem should be solved quickly.
> After all, 22.07 will be released soon.
> 
>> @Andrew, what's your opinion about the solution of this patch?

IMHO the patch is simply incorrect since it uses
rx_deferred_start incorrectly. The attribute does not say if
an Rx queue is started or stopped right now.

>>
>> Thanks,
>> Peng
>>
>>> -----Original Message-----
>>> From: lihuisong (C) <lihuisong@huawei.com>
>>> Sent: Monday, July 4, 2022 10:37 AM
>>> To: Zhang, Peng1X <peng1x.zhang@intel.com>; Andrew Rybchenko
>>> <andrew.rybchenko@oktetlabs.ru>; dev@dpdk.org
>>> Cc: Singh, Aman Deep <aman.deep.singh@intel.com>; Zhang, Yuying
>>> <yuying.zhang@intel.com>; stable@dpdk.org
>>> Subject: Re: [PATCH] app/testpmd: fix secondary process cannot dump 
>>> packet
>>>
>>> Hi Peng1X,
>>>
>>> 在 2022/7/1 19:36, Zhang, Peng1X 写道:
>>>> Hi,
>>>> In fact, the patch is aim to fix this issue that secondary process 
>>>> cannot dump
>>> packet after start testpmd.
>>>> This issue is induced by commit id is 3c4426db54fc ("app/testpmd: do
>>>> not poll stopped queues"). After secondary process start, the default
>>>> value of Rx/Tx queue state maintained by testpmd is
>>>> 'RTE_ETH_QUEUE_STATE_STOPPED', the 'fsm[sm_id]->disabled' flag will set
>>> true according to queues state, then packet cannot forward and dump.
>>> I get your meaning.
>>> However, failing to dump packet isn't the first exception, and the 
>>> first one is
>>> that testpmd doesn't call 'struct fwd_engine::packet_fwd()' to 
>>> receive or send
>>> packet.
>>> So, I think you should describe and resolve this problem from this 
>>> point. This
>>> patch cannot completely resolve this problem. The Tx queue state 
>>> should also
>>> be added here.
>>>> The reason why not use 'dev->data->rx_queue_state' is whether queue
>>>> state is start or stop in primary process depend on
>>>> rx_conf->rx_deferred_start after start testpmd. And after having 
>>>> started
>>> testpmd, queue state can be controlled by command for example 'port x 
>>> rxq x
>>> start'.
>>>> Should we align with the same behavior of queues state for primary and
>>> secondary process after start testpmd?
>>> If primary process stops a queue, but secondary doesn't know.
>>> we have to simplify this queue state problem like you momentioned if 
>>> we don't
>>> have a good idea.
>>>
>>> @Andrew, what do you think?

I'm sorry to say, but multi-process support in testpmd is not well-
thought. So, it is hard to suggest a good solution. I think that trying
to mirror state in secondary testpmd process is a bad idea. It could
become out-of-sync by definition. So, I'd either share testpmd state or
use state from ethdev (which is located in shared memory).
However, it sounds like a long story and non-trivial changes which are
too late at the current release cycle state. See below.

>>>
>>> Thanks,
>>>
>>> Huisong
>>>
>>>>> -----Original Message-----
>>>>> From: lihuisong (C) <lihuisong@huawei.com>
>>>>> Sent: Wednesday, June 29, 2022 10:55 AM
>>>>> To: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>; Zhang, Peng1X
>>>>> <peng1x.zhang@intel.com>; dev@dpdk.org
>>>>> Cc: Singh, Aman Deep <aman.deep.singh@intel.com>; Zhang, Yuying
>>>>> <yuying.zhang@intel.com>; stable@dpdk.org
>>>>> Subject: Re: [PATCH] app/testpmd: fix secondary process cannot dump
>>>>> packet
>>>>>
>>>>>
>>>>> 在 2022/6/23 20:10, Andrew Rybchenko 写道:
>>>>>> On 6/23/22 21:15, peng1x.zhang@intel.com wrote:
>>>>>>> From: Peng Zhang <peng1x.zhang@intel.com>
>>>>>>>
>>>>>>> The origin design is whether testpmd is primary or not, if state of
>>>>>>> receive queue is stop, then packets will not be dumped for show.
>>>>>>> While to secondary process, receive queue will not be set up, and
>>>>>>> state will still be stop even if testpmd is started. So packets of
>>>>>>> stated secondary process cannot be dumped for show.
>>>>>>>
>>>>>>> The current design is to secondary process state of queue will be
>>>>>>> set to start after testpmd is started. Then packets of started
>>>>>>> secondary process can be dumped for show.
>>>>>>>
>>>>>>> Fixes: a550baf24af9 ("app/testpmd: support multi-process")
>>>>>>> Cc: stable@dpdk.org
>>>>>>>
>>>>>>> Signed-off-by: Peng Zhang <peng1x.zhang@intel.com>
>>>>>>> ---
>>>>>>>     app/test-pmd/testpmd.c | 12 ++++++++++++
>>>>>>>     1 file changed, 12 insertions(+)
>>>>>>>
>>>>>>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
>>>>>>> 205d98ee3d..93ba7e7c9b 100644
>>>>>>> --- a/app/test-pmd/testpmd.c
>>>>>>> +++ b/app/test-pmd/testpmd.c
>>>>>>> @@ -3007,6 +3007,18 @@ start_port(portid_t pid)
>>>>>>>                 if (setup_hairpin_queues(pi, p_pi, cnt_pi) != 0)
>>>>>>>                     return -1;
>>>>>>>             }
>>>>>>> +
>>>>>>> +        if (port->need_reconfig_queues > 0 && !is_proc_primary())
>>>>>>> +{
>>>>>>> +            struct rte_eth_rxconf *rx_conf;
>>>>>>> +            for (qi = 0; qi < nb_rxq; qi++) {
>>>>>>> +                rx_conf = &(port->rxq[qi].conf);
>>>>>>> +                ports[pi].rxq[qi].state =
>>>>>>> +                    rx_conf->rx_deferred_start ?
>>>>>>> +                    RTE_ETH_QUEUE_STATE_STOPPED :
>>>>>>> +                    RTE_ETH_QUEUE_STATE_STARTED;
>>>>>> I'm not sure why it is correct to assume that deferred queue is not
>>>>>> yet started.
>>>>> +1.
>>>>>
>>>>> We should also consider whether the queue state can be changed in secondary.
>>>>> The 'rx_conf->rx_deferred_start' is the data in secondary.
>>>>> Why not use 'dev->data->rx_queue_state[]'.
>>>>>
>>>>> In fact, the issue you memtioned was introduced the following patch:
>>>>> Fixes: 3c4426db54fc ("app/testpmd: do not poll stopped queues")
>>>>>
>>>>> The root cause of this issue is that the default value of Rx/Tx queue
>>>>> state maintained by testpmd is 'RTE_ETH_QUEUE_STATE_STOPPED'. As a
>>>>> result, secondary doesn't start polling thread to receive packets
>>>>> when start packet forwarding. And now, secondary cannot receive and 
>>>>> send any packets.
>>>>> Could you fix it together?

As a simple fix which should work in assumption that everything is
started before secondary process start-up, I'd just set state to
RTE_ETH_QUEUE_STATE_STARTED.

>>>>>>> +            }
>>>>>>> +        }
>>>>>>> +
>>>>>>>             configure_rxtx_dump_callbacks(verbose_level);
>>>>>>>             if (clear_ptypes) {
>>>>>>>                 diag = rte_eth_dev_set_ptypes(pi, RTE_PTYPE_UNKNOWN,
>>>>>> .

[PATCH v2] app/testpmd: fix incorrect queues state of secondary process

[peng1x.zhang]

From: Peng Zhang <peng1x.zhang@intel.com>

Primary process could set up queues state correctly when starting port,
but under multi-process scenario, "stream_init" function would get wrong
queues state for secondary process.

This commit is to get queues state from ethdev which is located in
shared memory.

Fixes: 3c4426db54fc ("app/testpmd: do not poll stopped queues")
Cc: stable@dpdk.org

Signed-off-by: Peng Zhang <peng1x.zhang@intel.com>
---
 app/test-pmd/testpmd.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index addcbcac85..70f907d96b 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -75,6 +75,8 @@
 
 #include "testpmd.h"
 
+#include <ethdev_driver.h>
+
 #ifndef MAP_HUGETLB
 /* FreeBSD may not have MAP_HUGETLB (in fact, it probably doesn't) */
 #define HUGE_FLAG (0x40000)
@@ -2402,9 +2404,23 @@ start_packet_forwarding(int with_tx_first)
 	if (!pkt_fwd_shared_rxq_check())
 		return;
 
-	if (stream_init != NULL)
-		for (i = 0; i < cur_fwd_config.nb_fwd_streams; i++)
+	if (stream_init != NULL) {
+		for (i = 0; i < cur_fwd_config.nb_fwd_streams; i++) {
+			if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
+				struct fwd_stream *fs = fwd_streams[i];
+				struct rte_eth_dev_data *dev_rx_data, *dev_tx_data;
+
+				dev_rx_data = (&rte_eth_devices[fs->rx_port])->data;
+				dev_tx_data = (&rte_eth_devices[fs->tx_port])->data;
+
+				uint8_t rx_state = dev_rx_data->rx_queue_state[fs->rx_port];
+				ports[fs->rx_port].rxq[fs->rx_queue].state = rx_state;
+				uint8_t tx_state = dev_tx_data->tx_queue_state[fs->tx_port];
+				ports[fs->tx_port].txq[fs->tx_queue].state = tx_state;
+			}
 			stream_init(fwd_streams[i]);
+		}
+	}
 
 	port_fwd_begin = cur_fwd_config.fwd_eng->port_fwd_begin;
 	if (port_fwd_begin != NULL) {
-- 
2.25.1

Re: [PATCH v2] app/testpmd: fix incorrect queues state of secondary process

[Singh, Aman Deep]

Hi Peng,

On 8/19/2022 3:39 PM, peng1x.zhang@intel.com wrote:
> From: Peng Zhang <peng1x.zhang@intel.com>
>
> Primary process could set up queues state correctly when starting port,
> but under multi-process scenario, "stream_init" function would get wrong
> queues state for secondary process.
>
> This commit is to get queues state from ethdev which is located in
> shared memory.
>
> Fixes: 3c4426db54fc ("app/testpmd: do not poll stopped queues")
> Cc: stable@dpdk.org
>
> Signed-off-by: Peng Zhang <peng1x.zhang@intel.com>
> ---
>   app/test-pmd/testpmd.c | 20 ++++++++++++++++++--
>   1 file changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> index addcbcac85..70f907d96b 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -75,6 +75,8 @@
>   
>   #include "testpmd.h"
>   
> +#include <ethdev_driver.h>
> +
>   #ifndef MAP_HUGETLB
>   /* FreeBSD may not have MAP_HUGETLB (in fact, it probably doesn't) */
>   #define HUGE_FLAG (0x40000)
> @@ -2402,9 +2404,23 @@ start_packet_forwarding(int with_tx_first)
>   	if (!pkt_fwd_shared_rxq_check())
>   		return;
>   
> -	if (stream_init != NULL)
> -		for (i = 0; i < cur_fwd_config.nb_fwd_streams; i++)
> +	if (stream_init != NULL) {
> +		for (i = 0; i < cur_fwd_config.nb_fwd_streams; i++) {
> +			if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
> +				struct fwd_stream *fs = fwd_streams[i];
> +				struct rte_eth_dev_data *dev_rx_data, *dev_tx_data;
> +
> +				dev_rx_data = (&rte_eth_devices[fs->rx_port])->data;
> +				dev_tx_data = (&rte_eth_devices[fs->tx_port])->data;
> +
> +				uint8_t rx_state = dev_rx_data->rx_queue_state[fs->rx_port];

To get the queue state, the array rx_queue_state[] should be indexed by the queue number.
Using fs->rx_port may not give desired queue's state.

> +				ports[fs->rx_port].rxq[fs->rx_queue].state = rx_state;
> +				uint8_t tx_state = dev_tx_data->tx_queue_state[fs->tx_port];

Same as rx queue above. We might need to root cause the issue further.

> +				ports[fs->tx_port].txq[fs->tx_queue].state = tx_state;
> +			}
>   			stream_init(fwd_streams[i]);
> +		}
> +	}
>   
>   	port_fwd_begin = cur_fwd_config.fwd_eng->port_fwd_begin;
>   	if (port_fwd_begin != NULL) {

RE: [PATCH v2] app/testpmd: fix incorrect queues state of secondary process

[Zhang, Peng1X]

Hi Aman Deep,

Thanks for your comment, I will follow it. And root cause is to secondary process, it cannot get queue state directly, but from share memory. So when starting port, secondary process cannot get correct queue state even if queue has been started.

> -----Original Message-----
> From: Singh, Aman Deep <aman.deep.singh@intel.com>
> Sent: Thursday, August 25, 2022 2:22 AM
> To: Zhang, Peng1X <peng1x.zhang@intel.com>; dev@dpdk.org
> Cc: Zhang, Yuying <yuying.zhang@intel.com>; stable@dpdk.org
> Subject: Re: [PATCH v2] app/testpmd: fix incorrect queues state of secondary
> process
> 
> Hi Peng,
> 
> On 8/19/2022 3:39 PM, peng1x.zhang@intel.com wrote:
> > From: Peng Zhang <peng1x.zhang@intel.com>
> >
> > Primary process could set up queues state correctly when starting
> > port, but under multi-process scenario, "stream_init" function would
> > get wrong queues state for secondary process.
> >
> > This commit is to get queues state from ethdev which is located in
> > shared memory.
> >
> > Fixes: 3c4426db54fc ("app/testpmd: do not poll stopped queues")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Peng Zhang <peng1x.zhang@intel.com>
> > ---
> >   app/test-pmd/testpmd.c | 20 ++++++++++++++++++--
> >   1 file changed, 18 insertions(+), 2 deletions(-)
> >
> > diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> > addcbcac85..70f907d96b 100644
> > --- a/app/test-pmd/testpmd.c
> > +++ b/app/test-pmd/testpmd.c
> > @@ -75,6 +75,8 @@
> >
> >   #include "testpmd.h"
> >
> > +#include <ethdev_driver.h>
> > +
> >   #ifndef MAP_HUGETLB
> >   /* FreeBSD may not have MAP_HUGETLB (in fact, it probably doesn't) */
> >   #define HUGE_FLAG (0x40000)
> > @@ -2402,9 +2404,23 @@ start_packet_forwarding(int with_tx_first)
> >   	if (!pkt_fwd_shared_rxq_check())
> >   		return;
> >
> > -	if (stream_init != NULL)
> > -		for (i = 0; i < cur_fwd_config.nb_fwd_streams; i++)
> > +	if (stream_init != NULL) {
> > +		for (i = 0; i < cur_fwd_config.nb_fwd_streams; i++) {
> > +			if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
> > +				struct fwd_stream *fs = fwd_streams[i];
> > +				struct rte_eth_dev_data *dev_rx_data,
> *dev_tx_data;
> > +
> > +				dev_rx_data = (&rte_eth_devices[fs-
> >rx_port])->data;
> > +				dev_tx_data = (&rte_eth_devices[fs->tx_port])-
> >data;
> > +
> > +				uint8_t rx_state = dev_rx_data-
> >rx_queue_state[fs->rx_port];
> 
> To get the queue state, the array rx_queue_state[] should be indexed by the
> queue number.
> Using fs->rx_port may not give desired queue's state.
> 
> > +				ports[fs->rx_port].rxq[fs->rx_queue].state =
> rx_state;
> > +				uint8_t tx_state = dev_tx_data-
> >tx_queue_state[fs->tx_port];
> 
> Same as rx queue above. We might need to root cause the issue further.
> 
> > +				ports[fs->tx_port].txq[fs->tx_queue].state =
> tx_state;
> > +			}
> >   			stream_init(fwd_streams[i]);
> > +		}
> > +	}
> >
> >   	port_fwd_begin = cur_fwd_config.fwd_eng->port_fwd_begin;
> >   	if (port_fwd_begin != NULL) {

[PATCH v3] app/testpmd: fix incorrect queues state of secondary process

[Peng Zhang]

Primary process could set up queues state correctly when starting port,
while secondary process not. Under multi-process scenario, "stream_init"
function would get wrong queues state for secondary process.

This commit is to get queues state from ethdev which is located in
shared memory.

Fixes: 3c4426db54fc ("app/testpmd: do not poll stopped queues")
Cc: stable@dpdk.org

Signed-off-by: Peng Zhang <peng1x.zhang@intel.com>

---
 v3:
 - Modify the parameter of rx or tx queue state array 
 v2:
 - Change the way of getting secondary process queues states
---
 app/test-pmd/testpmd.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index addcbcac85..977ec4fa28 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -75,6 +75,8 @@
 
 #include "testpmd.h"
 
+#include <ethdev_driver.h>
+
 #ifndef MAP_HUGETLB
 /* FreeBSD may not have MAP_HUGETLB (in fact, it probably doesn't) */
 #define HUGE_FLAG (0x40000)
@@ -2402,10 +2404,24 @@ start_packet_forwarding(int with_tx_first)
 	if (!pkt_fwd_shared_rxq_check())
 		return;
 
-	if (stream_init != NULL)
-		for (i = 0; i < cur_fwd_config.nb_fwd_streams; i++)
-			stream_init(fwd_streams[i]);
+	if (stream_init != NULL) {
+		for (i = 0; i < cur_fwd_config.nb_fwd_streams; i++) {
+			if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
+				struct fwd_stream *fs = fwd_streams[i];
+				struct rte_eth_dev_data *dev_rx_data, *dev_tx_data;
+
+				dev_rx_data = (&rte_eth_devices[fs->rx_port])->data;
+				dev_tx_data = (&rte_eth_devices[fs->tx_port])->data;
+
+				uint8_t rx_state = dev_rx_data->rx_queue_state[fs->rx_queue];
+				ports[fs->rx_port].rxq[fs->rx_queue].state = rx_state;
+				uint8_t tx_state = dev_tx_data->tx_queue_state[fs->tx_queue];
+				ports[fs->tx_port].txq[fs->tx_queue].state = tx_state;
+			}
 
+			stream_init(fwd_streams[i]);
+		}
+	}
 	port_fwd_begin = cur_fwd_config.fwd_eng->port_fwd_begin;
 	if (port_fwd_begin != NULL) {
 		for (i = 0; i < cur_fwd_config.nb_fwd_ports; i++) {
-- 
2.25.1

Re: [PATCH v3] app/testpmd: fix incorrect queues state of secondary process

[lihuisong (C)]

在 2022/9/6 22:53, Peng Zhang 写道:
> Primary process could set up queues state correctly when starting port,
> while secondary process not. Under multi-process scenario, "stream_init"
> function would get wrong queues state for secondary process.
>
> This commit is to get queues state from ethdev which is located in
> shared memory.
>
> Fixes: 3c4426db54fc ("app/testpmd: do not poll stopped queues")
> Cc: stable@dpdk.org
>
> Signed-off-by: Peng Zhang <peng1x.zhang@intel.com>
>
> ---
>   v3:
>   - Modify the parameter of rx or tx queue state array
>   v2:
>   - Change the way of getting secondary process queues states
> ---
>   app/test-pmd/testpmd.c | 22 +++++++++++++++++++---
>   1 file changed, 19 insertions(+), 3 deletions(-)
>
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> index addcbcac85..977ec4fa28 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -75,6 +75,8 @@
>   
>   #include "testpmd.h"
>   
> +#include <ethdev_driver.h>
This header file is internal, app shouldn't use it.
> +
>   #ifndef MAP_HUGETLB
>   /* FreeBSD may not have MAP_HUGETLB (in fact, it probably doesn't) */
>   #define HUGE_FLAG (0x40000)
> @@ -2402,10 +2404,24 @@ start_packet_forwarding(int with_tx_first)
>   	if (!pkt_fwd_shared_rxq_check())
>   		return;
>   
> -	if (stream_init != NULL)
> -		for (i = 0; i < cur_fwd_config.nb_fwd_streams; i++)
> -			stream_init(fwd_streams[i]);
> +	if (stream_init != NULL) {
> +		for (i = 0; i < cur_fwd_config.nb_fwd_streams; i++) {
> +			if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
> +				struct fwd_stream *fs = fwd_streams[i];
> +				struct rte_eth_dev_data *dev_rx_data, *dev_tx_data;
> +
> +				dev_rx_data = (&rte_eth_devices[fs->rx_port])->data;
> +				dev_tx_data = (&rte_eth_devices[fs->tx_port])->data;
> +
> +				uint8_t rx_state = dev_rx_data->rx_queue_state[fs->rx_queue];
> +				ports[fs->rx_port].rxq[fs->rx_queue].state = rx_state;
> +				uint8_t tx_state = dev_tx_data->tx_queue_state[fs->tx_queue];
> +				ports[fs->tx_port].txq[fs->tx_queue].state = tx_state;
> +			}
>   
> +			stream_init(fwd_streams[i]);
> +		}
> +	}
>   
Suggest that an API in ethdev layer can be encapsulated to obtain the 
device Rx/Tx queue state.
Both primary and secondary process get or set queue state by this API.
>   	port_fwd_begin = cur_fwd_config.fwd_eng->port_fwd_begin;
>   	if (port_fwd_begin != NULL) {
>   		for (i = 0; i < cur_fwd_config.nb_fwd_ports; i++) {

RE: [PATCH v3] app/testpmd: fix incorrect queues state of secondary process

[Zhang, Peng1X]

> -----Original Message-----
> From: lihuisong (C) <lihuisong@huawei.com>
> Sent: Wednesday, September 7, 2022 9:53 AM
> To: Zhang, Peng1X <peng1x.zhang@intel.com>; dev@dpdk.org
> Cc: andrew.rybchenko@oktetlabs.ru; Singh, Aman Deep
> <aman.deep.singh@intel.com>; Zhang, Yuying <yuying.zhang@intel.com>;
> stable@dpdk.org
> Subject: Re: [PATCH v3] app/testpmd: fix incorrect queues state of secondary
> process
> 
> 
> 在 2022/9/6 22:53, Peng Zhang 写道:
> > Primary process could set up queues state correctly when starting
> > port, while secondary process not. Under multi-process scenario,
> "stream_init"
> > function would get wrong queues state for secondary process.
> >
> > This commit is to get queues state from ethdev which is located in
> > shared memory.
> >
> > Fixes: 3c4426db54fc ("app/testpmd: do not poll stopped queues")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Peng Zhang <peng1x.zhang@intel.com>
> >
> > ---
> >   v3:
> >   - Modify the parameter of rx or tx queue state array
> >   v2:
> >   - Change the way of getting secondary process queues states
> > ---
> >   app/test-pmd/testpmd.c | 22 +++++++++++++++++++---
> >   1 file changed, 19 insertions(+), 3 deletions(-)
> >
> > diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> > addcbcac85..977ec4fa28 100644
> > --- a/app/test-pmd/testpmd.c
> > +++ b/app/test-pmd/testpmd.c
> > @@ -75,6 +75,8 @@
> >
> >   #include "testpmd.h"
> >
> > +#include <ethdev_driver.h>
> This header file is internal, app shouldn't use it.

In this case not all PMDs implement 'rte_eth_rx/tx_queue_info_get()' and ethdev won't return 'dev->data->rx/tx_queue_state'.

> > +
> >   #ifndef MAP_HUGETLB
> >   /* FreeBSD may not have MAP_HUGETLB (in fact, it probably doesn't) */
> >   #define HUGE_FLAG (0x40000)
> > @@ -2402,10 +2404,24 @@ start_packet_forwarding(int with_tx_first)
> >   	if (!pkt_fwd_shared_rxq_check())
> >   		return;
> >
> > -	if (stream_init != NULL)
> > -		for (i = 0; i < cur_fwd_config.nb_fwd_streams; i++)
> > -			stream_init(fwd_streams[i]);
> > +	if (stream_init != NULL) {
> > +		for (i = 0; i < cur_fwd_config.nb_fwd_streams; i++) {
> > +			if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
> > +				struct fwd_stream *fs = fwd_streams[i];
> > +				struct rte_eth_dev_data *dev_rx_data,
> *dev_tx_data;
> > +
> > +				dev_rx_data = (&rte_eth_devices[fs->rx_port])-
> >data;
> > +				dev_tx_data = (&rte_eth_devices[fs->tx_port])-
> >data;
> > +
> > +				uint8_t rx_state = dev_rx_data-
> >rx_queue_state[fs->rx_queue];
> > +				ports[fs->rx_port].rxq[fs->rx_queue].state =
> rx_state;
> > +				uint8_t tx_state = dev_tx_data-
> >tx_queue_state[fs->tx_queue];
> > +				ports[fs->tx_port].txq[fs->tx_queue].state =
> tx_state;
> > +			}
> >
> > +			stream_init(fwd_streams[i]);
> > +		}
> > +	}
> >
> Suggest that an API in ethdev layer can be encapsulated to obtain the device
> Rx/Tx queue state.
> Both primary and secondary process get or set queue state by this API.

Suggestion sounds good, maybe better to add a new API in ethdev layer.

@andrew, what's your opinion about this solution and huisong's suggestion?

> >   	port_fwd_begin = cur_fwd_config.fwd_eng->port_fwd_begin;
> >   	if (port_fwd_begin != NULL) {
> >   		for (i = 0; i < cur_fwd_config.nb_fwd_ports; i++) {

Re: [PATCH v3] app/testpmd: fix incorrect queues state of secondary process

[lihuisong (C)]

在 2022/9/10 17:21, Zhang, Peng1X 写道:
>
>> -----Original Message-----
>> From: lihuisong (C) <lihuisong@huawei.com>
>> Sent: Wednesday, September 7, 2022 9:53 AM
>> To: Zhang, Peng1X <peng1x.zhang@intel.com>; dev@dpdk.org
>> Cc: andrew.rybchenko@oktetlabs.ru; Singh, Aman Deep
>> <aman.deep.singh@intel.com>; Zhang, Yuying <yuying.zhang@intel.com>;
>> stable@dpdk.org
>> Subject: Re: [PATCH v3] app/testpmd: fix incorrect queues state of secondary
>> process
>>
>>
>> 在 2022/9/6 22:53, Peng Zhang 写道:
>>> Primary process could set up queues state correctly when starting
>>> port, while secondary process not. Under multi-process scenario,
>> "stream_init"
>>> function would get wrong queues state for secondary process.
>>>
>>> This commit is to get queues state from ethdev which is located in
>>> shared memory.
>>>
>>> Fixes: 3c4426db54fc ("app/testpmd: do not poll stopped queues")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Peng Zhang <peng1x.zhang@intel.com>
>>>
>>> ---
>>>    v3:
>>>    - Modify the parameter of rx or tx queue state array
>>>    v2:
>>>    - Change the way of getting secondary process queues states
>>> ---
>>>    app/test-pmd/testpmd.c | 22 +++++++++++++++++++---
>>>    1 file changed, 19 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
>>> addcbcac85..977ec4fa28 100644
>>> --- a/app/test-pmd/testpmd.c
>>> +++ b/app/test-pmd/testpmd.c
>>> @@ -75,6 +75,8 @@
>>>
>>>    #include "testpmd.h"
>>>
>>> +#include <ethdev_driver.h>
>> This header file is internal, app shouldn't use it.
> In this case not all PMDs implement 'rte_eth_rx/tx_queue_info_get()' and ethdev won't return 'dev->data->rx/tx_queue_state'.
I don't think Rx/Tx queue state needs to be put in the API above.
The queue state is modified by calling 'rte_eth_dev_rx/tx_queue_stop/start'.
Can we create a new API to report queue state? like, 
'rte_eth_dev_rx/tx_queue_state_get'.
If some PMDs do not support 'rte_eth_dev_rx/tx_queue_stop/start', the new
API always return 'START' state and preserves its original behavior.

What do you think?
>
>>> +
>>>    #ifndef MAP_HUGETLB
>>>    /* FreeBSD may not have MAP_HUGETLB (in fact, it probably doesn't) */
>>>    #define HUGE_FLAG (0x40000)
>>> @@ -2402,10 +2404,24 @@ start_packet_forwarding(int with_tx_first)
>>>    	if (!pkt_fwd_shared_rxq_check())
>>>    		return;
>>>
>>> -	if (stream_init != NULL)
>>> -		for (i = 0; i < cur_fwd_config.nb_fwd_streams; i++)
>>> -			stream_init(fwd_streams[i]);
>>> +	if (stream_init != NULL) {
>>> +		for (i = 0; i < cur_fwd_config.nb_fwd_streams; i++) {
>>> +			if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
>>> +				struct fwd_stream *fs = fwd_streams[i];
>>> +				struct rte_eth_dev_data *dev_rx_data,
>> *dev_tx_data;
>>> +
>>> +				dev_rx_data = (&rte_eth_devices[fs->rx_port])-
>>> data;
>>> +				dev_tx_data = (&rte_eth_devices[fs->tx_port])-
>>> data;
>>> +
>>> +				uint8_t rx_state = dev_rx_data-
>>> rx_queue_state[fs->rx_queue];
>>> +				ports[fs->rx_port].rxq[fs->rx_queue].state =
>> rx_state;
>>> +				uint8_t tx_state = dev_tx_data-
>>> tx_queue_state[fs->tx_queue];
>>> +				ports[fs->tx_port].txq[fs->tx_queue].state =
>> tx_state;
>>> +			}
>>>
>>> +			stream_init(fwd_streams[i]);
>>> +		}
>>> +	}
>>>
>> Suggest that an API in ethdev layer can be encapsulated to obtain the device
>> Rx/Tx queue state.
>> Both primary and secondary process get or set queue state by this API.
> Suggestion sounds good, maybe better to add a new API in ethdev layer.
>
> @andrew, what's your opinion about this solution and huisong's suggestion?
>
>>>    	port_fwd_begin = cur_fwd_config.fwd_eng->port_fwd_begin;
>>>    	if (port_fwd_begin != NULL) {
>>>    		for (i = 0; i < cur_fwd_config.nb_fwd_ports; i++) {

RE: [PATCH v3] app/testpmd: fix incorrect queues state of secondary process

[Zhou, YidingX]

> > > Primary process could set up queues state correctly when starting
> > > port, while secondary process not. Under multi-process scenario,
> > "stream_init"
> > > function would get wrong queues state for secondary process.
> > >
> > > This commit is to get queues state from ethdev which is located in
> > > shared memory.
> > >
> > > Fixes: 3c4426db54fc ("app/testpmd: do not poll stopped queues")
> > > Cc: stable@dpdk.org
> > >
> > > Signed-off-by: Peng Zhang <peng1x.zhang@intel.com>
> > >
> > > ---
> > >   v3:
> > >   - Modify the parameter of rx or tx queue state array
> > >   v2:
> > >   - Change the way of getting secondary process queues states
> > > ---
> > >   app/test-pmd/testpmd.c | 22 +++++++++++++++++++---
> > >   1 file changed, 19 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> > > addcbcac85..977ec4fa28 100644
> > > --- a/app/test-pmd/testpmd.c
> > > +++ b/app/test-pmd/testpmd.c
> > > @@ -75,6 +75,8 @@
> > >
> > >   #include "testpmd.h"
> > >
> > > +#include <ethdev_driver.h>
> > This header file is internal, app shouldn't use it.
> 
> In this case not all PMDs implement 'rte_eth_rx/tx_queue_info_get()' and
> ethdev won't return 'dev->data->rx/tx_queue_state'.
> 
> > > +
> > >   #ifndef MAP_HUGETLB
> > >   /* FreeBSD may not have MAP_HUGETLB (in fact, it probably doesn't) */
> > >   #define HUGE_FLAG (0x40000)
> > > @@ -2402,10 +2404,24 @@ start_packet_forwarding(int with_tx_first)
> > >   	if (!pkt_fwd_shared_rxq_check())
> > >   		return;
> > >
> > > -	if (stream_init != NULL)
> > > -		for (i = 0; i < cur_fwd_config.nb_fwd_streams; i++)
> > > -			stream_init(fwd_streams[i]);
> > > +	if (stream_init != NULL) {
> > > +		for (i = 0; i < cur_fwd_config.nb_fwd_streams; i++) {
> > > +			if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
> > > +				struct fwd_stream *fs = fwd_streams[i];
> > > +				struct rte_eth_dev_data *dev_rx_data,
> > *dev_tx_data;
> > > +
> > > +				dev_rx_data = (&rte_eth_devices[fs-
> >rx_port])-
> > >data;
> > > +				dev_tx_data = (&rte_eth_devices[fs->tx_port])-
> > >data;
> > > +
> > > +				uint8_t rx_state = dev_rx_data-
> > >rx_queue_state[fs->rx_queue];
> > > +				ports[fs->rx_port].rxq[fs->rx_queue].state =
> > rx_state;
> > > +				uint8_t tx_state = dev_tx_data-
> > >tx_queue_state[fs->tx_queue];
> > > +				ports[fs->tx_port].txq[fs->tx_queue].state =
> > tx_state;
> > > +			}
> > >
> > > +			stream_init(fwd_streams[i]);
> > > +		}
> > > +	}
> > >
> > Suggest that an API in ethdev layer can be encapsulated to obtain the
> > device Rx/Tx queue state.
> > Both primary and secondary process get or set queue state by this API.
> 
> Suggestion sounds good, maybe better to add a new API in ethdev layer.
> 
> @andrew, what's your opinion about this solution and huisong's suggestion?
> 
> > >   	port_fwd_begin = cur_fwd_config.fwd_eng->port_fwd_begin;
> > >   	if (port_fwd_begin != NULL) {
> > >   		for (i = 0; i < cur_fwd_config.nb_fwd_ports; i++) {

Hi, @andrew.rybchenko@oktetlabs.ru can you please help review this patch? Thanks.

RE: [PATCH v3] app/testpmd: fix incorrect queues state of secondary process

[Zhou, YidingX]

Hi

> -----Original Message-----
> From: Zhou, YidingX <yidingx.zhou@intel.com>
> Sent: Thursday, September 29, 2022 9:58 AM
> To: Zhang, Peng1X <peng1x.zhang@intel.com>; lihuisong (C)
> <lihuisong@huawei.com>; dev@dpdk.org; andrew.rybchenko@oktetlabs.ru
> Cc: Singh, Aman Deep <aman.deep.singh@intel.com>; Zhang, Yuying
> <yuying.zhang@intel.com>
> Subject: RE: [PATCH v3] app/testpmd: fix incorrect queues state of secondary
> process
> 
> > > > Primary process could set up queues state correctly when starting
> > > > port, while secondary process not. Under multi-process scenario,
> > > "stream_init"
> > > > function would get wrong queues state for secondary process.
> > > >
> > > > This commit is to get queues state from ethdev which is located in
> > > > shared memory.
> > > >
> > > > Fixes: 3c4426db54fc ("app/testpmd: do not poll stopped queues")
> > > > Cc: stable@dpdk.org
> > > >
> > > > Signed-off-by: Peng Zhang <peng1x.zhang@intel.com>

Re: [PATCH v3] app/testpmd: fix incorrect queues state of secondary process

[Stephen Hemminger]

On Tue,  6 Sep 2022 22:53:10 +0800
Peng Zhang <peng1x.zhang@intel.com> wrote:

> +			if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
> +				struct fwd_stream *fs = fwd_streams[i];
> +				struct rte_eth_dev_data *dev_rx_data, *dev_tx_data;
> +
> +				dev_rx_data = (&rte_eth_devices[fs->rx_port])->data;
> +				dev_tx_data = (&rte_eth_devices[fs->tx_port])->data;
> +
> +				uint8_t rx_state = dev_rx_data->rx_queue_state[fs->rx_queue];
> +				ports[fs->rx_port].rxq[fs->rx_queue].state = rx_state;
> +				uint8_t tx_state = dev_tx_data->tx_queue_state[fs->tx_queue];
> +				ports[fs->tx_port].txq[fs->tx_queue].state = tx_state;
> +			}

Could the logic be put in stream_init() like this?

It keeps with the function pointer model object style model in that code.
Also, it makes testpmd more dependent on data structures that should be
hidden and internal only (rte_eth_devices).

RE: [PATCH v3] app/testpmd: fix incorrect queues state of secondary process

[Zhou, YidingX]

> 
> > +			if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
> > +				struct fwd_stream *fs = fwd_streams[i];
> > +				struct rte_eth_dev_data *dev_rx_data,
> *dev_tx_data;
> > +
> > +				dev_rx_data = (&rte_eth_devices[fs-
> >rx_port])->data;
> > +				dev_tx_data = (&rte_eth_devices[fs->tx_port])-
> >data;
> > +
> > +				uint8_t rx_state = dev_rx_data-
> >rx_queue_state[fs->rx_queue];
> > +				ports[fs->rx_port].rxq[fs->rx_queue].state =
> rx_state;
> > +				uint8_t tx_state = dev_tx_data-
> >tx_queue_state[fs->tx_queue];
> > +				ports[fs->tx_port].txq[fs->tx_queue].state =
> tx_state;
> > +			}
> 
> Could the logic be put in stream_init() like this?
> 
> It keeps with the function pointer model object style model in that code.
> Also, it makes testpmd more dependent on data structures that should be
> hidden and internal only (rte_eth_devices).
> 
'strean_init' is a function pointer, putting this logic in will produce a lot of similar redundant code in all forwarding modules, like 'iofwd' 'macfwd'  and etc.
It's better to hide 'rte_eth_devices' from testpmd, but this requires an API at the EAL layer to access the queue state,
'rte_eth_rx/tx_queue_info_get()' can not get queue's state now.

Re: [PATCH v3] app/testpmd: fix incorrect queues state of secondary process

[Andrew Rybchenko]

On 9/13/22 04:26, lihuisong (C) wrote:
> 
> 在 2022/9/10 17:21, Zhang, Peng1X 写道:
>>
>>> -----Original Message-----
>>> From: lihuisong (C) <lihuisong@huawei.com>
>>> Sent: Wednesday, September 7, 2022 9:53 AM
>>> To: Zhang, Peng1X <peng1x.zhang@intel.com>; dev@dpdk.org
>>> Cc: andrew.rybchenko@oktetlabs.ru; Singh, Aman Deep
>>> <aman.deep.singh@intel.com>; Zhang, Yuying <yuying.zhang@intel.com>;
>>> stable@dpdk.org
>>> Subject: Re: [PATCH v3] app/testpmd: fix incorrect queues state of 
>>> secondary
>>> process
>>>
>>>
>>> 在 2022/9/6 22:53, Peng Zhang 写道:
>>>> Primary process could set up queues state correctly when starting
>>>> port, while secondary process not. Under multi-process scenario,
>>> "stream_init"
>>>> function would get wrong queues state for secondary process.
>>>>
>>>> This commit is to get queues state from ethdev which is located in
>>>> shared memory.
>>>>
>>>> Fixes: 3c4426db54fc ("app/testpmd: do not poll stopped queues")
>>>> Cc: stable@dpdk.org
>>>>
>>>> Signed-off-by: Peng Zhang <peng1x.zhang@intel.com>
>>>>
>>>> ---
>>>>    v3:
>>>>    - Modify the parameter of rx or tx queue state array
>>>>    v2:
>>>>    - Change the way of getting secondary process queues states
>>>> ---
>>>>    app/test-pmd/testpmd.c | 22 +++++++++++++++++++---
>>>>    1 file changed, 19 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
>>>> addcbcac85..977ec4fa28 100644
>>>> --- a/app/test-pmd/testpmd.c
>>>> +++ b/app/test-pmd/testpmd.c
>>>> @@ -75,6 +75,8 @@
>>>>
>>>>    #include "testpmd.h"
>>>>
>>>> +#include <ethdev_driver.h>
>>> This header file is internal, app shouldn't use it.

+1

>> In this case not all PMDs implement 'rte_eth_rx/tx_queue_info_get()' 
>> and ethdev won't return 'dev->data->rx/tx_queue_state'.
> I don't think Rx/Tx queue state needs to be put in the API above.
> The queue state is modified by calling 
> 'rte_eth_dev_rx/tx_queue_stop/start'.
> Can we create a new API to report queue state? like, 
> 'rte_eth_dev_rx/tx_queue_state_get'.

We can and it does not require driver callback since ethdev
layer knows the queue state, but do we really need it?
Application controls queues state and should know the state
internally.

> If some PMDs do not support 'rte_eth_dev_rx/tx_queue_stop/start', the new
> API always return 'START' state and preserves its original behavior.

It is not required since ethdev layer knows queue state.

> 
> What do you think?
>>
>>>> +
>>>>    #ifndef MAP_HUGETLB
>>>>    /* FreeBSD may not have MAP_HUGETLB (in fact, it probably 
>>>> doesn't) */
>>>>    #define HUGE_FLAG (0x40000)
>>>> @@ -2402,10 +2404,24 @@ start_packet_forwarding(int with_tx_first)
>>>>        if (!pkt_fwd_shared_rxq_check())
>>>>            return;
>>>>
>>>> -    if (stream_init != NULL)
>>>> -        for (i = 0; i < cur_fwd_config.nb_fwd_streams; i++)
>>>> -            stream_init(fwd_streams[i]);
>>>> +    if (stream_init != NULL) {
>>>> +        for (i = 0; i < cur_fwd_config.nb_fwd_streams; i++) {
>>>> +            if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
>>>> +                struct fwd_stream *fs = fwd_streams[i];
>>>> +                struct rte_eth_dev_data *dev_rx_data,
>>> *dev_tx_data;
>>>> +
>>>> +                dev_rx_data = (&rte_eth_devices[fs->rx_port])-
>>>> data;
>>>> +                dev_tx_data = (&rte_eth_devices[fs->tx_port])-
>>>> data;
>>>> +
>>>> +                uint8_t rx_state = dev_rx_data-
>>>> rx_queue_state[fs->rx_queue];
>>>> +                ports[fs->rx_port].rxq[fs->rx_queue].state =
>>> rx_state;
>>>> +                uint8_t tx_state = dev_tx_data-
>>>> tx_queue_state[fs->tx_queue];
>>>> +                ports[fs->tx_port].txq[fs->tx_queue].state =
>>> tx_state;
>>>> +            }
>>>>
>>>> +            stream_init(fwd_streams[i]);
>>>> +        }
>>>> +    }
>>>>
>>> Suggest that an API in ethdev layer can be encapsulated to obtain the 
>>> device
>>> Rx/Tx queue state.
>>> Both primary and secondary process get or set queue state by this API.
>> Suggestion sounds good, maybe better to add a new API in ethdev layer.
>>
>> @andrew, what's your opinion about this solution and huisong's 
>> suggestion?

May be it is really more convenient to have ethdev API to get
queue state, but may be it is too late for the API in the
release cycle phase. I'm sorry for late review.

>>
>>>>        port_fwd_begin = cur_fwd_config.fwd_eng->port_fwd_begin;
>>>>        if (port_fwd_begin != NULL) {
>>>>            for (i = 0; i < cur_fwd_config.nb_fwd_ports; i++) {