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 763C5A0556; Mon, 17 Oct 2022 10:05:08 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 656B74021D; Mon, 17 Oct 2022 10:05:08 +0200 (CEST) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id 3D19340143 for ; Mon, 17 Oct 2022 10:05:07 +0200 (CEST) Received: from [192.168.38.17] (aros.oktetlabs.ru [192.168.38.17]) (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 A17D883; Mon, 17 Oct 2022 11:05:06 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru A17D883 DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1665993906; bh=9D1MmVLgkKz1eSkq/3IR21kAuw6qoe2iwqmE7uJs4EY=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=taXe+61pIihPcmrUNlDotInBA4W9uo8D7pBrTV4Iwu3lYzp0vDE0KLfD9I6rcPaCe XLULT4SaQIcCptw9yLb9FEb9zw5KdpuaFBqJj0RMTJysDrB7Vvk5Zzc+svQzLzxfXr pOb1aGvIcox36/QmMu4OQ7UHKZzHgYZqOWEhKVCc= Message-ID: <4e383f34-e990-9af0-46f8-4d328c7d9ccc@oktetlabs.ru> Date: Mon, 17 Oct 2022 11:05:06 +0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.3.0 Subject: Re: [PATCH v3] app/testpmd: fix incorrect queues state of secondary process Content-Language: en-US To: "lihuisong (C)" , "Zhang, Peng1X" , "dev@dpdk.org" Cc: "Singh, Aman Deep" , "Zhang, Yuying" , "Zhou, YidingX" References: <20220819100940.657437-1-peng1x.zhang@intel.com> <20220906145310.8849-1-peng1x.zhang@intel.com> <6f5b04f7-d6a0-a3b8-2e45-ec3e1744a3d2@huawei.com> <04c582f4-08c2-14c3-653d-4eac2e012181@huawei.com> From: Andrew Rybchenko Organization: OKTET Labs In-Reply-To: <04c582f4-08c2-14c3-653d-4eac2e012181@huawei.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org On 9/13/22 04:26, lihuisong (C) wrote: > > 在 2022/9/10 17:21, Zhang, Peng1X 写道: >> >>> -----Original Message----- >>> From: lihuisong (C) >>> Sent: Wednesday, September 7, 2022 9:53 AM >>> To: Zhang, Peng1X ; dev@dpdk.org >>> Cc: andrew.rybchenko@oktetlabs.ru; Singh, Aman Deep >>> ; Zhang, Yuying ; >>> 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 >>>> >>>> --- >>>>    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 >>> 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++) {