[v3] app/testpmd: fix secondary process not forwarding

Message ID 20230223144106.707999-1-shiyangx.he@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers
Series [v3] app/testpmd: fix secondary process not forwarding |

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/loongarch-compilation success Compilation OK
ci/loongarch-unit-testing success Unit Testing PASS
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/Intel-compilation success Compilation OK
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/intel-Testing success Testing PASS
ci/github-robot: build success github build: passed
ci/iol-testing success Testing PASS
ci/iol-x86_64-unit-testing success Testing PASS
ci/iol-x86_64-compile-testing success Testing PASS
ci/iol-abi-testing success Testing PASS

Commit Message

Shiyang He Feb. 23, 2023, 2:41 p.m. UTC
  Under multi-process scenario, the secondary process gets queue state
from the wrong location (the global variable 'ports'). Therefore, the
secondary process can not forward since "stream_init" is not called.

This commit fixes the issue by calling 'rte_eth_rx/tx_queue_info_get'
to get queue state from shared memory.

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

Signed-off-by: Shiyang He <shiyangx.he@intel.com>
Acked-by: Yuying Zhang <yuying.zhang@intel.com>

v3: Add return value description
---
 app/test-pmd/testpmd.c | 45 ++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 43 insertions(+), 2 deletions(-)
  

Comments

lihuisong (C) Feb. 23, 2023, 8:08 a.m. UTC | #1
在 2023/2/23 22:41, Shiyang He 写道:
> Under multi-process scenario, the secondary process gets queue state
> from the wrong location (the global variable 'ports'). Therefore, the
> secondary process can not forward since "stream_init" is not called.
>
> This commit fixes the issue by calling 'rte_eth_rx/tx_queue_info_get'
> to get queue state from shared memory.
>
> Fixes: 3c4426db54fc ("app/testpmd: do not poll stopped queues")
> Cc: stable@dpdk.org
>
> Signed-off-by: Shiyang He <shiyangx.he@intel.com>
> Acked-by: Yuying Zhang <yuying.zhang@intel.com>
>
> v3: Add return value description
> ---
>   app/test-pmd/testpmd.c | 45 ++++++++++++++++++++++++++++++++++++++++--
>   1 file changed, 43 insertions(+), 2 deletions(-)
>
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> index 0c14325b8d..a050472aea 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -2418,9 +2418,50 @@ 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_SECONDARY) {
> +				struct fwd_stream *fs = fwd_streams[i];
> +				struct rte_eth_rxq_info rx_qinfo;
> +				struct rte_eth_txq_info tx_qinfo;
> +				int32_t rc;
> +				rc = rte_eth_rx_queue_info_get(fs->rx_port,
> +						fs->rx_queue, &rx_qinfo);
> +				if (rc == 0) {
> +					ports[fs->rx_port].rxq[fs->rx_queue].state =
> +						rx_qinfo.queue_state;
> +				} else if (rc == -ENOTSUP) {
> +					/* Set the rxq state to RTE_ETH_QUEUE_STATE_STARTED
> +					 * to ensure that the PMDs do not implement
> +					 * rte_eth_rx_queue_info_get can forward.
> +					 */
> +					ports[fs->rx_port].rxq[fs->rx_queue].state =
> +						RTE_ETH_QUEUE_STATE_STARTED;
> +				} else {
> +					TESTPMD_LOG(WARNING,
> +						"Failed to get rx queue info\n");
> +				}
> +
> +				rc = rte_eth_tx_queue_info_get(fs->tx_port,
> +						fs->tx_queue, &tx_qinfo);
> +				if (rc == 0) {
> +					ports[fs->tx_port].txq[fs->tx_queue].state =
> +						tx_qinfo.queue_state;
> +				} else if (rc == -ENOTSUP) {
> +					/* Set the txq state to RTE_ETH_QUEUE_STATE_STARTED
> +					 * to ensure that the PMDs do not implement
> +					 * rte_eth_tx_queue_info_get can forward.
> +					 */
> +					ports[fs->tx_port].txq[fs->tx_queue].state =
> +						RTE_ETH_QUEUE_STATE_STARTED;
> +				} else {
> +					TESTPMD_LOG(WARNING,
> +						"Failed to get tx queue info\n");
> +				}
> +			}
This code is a little long after adding some comments.😂
It is a full code block. Let's extract a function to do it. what do you 
think, Shiyang?
>   			stream_init(fwd_streams[i]);
> +		}
> +	}
>   
>   	port_fwd_begin = cur_fwd_config.fwd_eng->port_fwd_begin;
>   	if (port_fwd_begin != NULL) {
  
Ferruh Yigit March 6, 2023, 3:05 p.m. UTC | #2
On 2/23/2023 2:41 PM, Shiyang He wrote:
> Under multi-process scenario, the secondary process gets queue state
> from the wrong location (the global variable 'ports'). Therefore, the
> secondary process can not forward since "stream_init" is not called.
> 
> This commit fixes the issue by calling 'rte_eth_rx/tx_queue_info_get'
> to get queue state from shared memory.
> 
> Fixes: 3c4426db54fc ("app/testpmd: do not poll stopped queues")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Shiyang He <shiyangx.he@intel.com>
> Acked-by: Yuying Zhang <yuying.zhang@intel.com>
> 
> v3: Add return value description
> ---
>  app/test-pmd/testpmd.c | 45 ++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 43 insertions(+), 2 deletions(-)
> 
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> index 0c14325b8d..a050472aea 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -2418,9 +2418,50 @@ 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_SECONDARY) {
> +				struct fwd_stream *fs = fwd_streams[i];
> +				struct rte_eth_rxq_info rx_qinfo;
> +				struct rte_eth_txq_info tx_qinfo;
> +				int32_t rc;
> +				rc = rte_eth_rx_queue_info_get(fs->rx_port,
> +						fs->rx_queue, &rx_qinfo);
> +				if (rc == 0) {
> +					ports[fs->rx_port].rxq[fs->rx_queue].state =
> +						rx_qinfo.queue_state;
> +				} else if (rc == -ENOTSUP) {
> +					/* Set the rxq state to RTE_ETH_QUEUE_STATE_STARTED
> +					 * to ensure that the PMDs do not implement
> +					 * rte_eth_rx_queue_info_get can forward.
> +					 */
> +					ports[fs->rx_port].rxq[fs->rx_queue].state =
> +						RTE_ETH_QUEUE_STATE_STARTED;
> +				} else {
> +					TESTPMD_LOG(WARNING,
> +						"Failed to get rx queue info\n");
> +				}
> +
> +				rc = rte_eth_tx_queue_info_get(fs->tx_port,
> +						fs->tx_queue, &tx_qinfo);
> +				if (rc == 0) {
> +					ports[fs->tx_port].txq[fs->tx_queue].state =
> +						tx_qinfo.queue_state;
> +				} else if (rc == -ENOTSUP) {
> +					/* Set the txq state to RTE_ETH_QUEUE_STATE_STARTED
> +					 * to ensure that the PMDs do not implement
> +					 * rte_eth_tx_queue_info_get can forward.
> +					 */
> +					ports[fs->tx_port].txq[fs->tx_queue].state =
> +						RTE_ETH_QUEUE_STATE_STARTED;
> +				} else {
> +					TESTPMD_LOG(WARNING,
> +						"Failed to get tx queue info\n");
> +				}
> +			}
>  			stream_init(fwd_streams[i]);
> +		}
> +	}
>  


