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

["lihuisong (C)" <lihuisong@huawei.com>]

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