app/testpmd: fix secondary process cannot dump packet
Checks
Commit Message
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(+)
Comments
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,
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
在 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,
>
> .
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
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,
> >
> > .
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,
>>> .
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.
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,
> >>> .
在 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,
>>>>> .
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,
>>>>>> .
@@ -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,