Testpmd duplicates some dpdk/ethdev state/config in application level,
and this can bite in multiple cases, as it is happening here.

I am not sure if this was a design decision, but I think instead of
testpmd storing ethdev related state/config in application level, it
should store only application level state/config, and when ethdev
related state/config is required app should get it directly from ethdev.

It may be too late already for testpmd, there is a mixed usage, but I am
for preferring this approach when there is an opportunity.



For above issue, why queue state needs to be stored in application level
'port' variable?
Where is this queue state used?

Can it work to get queue state directly from ethdev where this state is
used, instead of storing it in the 'port' variable in advance?

And perhaps testpmd 'port' variable can be updated there, both for
primary and secondary, for backward compatibility (other existing users
of this queue state).

What do you think?
  
Ferruh Yigit March 6, 2023, 3:16 p.m. UTC | #3
On 2/23/2023 8:08 AM, lihuisong (C) wrote:
> 
> 在 2023/2/23 22:41, Shiyang He 写道:
>> Under multi-process scenario, the secondary process gets queue state
>> from the wrong location (the global variable 'ports'). Therefore, the
>> secondary process can not forward since "stream_init" is not called.
>>
>> This commit fixes the issue by calling 'rte_eth_rx/tx_queue_info_get'
>> to get queue state from shared memory.
>>
>> Fixes: 3c4426db54fc ("app/testpmd: do not poll stopped queues")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Shiyang He <shiyangx.he@intel.com>
>> Acked-by: Yuying Zhang <yuying.zhang@intel.com>
>>
>> v3: Add return value description
>> ---
>>   app/test-pmd/testpmd.c | 45 ++++++++++++++++++++++++++++++++++++++++--
>>   1 file changed, 43 insertions(+), 2 deletions(-)
>>
>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
>> index 0c14325b8d..a050472aea 100644
>> --- a/app/test-pmd/testpmd.c
>> +++ b/app/test-pmd/testpmd.c
>> @@ -2418,9 +2418,50 @@ 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_SECONDARY) {
>> +                struct fwd_stream *fs = fwd_streams[i];
>> +                struct rte_eth_rxq_info rx_qinfo;
>> +                struct rte_eth_txq_info tx_qinfo;
>> +                int32_t rc;
>> +                rc = rte_eth_rx_queue_info_get(fs->rx_port,
>> +                        fs->rx_queue, &rx_qinfo);
>> +                if (rc == 0) {
>> +                    ports[fs->rx_port].rxq[fs->rx_queue].state =
>> +                        rx_qinfo.queue_state;
>> +                } else if (rc == -ENOTSUP) {
>> +                    /* Set the rxq state to RTE_ETH_QUEUE_STATE_STARTED
>> +                     * to ensure that the PMDs do not implement
>> +                     * rte_eth_rx_queue_info_get can forward.
>> +                     */
>> +                    ports[fs->rx_port].rxq[fs->rx_queue].state =
>> +                        RTE_ETH_QUEUE_STATE_STARTED;
>> +                } else {
>> +                    TESTPMD_LOG(WARNING,
>> +                        "Failed to get rx queue info\n");
>> +                }
>> +
>> +                rc = rte_eth_tx_queue_info_get(fs->tx_port,
>> +                        fs->tx_queue, &tx_qinfo);
>> +                if (rc == 0) {
>> +                    ports[fs->tx_port].txq[fs->tx_queue].state =
>> +                        tx_qinfo.queue_state;
>> +                } else if (rc == -ENOTSUP) {
>> +                    /* Set the txq state to RTE_ETH_QUEUE_STATE_STARTED
>> +                     * to ensure that the PMDs do not implement
>> +                     * rte_eth_tx_queue_info_get can forward.
>> +                     */
>> +                    ports[fs->tx_port].txq[fs->tx_queue].state =
>> +                        RTE_ETH_QUEUE_STATE_STARTED;
>> +                } else {
>> +                    TESTPMD_LOG(WARNING,
>> +                        "Failed to get tx queue info\n");
>> +                }
>> +            }
> This code is a little long after adding some comments.😂
> It is a full code block. Let's extract a function to do it. what do you
> think, Shiyang?


+1 to extract code into a function.

