From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id CC7B3A0543 for ; Wed, 6 Jul 2022 15:40:28 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id BC50940DF7; Wed, 6 Jul 2022 15:40:28 +0200 (CEST) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id E3B0D40150; Wed, 6 Jul 2022 15:40:25 +0200 (CEST) Received: from [192.168.1.38] (unknown [188.170.75.69]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by shelob.oktetlabs.ru (Postfix) with ESMTPSA id E3CD4F5; Wed, 6 Jul 2022 16:40:24 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru E3CD4F5 DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1657114825; bh=4RES1UDuygnE3cd720V0XaqEH7g2oN00VzPHVvWMMsc=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=PSct1TptrVwX5q2axSZm9gwscP3WtP6NXf66WMFd9gf78veuZ48eJNWwGzkCXb3Ap vZ7YCUBgcY4oupnBNU1inimNp/9EeBldGJ+RE/jRahhOFIZVrurR1KqHn0Hym+GDDY /+GiC8dVieS0b++xiNT2UVVgIiCnpdPa2a7B7VII= Message-ID: <467ead05-0a38-2b09-c1ad-b87650ad4300@oktetlabs.ru> Date: Wed, 6 Jul 2022 16:40:17 +0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.10.0 Subject: Re: [PATCH] app/testpmd: fix secondary process cannot dump packet Content-Language: en-US To: "lihuisong (C)" , "Zhang, Peng1X" , "dev@dpdk.org" , Thomas Monjalon Cc: "Singh, Aman Deep" , "Zhang, Yuying" , "stable@dpdk.org" References: <20220623181502.181567-1-peng1x.zhang@intel.com> <5bd19b16-f878-98fe-e1ea-d992c1ffcaa7@oktetlabs.ru> <9a5ab8bb-ccdd-25d2-65d6-370ef95ab712@huawei.com> <02c5984f-d2e4-5d6c-3f75-924246c4c716@huawei.com> From: Andrew Rybchenko In-Reply-To: <02c5984f-d2e4-5d6c-3f75-924246c4c716@huawei.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-BeenThere: stable@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: patches for DPDK stable branches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: stable-bounces@dpdk.org 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) >>> Sent: Monday, July 4, 2022 10:37 AM >>> To: Zhang, Peng1X ; Andrew Rybchenko >>> ; dev@dpdk.org >>> Cc: Singh, Aman Deep ; Zhang, Yuying >>> ; 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) >>>>> Sent: Wednesday, June 29, 2022 10:55 AM >>>>> To: Andrew Rybchenko ; Zhang, Peng1X >>>>> ; dev@dpdk.org >>>>> Cc: Singh, Aman Deep ; Zhang, Yuying >>>>> ; 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 >>>>>>> >>>>>>> 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 >>>>>>> --- >>>>>>>     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, >>>>>> .