>>               stream_init(fwd_streams[i]);
>> +        }
>> +    }
>>         port_fwd_begin = cur_fwd_config.fwd_eng->port_fwd_begin;
>>       if (port_fwd_begin != NULL) {
  
Shiyang He March 7, 2023, 3:25 a.m. UTC | #4
>-----Original Message-----
>From: Ferruh Yigit <ferruh.yigit@amd.com>
>Sent: Monday, March 6, 2023 11:06 PM
>To: He, ShiyangX <shiyangx.he@intel.com>; dev@dpdk.org
>Cc: Zhou, YidingX <yidingx.zhou@intel.com>; stable@dpdk.org; Zhang, Yuying
><yuying.zhang@intel.com>; Singh, Aman Deep
><aman.deep.singh@intel.com>; Burakov, Anatoly
><anatoly.burakov@intel.com>; Matan Azrad <matan@nvidia.com>; Dmitry
>Kozlyuk <dmitry.kozliuk@gmail.com>
>Subject: Re: [PATCH v3] app/testpmd: fix secondary process not forwarding
>
>On 2/23/2023 2:41 PM, Shiyang He wrote:
>> Under multi-process scenario, the secondary process gets queue state
>> from the wrong location (the global variable 'ports'). Therefore, the
>> secondary process can not forward since "stream_init" is not called.
>>
>> This commit fixes the issue by calling 'rte_eth_rx/tx_queue_info_get'
>> to get queue state from shared memory.
>>
>> Fixes: 3c4426db54fc ("app/testpmd: do not poll stopped queues")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Shiyang He <shiyangx.he@intel.com>
>> Acked-by: Yuying Zhang <yuying.zhang@intel.com>
>>
>> v3: Add return value description
>> ---
>>  app/test-pmd/testpmd.c | 45
>> ++++++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 43 insertions(+), 2 deletions(-)
>>
>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
>> 0c14325b8d..a050472aea 100644
>> --- a/app/test-pmd/testpmd.c
>> +++ b/app/test-pmd/testpmd.c
>> @@ -2418,9 +2418,50 @@ 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_SECONDARY)
>{
>> +				struct fwd_stream *fs = fwd_streams[i];
>> +				struct rte_eth_rxq_info rx_qinfo;
>> +				struct rte_eth_txq_info tx_qinfo;
>> +				int32_t rc;
>> +				rc = rte_eth_rx_queue_info_get(fs->rx_port,
>> +						fs->rx_queue, &rx_qinfo);
>> +				if (rc == 0) {
>> +					ports[fs->rx_port].rxq[fs-
>>rx_queue].state =
>> +						rx_qinfo.queue_state;
>> +				} else if (rc == -ENOTSUP) {
>> +					/* Set the rxq state to
>RTE_ETH_QUEUE_STATE_STARTED
>> +					 * to ensure that the PMDs do not
>implement
>> +					 * rte_eth_rx_queue_info_get can
>forward.
>> +					 */
>> +					ports[fs->rx_port].rxq[fs-
>>rx_queue].state =
>> +
>	RTE_ETH_QUEUE_STATE_STARTED;
>> +				} else {
>> +					TESTPMD_LOG(WARNING,
>> +						"Failed to get rx queue
>info\n");
>> +				}
>> +
>> +				rc = rte_eth_tx_queue_info_get(fs->tx_port,
>> +						fs->tx_queue, &tx_qinfo);
>> +				if (rc == 0) {
>> +					ports[fs->tx_port].txq[fs-
>>tx_queue].state =
>> +						tx_qinfo.queue_state;
>> +				} else if (rc == -ENOTSUP) {
>> +					/* Set the txq state to
>RTE_ETH_QUEUE_STATE_STARTED
>> +					 * to ensure that the PMDs do not
>implement
>> +					 * rte_eth_tx_queue_info_get can
>forward.
>> +					 */
>> +					ports[fs->tx_port].txq[fs-
>>tx_queue].state =
>> +
>	RTE_ETH_QUEUE_STATE_STARTED;
>> +				} else {
>> +					TESTPMD_LOG(WARNING,
>> +						"Failed to get tx queue
>info\n");
>> +				}
>> +			}
>>  			stream_init(fwd_streams[i]);
>> +		}
>> +	}
>>
>
>
>Testpmd duplicates some dpdk/ethdev state/config in application level, and
>this can bite in multiple cases, as it is happening here.
>
>I am not sure if this was a design decision, but I think instead of testpmd
>storing ethdev related state/config in application level, it should store only
>application level state/config, and when ethdev related state/config is
>required app should get it directly from ethdev.
>
>It may be too late already for testpmd, there is a mixed usage, but I am for
>preferring this approach when there is an opportunity.
>
>
>
>For above issue, why queue state needs to be stored in application level 'port'
>variable?
>Where is this queue state used?
>
>Can it work to get queue state directly from ethdev where this state is used,
>instead of storing it in the 'port' variable in advance?
>
>And perhaps testpmd 'port' variable can be updated there, both for primary
>and secondary, for backward compatibility (other existing users of this queue
>state).
>
>What do you think?

Thanks for your comments!

It is an effective method to get queue state directly from ethdev where this state is used.
I also don't know the design meaning of the 'ports' variable. If modification is needed,
a higher level of design and more work are required.

As a bug fix, apart from extracting the code block into a function, is the solution feasible?
  
Ferruh Yigit March 7, 2023, 11:41 a.m. UTC | #5
On 3/7/2023 3:25 AM, He, ShiyangX wrote:
> 
> 
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit@amd.com>
>> Sent: Monday, March 6, 2023 11:06 PM
>> To: He, ShiyangX <shiyangx.he@intel.com>; dev@dpdk.org
>> Cc: Zhou, YidingX <yidingx.zhou@intel.com>; stable@dpdk.org; Zhang, Yuying
>> <yuying.zhang@intel.com>; Singh, Aman Deep
>> <aman.deep.singh@intel.com>; Burakov, Anatoly
>> <anatoly.burakov@intel.com>; Matan Azrad <matan@nvidia.com>; Dmitry
>> Kozlyuk <dmitry.kozliuk@gmail.com>
>> Subject: Re: [PATCH v3] app/testpmd: fix secondary process not forwarding
>>
>> On 2/23/2023 2:41 PM, Shiyang He wrote:
>>> Under multi-process scenario, the secondary process gets queue state
>>> from the wrong location (the global variable 'ports'). Therefore, the
>>> secondary process can not forward since "stream_init" is not called.
>>>
>>> This commit fixes the issue by calling 'rte_eth_rx/tx_queue_info_get'
>>> to get queue state from shared memory.
>>>
>>> Fixes: 3c4426db54fc ("app/testpmd: do not poll stopped queues")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Shiyang He <shiyangx.he@intel.com>
>>> Acked-by: Yuying Zhang <yuying.zhang@intel.com>
>>>
>>> v3: Add return value description
>>> ---
>>>  app/test-pmd/testpmd.c | 45
>>> ++++++++++++++++++++++++++++++++++++++++--
>>>  1 file changed, 43 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
>>> 0c14325b8d..a050472aea 100644
>>> --- a/app/test-pmd/testpmd.c
>>> +++ b/app/test-pmd/testpmd.c
>>> @@ -2418,9 +2418,50 @@ 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_SECONDARY)
>> {
>>> +				struct fwd_stream *fs = fwd_streams[i];
>>> +				struct rte_eth_rxq_info rx_qinfo;
>>> +				struct rte_eth_txq_info tx_qinfo;
>>> +				int32_t rc;
>>> +				rc = rte_eth_rx_queue_info_get(fs->rx_port,
>>> +						fs->rx_queue, &rx_qinfo);
>>> +				if (rc == 0) {
>>> +					ports[fs->rx_port].rxq[fs-
>>> rx_queue].state =
>>> +						rx_qinfo.queue_state;
>>> +				} else if (rc == -ENOTSUP) {
>>> +					/* Set the rxq state to
>> RTE_ETH_QUEUE_STATE_STARTED
>>> +					 * to ensure that the PMDs do not
>> implement
>>> +					 * rte_eth_rx_queue_info_get can
>> forward.
>>> +					 */
>>> +					ports[fs->rx_port].rxq[fs-
>>> rx_queue].state =
>>> +
>> 	RTE_ETH_QUEUE_STATE_STARTED;
>>> +				} else {
>>> +					TESTPMD_LOG(WARNING,
>>> +						"Failed to get rx queue
>> info\n");
>>> +				}
>>> +
>>> +				rc = rte_eth_tx_queue_info_get(fs->tx_port,
>>> +						fs->tx_queue, &tx_qinfo);
>>> +				if (rc == 0) {
>>> +					ports[fs->tx_port].txq[fs-
>>> tx_queue].state =
>>> +						tx_qinfo.queue_state;
>>> +				} else if (rc == -ENOTSUP) {
>>> +					/* Set the txq state to
>> RTE_ETH_QUEUE_STATE_STARTED
>>> +					 * to ensure that the PMDs do not
>> implement
>>> +					 * rte_eth_tx_queue_info_get can
>> forward.
>>> +					 */
>>> +					ports[fs->tx_port].txq[fs-
>>> tx_queue].state =
>>> +
>> 	RTE_ETH_QUEUE_STATE_STARTED;
>>> +				} else {
>>> +					TESTPMD_LOG(WARNING,
>>> +						"Failed to get tx queue
>> info\n");
>>> +				}
>>> +			}
>>>  			stream_init(fwd_streams[i]);
>>> +		}
>>> +	}
>>>
>>
>>
>> Testpmd duplicates some dpdk/ethdev state/config in application level, and
>> this can bite in multiple cases, as it is happening here.
>>
>> I am not sure if this was a design decision, but I think instead of testpmd
>> storing ethdev related state/config in application level, it should store only
>> application level state/config, and when ethdev related state/config is
>> required app should get it directly from ethdev.
>>
>> It may be too late already for testpmd, there is a mixed usage, but I am for
>> preferring this approach when there is an opportunity.
>>
>>
>>
>> For above issue, why queue state needs to be stored in application level 'port'
>> variable?
>> Where is this queue state used?
>>
>> Can it work to get queue state directly from ethdev where this state is used,
>> instead of storing it in the 'port' variable in advance?
>>
>> And perhaps testpmd 'port' variable can be updated there, both for primary
>> and secondary, for backward compatibility (other existing users of this queue
>> state).
>>
>> What do you think?
> 
> Thanks for your comments!
> 
> It is an effective method to get queue state directly from ethdev where this state is used.
> I also don't know the design meaning of the 'ports' variable. If modification is needed,
> a higher level of design and more work are required.
> 
> As a bug fix, apart from extracting the code block into a function, is the solution feasible?

Hi Shiyang,

As a bug fix, this issue (testpmd stored state not being up to date for
secondary process) looks like have potential to occur many different
flavors, that is why what about having a central update?

I think 'start_port()' can be a good place for this kind of update:

start_port() {

	...
	if (secondary)
		update_state()
}

update_state() {
	update_queue_state()
}

update_queue_state() {
	<your code goes here>
}


Having secondary checks and updates in multiple places can make code
harder to understand.

What do you think to update as above?
  
Shiyang He March 8, 2023, 2:05 a.m. UTC | #6
>-----Original Message-----
>From: Ferruh Yigit <ferruh.yigit@amd.com>
>Sent: Tuesday, March 7, 2023 7:41 PM
>To: He, ShiyangX <shiyangx.he@intel.com>; dev@dpdk.org
>Cc: Zhou, YidingX <yidingx.zhou@intel.com>; stable@dpdk.org; Zhang, Yuying
><yuying.zhang@intel.com>; Singh, Aman Deep
><aman.deep.singh@intel.com>; Burakov, Anatoly
><anatoly.burakov@intel.com>; Matan Azrad <matan@nvidia.com>; Dmitry
>Kozlyuk <dmitry.kozliuk@gmail.com>
>Subject: Re: [PATCH v3] app/testpmd: fix secondary process not forwarding
>
>On 3/7/2023 3:25 AM, He, ShiyangX wrote:
>>
>>
>>> -----Original Message-----
>>> From: Ferruh Yigit <ferruh.yigit@amd.com>
>>> Sent: Monday, March 6, 2023 11:06 PM
>>> To: He, ShiyangX <shiyangx.he@intel.com>; dev@dpdk.org
>>> Cc: Zhou, YidingX <yidingx.zhou@intel.com>; stable@dpdk.org; Zhang,
>>> Yuying <yuying.zhang@intel.com>; Singh, Aman Deep
>>> <aman.deep.singh@intel.com>; Burakov, Anatoly
>>> <anatoly.burakov@intel.com>; Matan Azrad <matan@nvidia.com>; Dmitry
>>> Kozlyuk <dmitry.kozliuk@gmail.com>
>>> Subject: Re: [PATCH v3] app/testpmd: fix secondary process not
>>> forwarding
>>>
>>> On 2/23/2023 2:41 PM, Shiyang He wrote:
>>>> Under multi-process scenario, the secondary process gets queue state
>>>> from the wrong location (the global variable 'ports'). Therefore,
>>>> the secondary process can not forward since "stream_init" is not called.
>>>>
>>>> This commit fixes the issue by calling 'rte_eth_rx/tx_queue_info_get'
>>>> to get queue state from shared memory.
>>>>
>>>> Fixes: 3c4426db54fc ("app/testpmd: do not poll stopped queues")
>>>> Cc: stable@dpdk.org
>>>>
>>>> Signed-off-by: Shiyang He <shiyangx.he@intel.com>
>>>> Acked-by: Yuying Zhang <yuying.zhang@intel.com>
>>>>
>>>> v3: Add return value description
>>>> ---
>>>>  app/test-pmd/testpmd.c | 45
>>>> ++++++++++++++++++++++++++++++++++++++++--
>>>>  1 file changed, 43 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
>>>> 0c14325b8d..a050472aea 100644
>>>> --- a/app/test-pmd/testpmd.c
>>>> +++ b/app/test-pmd/testpmd.c
>>>> @@ -2418,9 +2418,50 @@ 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_SECONDARY)
>>> {
>>>> +				struct fwd_stream *fs = fwd_streams[i];
>>>> +				struct rte_eth_rxq_info rx_qinfo;
>>>> +				struct rte_eth_txq_info tx_qinfo;
>>>> +				int32_t rc;
>>>> +				rc = rte_eth_rx_queue_info_get(fs->rx_port,
>>>> +						fs->rx_queue, &rx_qinfo);
>>>> +				if (rc == 0) {
>>>> +					ports[fs->rx_port].rxq[fs-
>>>> rx_queue].state =
>>>> +						rx_qinfo.queue_state;
>>>> +				} else if (rc == -ENOTSUP) {
>>>> +					/* Set the rxq state to
>>> RTE_ETH_QUEUE_STATE_STARTED
>>>> +					 * to ensure that the PMDs do not
>>> implement
>>>> +					 * rte_eth_rx_queue_info_get can
>>> forward.
>>>> +					 */
>>>> +					ports[fs->rx_port].rxq[fs-
>>>> rx_queue].state =
>>>> +
>>> 	RTE_ETH_QUEUE_STATE_STARTED;
>>>> +				} else {
>>>> +					TESTPMD_LOG(WARNING,
>>>> +						"Failed to get rx queue
>>> info\n");
>>>> +				}
>>>> +
>>>> +				rc = rte_eth_tx_queue_info_get(fs->tx_port,
>>>> +						fs->tx_queue, &tx_qinfo);
>>>> +				if (rc == 0) {
>>>> +					ports[fs->tx_port].txq[fs-
>>>> tx_queue].state =
>>>> +						tx_qinfo.queue_state;
>>>> +				} else if (rc == -ENOTSUP) {
>>>> +					/* Set the txq state to
>>> RTE_ETH_QUEUE_STATE_STARTED
>>>> +					 * to ensure that the PMDs do not
>>> implement
>>>> +					 * rte_eth_tx_queue_info_get can
>>> forward.
>>>> +					 */
>>>> +					ports[fs->tx_port].txq[fs-
>>>> tx_queue].state =
>>>> +
>>> 	RTE_ETH_QUEUE_STATE_STARTED;
>>>> +				} else {
>>>> +					TESTPMD_LOG(WARNING,
>>>> +						"Failed to get tx queue
>>> info\n");
>>>> +				}
>>>> +			}
>>>>  			stream_init(fwd_streams[i]);
>>>> +		}
>>>> +	}
>>>>
>>>
>>>
>>> Testpmd duplicates some dpdk/ethdev state/config in application
>>> level, and this can bite in multiple cases, as it is happening here.
>>>
>>> I am not sure if this was a design decision, but I think instead of
>>> testpmd storing ethdev related state/config in application level, it
>>> should store only application level state/config, and when ethdev
>>> related state/config is required app should get it directly from ethdev.
>>>
>>> It may be too late already for testpmd, there is a mixed usage, but I
>>> am for preferring this approach when there is an opportunity.
>>>
>>>
>>>
>>> For above issue, why queue state needs to be stored in application level
>'port'
>>> variable?
>>> Where is this queue state used?
>>>
>>> Can it work to get queue state directly from ethdev where this state
>>> is used, instead of storing it in the 'port' variable in advance?
>>>
>>> And perhaps testpmd 'port' variable can be updated there, both for
>>> primary and secondary, for backward compatibility (other existing
>>> users of this queue state).
>>>
>>> What do you think?
>>
>> Thanks for your comments!
>>
>> It is an effective method to get queue state directly from ethdev where this
>state is used.
>> I also don't know the design meaning of the 'ports' variable. If
>> modification is needed, a higher level of design and more work are required.
>>
>> As a bug fix, apart from extracting the code block into a function, is the
>solution feasible?
>
>Hi Shiyang,
>
>As a bug fix, this issue (testpmd stored state not being up to date for
>secondary process) looks like have potential to occur many different flavors,
>that is why what about having a central update?
>
>I think 'start_port()' can be a good place for this kind of update:
>
>start_port() {
>
>	...
>	if (secondary)
>		update_state()
>}
>
>update_state() {
>	update_queue_state()
>}
>
>update_queue_state() {
>	<your code goes here>
>}
>
>
>Having secondary checks and updates in multiple places can make code harder
>to understand.
>
>What do you think to update as above?
>
>
>

Thanks for your reply! 
It is more reasonable to update the queue state in 'start_port()'. I'll send a new patch asap.
  
lihuisong (C) March 8, 2023, 2:54 a.m. UTC | #7
在 2023/3/8 10:05, He, ShiyangX 写道:
>
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit@amd.com>
>> Sent: Tuesday, March 7, 2023 7:41 PM
>> To: He, ShiyangX <shiyangx.he@intel.com>; dev@dpdk.org
>> Cc: Zhou, YidingX <yidingx.zhou@intel.com>; stable@dpdk.org; Zhang, Yuying
>> <yuying.zhang@intel.com>; Singh, Aman Deep
>> <aman.deep.singh@intel.com>; Burakov, Anatoly
>> <anatoly.burakov@intel.com>; Matan Azrad <matan@nvidia.com>; Dmitry
>> Kozlyuk <dmitry.kozliuk@gmail.com>
>> Subject: Re: [PATCH v3] app/testpmd: fix secondary process not forwarding
>>
>> On 3/7/2023 3:25 AM, He, ShiyangX wrote:
>>>
>>>> -----Original Message-----
>>>> From: Ferruh Yigit <ferruh.yigit@amd.com>
>>>> Sent: Monday, March 6, 2023 11:06 PM
>>>> To: He, ShiyangX <shiyangx.he@intel.com>; dev@dpdk.org
>>>> Cc: Zhou, YidingX <yidingx.zhou@intel.com>; stable@dpdk.org; Zhang,
>>>> Yuying <yuying.zhang@intel.com>; Singh, Aman Deep
>>>> <aman.deep.singh@intel.com>; Burakov, Anatoly
>>>> <anatoly.burakov@intel.com>; Matan Azrad <matan@nvidia.com>; Dmitry
>>>> Kozlyuk <dmitry.kozliuk@gmail.com>
>>>> Subject: Re: [PATCH v3] app/testpmd: fix secondary process not
>>>> forwarding
>>>>
>>>> On 2/23/2023 2:41 PM, Shiyang He wrote:
>>>>> Under multi-process scenario, the secondary process gets queue state
>>>>> from the wrong location (the global variable 'ports'). Therefore,
>>>>> the secondary process can not forward since "stream_init" is not called.
>>>>>
>>>>> This commit fixes the issue by calling 'rte_eth_rx/tx_queue_info_get'
>>>>> to get queue state from shared memory.
>>>>>
>>>>> Fixes: 3c4426db54fc ("app/testpmd: do not poll stopped queues")
>>>>> Cc: stable@dpdk.org
>>>>>
>>>>> Signed-off-by: Shiyang He <shiyangx.he@intel.com>
>>>>> Acked-by: Yuying Zhang <yuying.zhang@intel.com>
>>>>>
>>>>> v3: Add return value description
>>>>> ---
>>>>>   app/test-pmd/testpmd.c | 45
>>>>> ++++++++++++++++++++++++++++++++++++++++--
>>>>>   1 file changed, 43 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
>>>>> 0c14325b8d..a050472aea 100644
>>>>> --- a/app/test-pmd/testpmd.c
>>>>> +++ b/app/test-pmd/testpmd.c
>>>>> @@ -2418,9 +2418,50 @@ 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_SECONDARY)
>>>> {
>>>>> +				struct fwd_stream *fs = fwd_streams[i];
>>>>> +				struct rte_eth_rxq_info rx_qinfo;
>>>>> +				struct rte_eth_txq_info tx_qinfo;
>>>>> +				int32_t rc;
>>>>> +				rc = rte_eth_rx_queue_info_get(fs->rx_port,
>>>>> +						fs->rx_queue, &rx_qinfo);
>>>>> +				if (rc == 0) {
>>>>> +					ports[fs->rx_port].rxq[fs-
>>>>> rx_queue].state =
>>>>> +						rx_qinfo.queue_state;
>>>>> +				} else if (rc == -ENOTSUP) {
>>>>> +					/* Set the rxq state to
>>>> RTE_ETH_QUEUE_STATE_STARTED
>>>>> +					 * to ensure that the PMDs do not
>>>> implement
>>>>> +					 * rte_eth_rx_queue_info_get can
>>>> forward.
>>>>> +					 */
>>>>> +					ports[fs->rx_port].rxq[fs-
>>>>> rx_queue].state =
>>>>> +
>>>> 	RTE_ETH_QUEUE_STATE_STARTED;
>>>>> +				} else {
>>>>> +					TESTPMD_LOG(WARNING,
>>>>> +						"Failed to get rx queue
>>>> info\n");
>>>>> +				}
>>>>> +
>>>>> +				rc = rte_eth_tx_queue_info_get(fs->tx_port,
>>>>> +						fs->tx_queue, &tx_qinfo);
>>>>> +				if (rc == 0) {
>>>>> +					ports[fs->tx_port].txq[fs-
>>>>> tx_queue].state =
>>>>> +						tx_qinfo.queue_state;
>>>>> +				} else if (rc == -ENOTSUP) {
>>>>> +					/* Set the txq state to
>>>> RTE_ETH_QUEUE_STATE_STARTED
>>>>> +					 * to ensure that the PMDs do not
>>>> implement
>>>>> +					 * rte_eth_tx_queue_info_get can
>>>> forward.
>>>>> +					 */
>>>>> +					ports[fs->tx_port].txq[fs-
>>>>> tx_queue].state =
>>>>> +
>>>> 	RTE_ETH_QUEUE_STATE_STARTED;
>>>>> +				} else {
>>>>> +					TESTPMD_LOG(WARNING,
>>>>> +						"Failed to get tx queue
>>>> info\n");
>>>>> +				}
>>>>> +			}
>>>>>   			stream_init(fwd_streams[i]);
>>>>> +		}
>>>>> +	}
>>>>>
>>>>
>>>> Testpmd duplicates some dpdk/ethdev state/config in application
>>>> level, and this can bite in multiple cases, as it is happening here.
>>>>
>>>> I am not sure if this was a design decision, but I think instead of
>>>> testpmd storing ethdev related state/config in application level, it
>>>> should store only application level state/config, and when ethdev
>>>> related state/config is required app should get it directly from ethdev.
>>>>
>>>> It may be too late already for testpmd, there is a mixed usage, but I
>>>> am for preferring this approach when there is an opportunity.
>>>>
>>>>
>>>>
>>>> For above issue, why queue state needs to be stored in application level
>> 'port'
>>>> variable?
>>>> Where is this queue state used?
>>>>
>>>> Can it work to get queue state directly from ethdev where this state
>>>> is used, instead of storing it in the 'port' variable in advance?
>>>>
>>>> And perhaps testpmd 'port' variable can be updated there, both for
>>>> primary and secondary, for backward compatibility (other existing
>>>> users of this queue state).
>>>>
>>>> What do you think?
>>> Thanks for your comments!
>>>
>>> It is an effective method to get queue state directly from ethdev where this
>> state is used.
>>> I also don't know the design meaning of the 'ports' variable. If
>>> modification is needed, a higher level of design and more work are required.
>>>
>>> As a bug fix, apart from extracting the code block into a function, is the
>> solution feasible?
>>
>> Hi Shiyang,
>>
>> As a bug fix, this issue (testpmd stored state not being up to date for
>> secondary process) looks like have potential to occur many different flavors,
>> that is why what about having a central update?
>>
>> I think 'start_port()' can be a good place for this kind of update:
>>
>> start_port() {
>>
>> 	...
>> 	if (secondary)
>> 		update_state()
>> }
>>
>> update_state() {
>> 	update_queue_state()
>> }
>>
>> update_queue_state() {
>> 	<your code goes here>
>> }
>>
>>
>> Having secondary checks and updates in multiple places can make code harder
>> to understand.
>>
>> What do you think to update as above?
>>
>>
>>
> Thanks for your reply!
> It is more reasonable to update the queue state in 'start_port()'. I'll send a new patch asap.
It's also necessary to update the queue state when start forwarding.
Because the state may be changed after start port.
The state cannot be updated in real time(because of no notification), 
but it's helpful for secondary.
  
Ferruh Yigit March 8, 2023, 9:54 a.m. UTC | #8
On 3/8/2023 2:54 AM, lihuisong (C) wrote:
> 
> 在 2023/3/8 10:05, He, ShiyangX 写道:
>>
>>> -----Original Message-----
>>> From: Ferruh Yigit <ferruh.yigit@amd.com>
>>> Sent: Tuesday, March 7, 2023 7:41 PM
>>> To: He, ShiyangX <shiyangx.he@intel.com>; dev@dpdk.org
>>> Cc: Zhou, YidingX <yidingx.zhou@intel.com>; stable@dpdk.org; Zhang,
>>> Yuying
>>> <yuying.zhang@intel.com>; Singh, Aman Deep
>>> <aman.deep.singh@intel.com>; Burakov, Anatoly
>>> <anatoly.burakov@intel.com>; Matan Azrad <matan@nvidia.com>; Dmitry
>>> Kozlyuk <dmitry.kozliuk@gmail.com>
>>> Subject: Re: [PATCH v3] app/testpmd: fix secondary process not
>>> forwarding
>>>
>>> On 3/7/2023 3:25 AM, He, ShiyangX wrote:
>>>>
>>>>> -----Original Message-----
>>>>> From: Ferruh Yigit <ferruh.yigit@amd.com>
>>>>> Sent: Monday, March 6, 2023 11:06 PM
>>>>> To: He, ShiyangX <shiyangx.he@intel.com>; dev@dpdk.org
>>>>> Cc: Zhou, YidingX <yidingx.zhou@intel.com>; stable@dpdk.org; Zhang,
>>>>> Yuying <yuying.zhang@intel.com>; Singh, Aman Deep
>>>>> <aman.deep.singh@intel.com>; Burakov, Anatoly
>>>>> <anatoly.burakov@intel.com>; Matan Azrad <matan@nvidia.com>; Dmitry
>>>>> Kozlyuk <dmitry.kozliuk@gmail.com>
>>>>> Subject: Re: [PATCH v3] app/testpmd: fix secondary process not
>>>>> forwarding
>>>>>
>>>>> On 2/23/2023 2:41 PM, Shiyang He wrote:
>>>>>> Under multi-process scenario, the secondary process gets queue state
>>>>>> from the wrong location (the global variable 'ports'). Therefore,
>>>>>> the secondary process can not forward since "stream_init" is not
>>>>>> called.
>>>>>>
>>>>>> This commit fixes the issue by calling 'rte_eth_rx/tx_queue_info_get'
>>>>>> to get queue state from shared memory.
>>>>>>
>>>>>> Fixes: 3c4426db54fc ("app/testpmd: do not poll stopped queues")
>>>>>> Cc: stable@dpdk.org
>>>>>>
>>>>>> Signed-off-by: Shiyang He <shiyangx.he@intel.com>
>>>>>> Acked-by: Yuying Zhang <yuying.zhang@intel.com>
>>>>>>
>>>>>> v3: Add return value description
>>>>>> ---
>>>>>>   app/test-pmd/testpmd.c | 45
>>>>>> ++++++++++++++++++++++++++++++++++++++++--
>>>>>>   1 file changed, 43 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
>>>>>> 0c14325b8d..a050472aea 100644
>>>>>> --- a/app/test-pmd/testpmd.c
>>>>>> +++ b/app/test-pmd/testpmd.c
>>>>>> @@ -2418,9 +2418,50 @@ 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_SECONDARY)
>>>>> {
>>>>>> +                struct fwd_stream *fs = fwd_streams[i];
>>>>>> +                struct rte_eth_rxq_info rx_qinfo;
>>>>>> +                struct rte_eth_txq_info tx_qinfo;
>>>>>> +                int32_t rc;
>>>>>> +                rc = rte_eth_rx_queue_info_get(fs->rx_port,
>>>>>> +                        fs->rx_queue, &rx_qinfo);
>>>>>> +                if (rc == 0) {
>>>>>> +                    ports[fs->rx_port].rxq[fs-
>>>>>> rx_queue].state =
>>>>>> +                        rx_qinfo.queue_state;
>>>>>> +                } else if (rc == -ENOTSUP) {
>>>>>> +                    /* Set the rxq state to
>>>>> RTE_ETH_QUEUE_STATE_STARTED
>>>>>> +                     * to ensure that the PMDs do not
>>>>> implement
>>>>>> +                     * rte_eth_rx_queue_info_get can
>>>>> forward.
>>>>>> +                     */
>>>>>> +                    ports[fs->rx_port].rxq[fs-
>>>>>> rx_queue].state =
>>>>>> +
>>>>>     RTE_ETH_QUEUE_STATE_STARTED;
>>>>>> +                } else {
>>>>>> +                    TESTPMD_LOG(WARNING,
>>>>>> +                        "Failed to get rx queue
>>>>> info\n");
>>>>>> +                }
>>>>>> +
>>>>>> +                rc = rte_eth_tx_queue_info_get(fs->tx_port,
>>>>>> +                        fs->tx_queue, &tx_qinfo);
>>>>>> +                if (rc == 0) {
>>>>>> +                    ports[fs->tx_port].txq[fs-
>>>>>> tx_queue].state =
>>>>>> +                        tx_qinfo.queue_state;
>>>>>> +                } else if (rc == -ENOTSUP) {
>>>>>> +                    /* Set the txq state to
>>>>> RTE_ETH_QUEUE_STATE_STARTED
>>>>>> +                     * to ensure that the PMDs do not
>>>>> implement
>>>>>> +                     * rte_eth_tx_queue_info_get can
>>>>> forward.
>>>>>> +                     */
>>>>>> +                    ports[fs->tx_port].txq[fs-
>>>>>> tx_queue].state =
>>>>>> +
>>>>>     RTE_ETH_QUEUE_STATE_STARTED;
>>>>>> +                } else {
>>>>>> +                    TESTPMD_LOG(WARNING,
>>>>>> +                        "Failed to get tx queue
>>>>> info\n");
>>>>>> +                }
>>>>>> +            }
>>>>>>               stream_init(fwd_streams[i]);
>>>>>> +        }
>>>>>> +    }
>>>>>>
>>>>>
>>>>> Testpmd duplicates some dpdk/ethdev state/config in application
>>>>> level, and this can bite in multiple cases, as it is happening here.
>>>>>
>>>>> I am not sure if this was a design decision, but I think instead of
>>>>> testpmd storing ethdev related state/config in application level, it
>>>>> should store only application level state/config, and when ethdev
>>>>> related state/config is required app should get it directly from
>>>>> ethdev.
>>>>>
>>>>> It may be too late already for testpmd, there is a mixed usage, but I
>>>>> am for preferring this approach when there is an opportunity.
>>>>>
>>>>>
>>>>>
>>>>> For above issue, why queue state needs to be stored in application
>>>>> level
>>> 'port'
>>>>> variable?
>>>>> Where is this queue state used?
>>>>>
>>>>> Can it work to get queue state directly from ethdev where this state
>>>>> is used, instead of storing it in the 'port' variable in advance?
>>>>>
>>>>> And perhaps testpmd 'port' variable can be updated there, both for
>>>>> primary and secondary, for backward compatibility (other existing
>>>>> users of this queue state).
>>>>>
>>>>> What do you think?
>>>> Thanks for your comments!
>>>>
>>>> It is an effective method to get queue state directly from ethdev
>>>> where this
>>> state is used.
>>>> I also don't know the design meaning of the 'ports' variable. If
>>>> modification is needed, a higher level of design and more work are
>>>> required.
>>>>
>>>> As a bug fix, apart from extracting the code block into a function,
>>>> is the
>>> solution feasible?
>>>
>>> Hi Shiyang,
>>>
>>> As a bug fix, this issue (testpmd stored state not being up to date for
>>> secondary process) looks like have potential to occur many different
>>> flavors,
>>> that is why what about having a central update?
>>>
>>> I think 'start_port()' can be a good place for this kind of update:
>>>
>>> start_port() {
>>>
>>>     ...
>>>     if (secondary)
>>>         update_state()
>>> }
>>>
>>> update_state() {
>>>     update_queue_state()
>>> }
>>>
>>> update_queue_state() {
>>>     <your code goes here>
>>> }
>>>
>>>
>>> Having secondary checks and updates in multiple places can make code
>>> harder
>>> to understand.
>>>
>>> What do you think to update as above?
>>>
>>>
>>>
>> Thanks for your reply!
>> It is more reasonable to update the queue state in 'start_port()'.
>> I'll send a new patch asap.
> It's also necessary to update the queue state when start forwarding.
> Because the state may be changed after start port.


I was hoping updating on a single point can be sufficient, is this
needed because of testpmd commands?

> The state cannot be updated in real time(because of no notification),
> but it's helpful for secondary.
  

Patch

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 0c14325b8d..a050472aea 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -2418,9 +2418,50 @@  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_SECONDARY) {
+				struct fwd_stream *fs = fwd_streams[i];
+				struct rte_eth_rxq_info rx_qinfo;
+				struct rte_eth_txq_info tx_qinfo;
+				int32_t rc;
+				rc = rte_eth_rx_queue_info_get(fs->rx_port,
+						fs->rx_queue, &rx_qinfo);
+				if (rc == 0) {
+					ports[fs->rx_port].rxq[fs->rx_queue].state =
+						rx_qinfo.queue_state;
+				} else if (rc == -ENOTSUP) {
+					/* Set the rxq state to RTE_ETH_QUEUE_STATE_STARTED
+					 * to ensure that the PMDs do not implement
+					 * rte_eth_rx_queue_info_get can forward.
+					 */
+					ports[fs->rx_port].rxq[fs->rx_queue].state =
+						RTE_ETH_QUEUE_STATE_STARTED;
+				} else {
+					TESTPMD_LOG(WARNING,
+						"Failed to get rx queue info\n");
+				}
+
+				rc = rte_eth_tx_queue_info_get(fs->tx_port,
+						fs->tx_queue, &tx_qinfo);
+				if (rc == 0) {
+					ports[fs->tx_port].txq[fs->tx_queue].state =
+						tx_qinfo.queue_state;
+				} else if (rc == -ENOTSUP) {
+					/* Set the txq state to RTE_ETH_QUEUE_STATE_STARTED
+					 * to ensure that the PMDs do not implement
+					 * rte_eth_tx_queue_info_get can forward.
+					 */
+					ports[fs->tx_port].txq[fs->tx_queue].state =
+						RTE_ETH_QUEUE_STATE_STARTED;
+				} else {
+					TESTPMD_LOG(WARNING,
+						"Failed to get tx queue info\n");
+				}
+			}
 			stream_init(fwd_streams[i]);
+		}
+	}
 
 	port_fwd_begin = cur_fwd_config.fwd_eng->port_fwd_begin;
 	if (port_fwd_begin != NULL) {