* [PATCH] app/testpmd: fix secondary process not forwarding
@ 2022-12-30 7:55 Shiyang He
2022-12-30 17:23 ` Stephen Hemminger
` (3 more replies)
0 siblings, 4 replies; 24+ messages in thread
From: Shiyang He @ 2022-12-30 7:55 UTC (permalink / raw)
To: dev
Cc: yidingx.zhou, Shiyang He, stable, Aman Singh, Yuying Zhang,
Anatoly Burakov, Xiaoyun Li, Alvin Zhang
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: a78040c990cb ("app/testpmd: update forward engine beginning")
Cc: stable@dpdk.org
Signed-off-by: Shiyang He <shiyangx.he@intel.com>
---
app/test-pmd/testpmd.c | 29 +++++++++++++++++++++++++++--
1 file changed, 27 insertions(+), 2 deletions(-)
diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 134d79a555..2c73daf9eb 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -2378,9 +2378,34 @@ 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_PRIMARY) {
+ 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)
+ ports[fs->rx_port].rxq[fs->rx_queue].state =
+ rx_qinfo.queue_state;
+ 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)
+ ports[fs->tx_port].txq[fs->tx_queue].state =
+ tx_qinfo.queue_state;
+ 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) {
--
2.34.1
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] app/testpmd: fix secondary process not forwarding
2022-12-30 7:55 [PATCH] app/testpmd: fix secondary process not forwarding Shiyang He
@ 2022-12-30 17:23 ` Stephen Hemminger
2023-01-04 2:02 ` He, ShiyangX
2023-02-20 12:45 ` lihuisong (C)
` (2 subsequent siblings)
3 siblings, 1 reply; 24+ messages in thread
From: Stephen Hemminger @ 2022-12-30 17:23 UTC (permalink / raw)
To: Shiyang He
Cc: dev, yidingx.zhou, stable, Aman Singh, Yuying Zhang,
Anatoly Burakov, Xiaoyun Li, Alvin Zhang
On Fri, 30 Dec 2022 07:55:53 +0000
Shiyang He <shiyangx.he@intel.com> 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: a78040c990cb ("app/testpmd: update forward engine beginning")
> Cc: stable@dpdk.org
>
> Signed-off-by: Shiyang He <shiyangx.he@intel.com>
Would it be possible to fix this the initialization
of ports variable, rather than doing a per-state fixup here?
^ permalink raw reply [flat|nested] 24+ messages in thread
* RE: [PATCH] app/testpmd: fix secondary process not forwarding
2022-12-30 17:23 ` Stephen Hemminger
@ 2023-01-04 2:02 ` He, ShiyangX
2023-01-13 9:07 ` He, ShiyangX
0 siblings, 1 reply; 24+ messages in thread
From: He, ShiyangX @ 2023-01-04 2:02 UTC (permalink / raw)
To: Stephen Hemminger
Cc: dev, Zhou, YidingX, stable, Singh, Aman Deep, Zhang, Yuying,
Burakov, Anatoly, Li, Xiaoyun, Alvin Zhang
>> 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: a78040c990cb ("app/testpmd: update forward engine beginning")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Shiyang He <shiyangx.he@intel.com>
>
>Would it be possible to fix this the initialization of ports variable, rather than
>doing a per-state fixup here?
In multi-process scenario, the secondary process does not initialize the queue
state in the 'ports' variable, and the ethdev's queue state may be changed by
any other process, which causes 'ports' queue state of per-process and ethdev's
queue state are inconsistent. Therefore, getting the queue state from ethdev is a
feasible way which I can think of.
^ permalink raw reply [flat|nested] 24+ messages in thread
* RE: [PATCH] app/testpmd: fix secondary process not forwarding
2023-01-04 2:02 ` He, ShiyangX
@ 2023-01-13 9:07 ` He, ShiyangX
2023-02-08 3:22 ` Zhang, Yuying
0 siblings, 1 reply; 24+ messages in thread
From: He, ShiyangX @ 2023-01-13 9:07 UTC (permalink / raw)
To: He, ShiyangX, Zhang, Yuying
Cc: dev, Zhou, YidingX, stable, Singh, Aman Deep, Burakov, Anatoly,
Li, Xiaoyun, Alvin Zhang
@Zhang, Yuying Hi, please take a look at this patch! Are there any comments?
>-----Original Message-----
>From: He, ShiyangX <shiyangx.he@intel.com>
>Sent: Wednesday, January 4, 2023 10:02 AM
>To: Stephen Hemminger <stephen@networkplumber.org>
>Cc: dev@dpdk.org; Zhou, YidingX <yidingx.zhou@intel.com>;
>stable@dpdk.org; Singh, Aman Deep <aman.deep.singh@intel.com>; Zhang,
>Yuying <yuying.zhang@intel.com>; Burakov, Anatoly
><anatoly.burakov@intel.com>; Li, Xiaoyun <xiaoyun.li@intel.com>; Alvin
>Zhang <alvinx.zhang@intel.com>
>Subject: RE: [PATCH] app/testpmd: fix secondary process not forwarding
>
>>> 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: a78040c990cb ("app/testpmd: update forward engine beginning")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Shiyang He <shiyangx.he@intel.com>
>>
>>Would it be possible to fix this the initialization of ports variable,
>>rather than doing a per-state fixup here?
>
>In multi-process scenario, the secondary process does not initialize the queue
>state in the 'ports' variable, and the ethdev's queue state may be changed by
>any other process, which causes 'ports' queue state of per-process and
>ethdev's queue state are inconsistent. Therefore, getting the queue state
>from ethdev is a feasible way which I can think of.
^ permalink raw reply [flat|nested] 24+ messages in thread
* RE: [PATCH] app/testpmd: fix secondary process not forwarding
2023-01-13 9:07 ` He, ShiyangX
@ 2023-02-08 3:22 ` Zhang, Yuying
2023-02-08 6:38 ` He, ShiyangX
0 siblings, 1 reply; 24+ messages in thread
From: Zhang, Yuying @ 2023-02-08 3:22 UTC (permalink / raw)
To: He, ShiyangX
Cc: dev, Zhou, YidingX, stable, Singh, Aman Deep, Burakov, Anatoly,
Li, Xiaoyun, Alvin Zhang
Hi Shiyang,
> -----Original Message-----
> From: He, ShiyangX <shiyangx.he@intel.com>
> Sent: 2023年1月13日 17:08
> To: He, ShiyangX <shiyangx.he@intel.com>; Zhang, Yuying
> <yuying.zhang@intel.com>
> Cc: dev@dpdk.org; Zhou, YidingX <yidingx.zhou@intel.com>;
> stable@dpdk.org; Singh, Aman Deep <aman.deep.singh@intel.com>;
> Burakov, Anatoly <anatoly.burakov@intel.com>; Li, Xiaoyun
> <xiaoyun.li@intel.com>; Alvin Zhang <alvinx.zhang@intel.com>
> Subject: RE: [PATCH] app/testpmd: fix secondary process not forwarding
>
> @Zhang, Yuying Hi, please take a look at this patch! Are there any comments?
>
> >-----Original Message-----
> >From: He, ShiyangX <shiyangx.he@intel.com>
> >Sent: Wednesday, January 4, 2023 10:02 AM
> >To: Stephen Hemminger <stephen@networkplumber.org>
> >Cc: dev@dpdk.org; Zhou, YidingX <yidingx.zhou@intel.com>;
> >stable@dpdk.org; Singh, Aman Deep <aman.deep.singh@intel.com>; Zhang,
> >Yuying <yuying.zhang@intel.com>; Burakov, Anatoly
> ><anatoly.burakov@intel.com>; Li, Xiaoyun <xiaoyun.li@intel.com>; Alvin
> >Zhang <alvinx.zhang@intel.com>
> >Subject: RE: [PATCH] app/testpmd: fix secondary process not forwarding
> >
> >>> 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: a78040c990cb ("app/testpmd: update forward engine beginning")
> >>> Cc: stable@dpdk.org
> >>>
> >>> Signed-off-by: Shiyang He <shiyangx.he@intel.com>
> >>
> >>Would it be possible to fix this the initialization of ports variable,
> >>rather than doing a per-state fixup here?
> >
> >In multi-process scenario, the secondary process does not initialize
> >the queue state in the 'ports' variable, and the ethdev's queue state
> >may be changed by any other process, which causes 'ports' queue state
> >of per-process and ethdev's queue state are inconsistent. Therefore,
> >getting the queue state from ethdev is a feasible way which I can think of.
You should fix the queue state in the 'ports' variable of secondary process in the initialization
of ports variable instead of fixup here.
^ permalink raw reply [flat|nested] 24+ messages in thread
* RE: [PATCH] app/testpmd: fix secondary process not forwarding
2023-02-08 3:22 ` Zhang, Yuying
@ 2023-02-08 6:38 ` He, ShiyangX
2023-02-20 5:39 ` Zhang, Yuying
0 siblings, 1 reply; 24+ messages in thread
From: He, ShiyangX @ 2023-02-08 6:38 UTC (permalink / raw)
To: Zhang, Yuying
Cc: dev, Zhou, YidingX, stable, Singh, Aman Deep, Burakov, Anatoly,
Li, Xiaoyun, Alvin Zhang
>-----Original Message-----
>From: Zhang, Yuying <yuying.zhang@intel.com>
>Sent: Wednesday, February 8, 2023 11:22 AM
>To: He, ShiyangX <shiyangx.he@intel.com>
>Cc: dev@dpdk.org; Zhou, YidingX <yidingx.zhou@intel.com>;
>stable@dpdk.org; Singh, Aman Deep <aman.deep.singh@intel.com>; Burakov,
>Anatoly <anatoly.burakov@intel.com>; Li, Xiaoyun <xiaoyun.li@intel.com>;
>Alvin Zhang <alvinx.zhang@intel.com>
>Subject: RE: [PATCH] app/testpmd: fix secondary process not forwarding
>
>Hi Shiyang,
>
>> -----Original Message-----
>> From: He, ShiyangX <shiyangx.he@intel.com>
>> Sent: 2023年1月13日 17:08
>> To: He, ShiyangX <shiyangx.he@intel.com>; Zhang, Yuying
>> <yuying.zhang@intel.com>
>> Cc: dev@dpdk.org; Zhou, YidingX <yidingx.zhou@intel.com>;
>> stable@dpdk.org; Singh, Aman Deep <aman.deep.singh@intel.com>;
>> Burakov, Anatoly <anatoly.burakov@intel.com>; Li, Xiaoyun
>> <xiaoyun.li@intel.com>; Alvin Zhang <alvinx.zhang@intel.com>
>> Subject: RE: [PATCH] app/testpmd: fix secondary process not forwarding
>>
>> @Zhang, Yuying Hi, please take a look at this patch! Are there any comments?
>>
>> >-----Original Message-----
>> >From: He, ShiyangX <shiyangx.he@intel.com>
>> >Sent: Wednesday, January 4, 2023 10:02 AM
>> >To: Stephen Hemminger <stephen@networkplumber.org>
>> >Cc: dev@dpdk.org; Zhou, YidingX <yidingx.zhou@intel.com>;
>> >stable@dpdk.org; Singh, Aman Deep <aman.deep.singh@intel.com>;
>Zhang,
>> >Yuying <yuying.zhang@intel.com>; Burakov, Anatoly
>> ><anatoly.burakov@intel.com>; Li, Xiaoyun <xiaoyun.li@intel.com>;
>> >Alvin Zhang <alvinx.zhang@intel.com>
>> >Subject: RE: [PATCH] app/testpmd: fix secondary process not
>> >forwarding
>> >
>> >>> 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: a78040c990cb ("app/testpmd: update forward engine
>> >>> beginning")
>> >>> Cc: stable@dpdk.org
>> >>>
>> >>> Signed-off-by: Shiyang He <shiyangx.he@intel.com>
>> >>
>> >>Would it be possible to fix this the initialization of ports
>> >>variable, rather than doing a per-state fixup here?
>> >
>> >In multi-process scenario, the secondary process does not initialize
>> >the queue state in the 'ports' variable, and the ethdev's queue state
>> >may be changed by any other process, which causes 'ports' queue state
>> >of per-process and ethdev's queue state are inconsistent. Therefore,
>> >getting the queue state from ethdev is a feasible way which I can think of.
>
>You should fix the queue state in the 'ports' variable of secondary process in
>the initialization of ports variable instead of fixup here.
If the ethdev's queue state is changed by other processes, the queue state in the 'ports' variable is inconsistent with ethdev's queue state. Therefore, should obtain it from ethdev when accessing the queue state in the 'ports' variable.
^ permalink raw reply [flat|nested] 24+ messages in thread
* RE: [PATCH] app/testpmd: fix secondary process not forwarding
2023-02-08 6:38 ` He, ShiyangX
@ 2023-02-20 5:39 ` Zhang, Yuying
0 siblings, 0 replies; 24+ messages in thread
From: Zhang, Yuying @ 2023-02-20 5:39 UTC (permalink / raw)
To: He, ShiyangX
Cc: dev, Zhou, YidingX, stable, Singh, Aman Deep, Burakov, Anatoly,
Li, Xiaoyun, Alvin Zhang
> -----Original Message-----
> From: He, ShiyangX <shiyangx.he@intel.com>
> Sent: 2023年2月8日 14:39
> To: Zhang, Yuying <yuying.zhang@intel.com>
> Cc: dev@dpdk.org; Zhou, YidingX <yidingx.zhou@intel.com>;
> stable@dpdk.org; Singh, Aman Deep <aman.deep.singh@intel.com>;
> Burakov, Anatoly <anatoly.burakov@intel.com>; Li, Xiaoyun
> <xiaoyun.li@intel.com>; Alvin Zhang <alvinx.zhang@intel.com>
> Subject: RE: [PATCH] app/testpmd: fix secondary process not forwarding
>
>
>
> >-----Original Message-----
> >From: Zhang, Yuying <yuying.zhang@intel.com>
> >Sent: Wednesday, February 8, 2023 11:22 AM
> >To: He, ShiyangX <shiyangx.he@intel.com>
> >Cc: dev@dpdk.org; Zhou, YidingX <yidingx.zhou@intel.com>;
> >stable@dpdk.org; Singh, Aman Deep <aman.deep.singh@intel.com>;
> Burakov,
> >Anatoly <anatoly.burakov@intel.com>; Li, Xiaoyun
> ><xiaoyun.li@intel.com>; Alvin Zhang <alvinx.zhang@intel.com>
> >Subject: RE: [PATCH] app/testpmd: fix secondary process not forwarding
> >
> >Hi Shiyang,
> >
> >> -----Original Message-----
> >> From: He, ShiyangX <shiyangx.he@intel.com>
> >> Sent: 2023年1月13日 17:08
> >> To: He, ShiyangX <shiyangx.he@intel.com>; Zhang, Yuying
> >> <yuying.zhang@intel.com>
> >> Cc: dev@dpdk.org; Zhou, YidingX <yidingx.zhou@intel.com>;
> >> stable@dpdk.org; Singh, Aman Deep <aman.deep.singh@intel.com>;
> >> Burakov, Anatoly <anatoly.burakov@intel.com>; Li, Xiaoyun
> >> <xiaoyun.li@intel.com>; Alvin Zhang <alvinx.zhang@intel.com>
> >> Subject: RE: [PATCH] app/testpmd: fix secondary process not
> >> forwarding
> >>
> >> @Zhang, Yuying Hi, please take a look at this patch! Are there any
> comments?
> >>
> >> >-----Original Message-----
> >> >From: He, ShiyangX <shiyangx.he@intel.com>
> >> >Sent: Wednesday, January 4, 2023 10:02 AM
> >> >To: Stephen Hemminger <stephen@networkplumber.org>
> >> >Cc: dev@dpdk.org; Zhou, YidingX <yidingx.zhou@intel.com>;
> >> >stable@dpdk.org; Singh, Aman Deep <aman.deep.singh@intel.com>;
> >Zhang,
> >> >Yuying <yuying.zhang@intel.com>; Burakov, Anatoly
> >> ><anatoly.burakov@intel.com>; Li, Xiaoyun <xiaoyun.li@intel.com>;
> >> >Alvin Zhang <alvinx.zhang@intel.com>
> >> >Subject: RE: [PATCH] app/testpmd: fix secondary process not
> >> >forwarding
> >> >
> >> >>> 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: a78040c990cb ("app/testpmd: update forward engine
> >> >>> beginning")
> >> >>> Cc: stable@dpdk.org
> >> >>>
> >> >>> Signed-off-by: Shiyang He <shiyangx.he@intel.com>
Acked-by: Yuying Zhang <yuying.zhang@intel.com>
> >> >>
> >> >>Would it be possible to fix this the initialization of ports
> >> >>variable, rather than doing a per-state fixup here?
> >> >
> >> >In multi-process scenario, the secondary process does not initialize
> >> >the queue state in the 'ports' variable, and the ethdev's queue
> >> >state may be changed by any other process, which causes 'ports'
> >> >queue state of per-process and ethdev's queue state are
> >> >inconsistent. Therefore, getting the queue state from ethdev is a feasible
> way which I can think of.
> >
> >You should fix the queue state in the 'ports' variable of secondary
> >process in the initialization of ports variable instead of fixup here.
>
> If the ethdev's queue state is changed by other processes, the queue state in
> the 'ports' variable is inconsistent with ethdev's queue state. Therefore,
> should obtain it from ethdev when accessing the queue state in the 'ports'
> variable.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] app/testpmd: fix secondary process not forwarding
2022-12-30 7:55 [PATCH] app/testpmd: fix secondary process not forwarding Shiyang He
2022-12-30 17:23 ` Stephen Hemminger
@ 2023-02-20 12:45 ` lihuisong (C)
2023-02-21 2:52 ` He, ShiyangX
2023-02-21 15:44 ` [PATCH v2] " Shiyang He
2023-02-23 14:41 ` [PATCH v3] " Shiyang He
3 siblings, 1 reply; 24+ messages in thread
From: lihuisong (C) @ 2023-02-20 12:45 UTC (permalink / raw)
To: Shiyang He, dev
Cc: yidingx.zhou, stable, Aman Singh, Yuying Zhang, Anatoly Burakov,
Xiaoyun Li, Alvin Zhang
在 2022/12/30 15:55, 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: a78040c990cb ("app/testpmd: update forward engine beginning")
should use this commit:
Fixes: 3c4426db54fc ("app/testpmd: do not poll stopped queues")
> Cc: stable@dpdk.org
>
> Signed-off-by: Shiyang He <shiyangx.he@intel.com>
> ---
> app/test-pmd/testpmd.c | 29 +++++++++++++++++++++++++++--
> 1 file changed, 27 insertions(+), 2 deletions(-)
>
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> index 134d79a555..2c73daf9eb 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -2378,9 +2378,34 @@ 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_PRIMARY) {
directly use "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)
> + ports[fs->rx_port].rxq[fs->rx_queue].state =
> + rx_qinfo.queue_state;
> + 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)
> + ports[fs->tx_port].txq[fs->tx_queue].state =
> + tx_qinfo.queue_state;
> + else
> + TESTPMD_LOG(WARNING,
> + "Failed to get tx queue info\n");
not all PMDs implement rte_eth_rx/tx_queue_info_get() to query the
state, right?
Can you set this state to 'START' if the return value is '-ENOTSUP'?
> + }
> stream_init(fwd_streams[i]);
> + }
> + }
>
> port_fwd_begin = cur_fwd_config.fwd_eng->port_fwd_begin;
> if (port_fwd_begin != NULL) {
^ permalink raw reply [flat|nested] 24+ messages in thread
* RE: [PATCH] app/testpmd: fix secondary process not forwarding
2023-02-20 12:45 ` lihuisong (C)
@ 2023-02-21 2:52 ` He, ShiyangX
2023-02-21 6:37 ` lihuisong (C)
0 siblings, 1 reply; 24+ messages in thread
From: He, ShiyangX @ 2023-02-21 2:52 UTC (permalink / raw)
To: lihuisong (C), dev
Cc: Zhou, YidingX, stable, Singh, Aman Deep, Zhang, Yuying, Burakov,
Anatoly, Li, Xiaoyun, Alvin Zhang
>-----Original Message-----
>From: lihuisong (C) <lihuisong@huawei.com>
>Sent: Monday, February 20, 2023 8:46 PM
>To: He, ShiyangX <shiyangx.he@intel.com>; dev@dpdk.org
>Cc: Zhou, YidingX <yidingx.zhou@intel.com>; stable@dpdk.org; Singh, Aman
>Deep <aman.deep.singh@intel.com>; Zhang, Yuying
><yuying.zhang@intel.com>; Burakov, Anatoly <anatoly.burakov@intel.com>;
>Li, Xiaoyun <xiaoyun.li@intel.com>; Alvin Zhang <alvinx.zhang@intel.com>
>Subject: Re: [PATCH] app/testpmd: fix secondary process not forwarding
>
>
>在 2022/12/30 15:55, 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: a78040c990cb ("app/testpmd: update forward engine beginning")
>should use this commit:
>Fixes: 3c4426db54fc ("app/testpmd: do not poll stopped queues")
Thanks for your comments, I will ask maintainer to help fix this problem.
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Shiyang He <shiyangx.he@intel.com>
>> ---
>> app/test-pmd/testpmd.c | 29 +++++++++++++++++++++++++++--
>> 1 file changed, 27 insertions(+), 2 deletions(-)
>>
>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
>> 134d79a555..2c73daf9eb 100644
>> --- a/app/test-pmd/testpmd.c
>> +++ b/app/test-pmd/testpmd.c
>> @@ -2378,9 +2378,34 @@ 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_PRIMARY) {
>
>directly use "rte_eal_process_type() == RTE_PROC_SECONDARY"?
The following action should be executed for all non-primary processes.
>
>> + 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)
>> + ports[fs->rx_port].rxq[fs-
>>rx_queue].state =
>> + rx_qinfo.queue_state;
>> + 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)
>> + ports[fs->tx_port].txq[fs-
>>tx_queue].state =
>> + tx_qinfo.queue_state;
>> + else
>> + TESTPMD_LOG(WARNING,
>> + "Failed to get tx queue
>info\n");
>not all PMDs implement rte_eth_rx/tx_queue_info_get() to query the state,
>right?
>Can you set this state to 'START' if the return value is '-ENOTSUP'?
If pmd doesn't implement "rte_eth_rx/tx_queue_info_get()" to query queue state, should use the default value instead of modifying the state, because it may be modified elsewhere.
>> + }
>> stream_init(fwd_streams[i]);
>> + }
>> + }
>>
>> port_fwd_begin = cur_fwd_config.fwd_eng->port_fwd_begin;
>> if (port_fwd_begin != NULL) {
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] app/testpmd: fix secondary process not forwarding
2023-02-21 2:52 ` He, ShiyangX
@ 2023-02-21 6:37 ` lihuisong (C)
2023-02-21 6:51 ` He, ShiyangX
0 siblings, 1 reply; 24+ messages in thread
From: lihuisong (C) @ 2023-02-21 6:37 UTC (permalink / raw)
To: He, ShiyangX, dev
Cc: Zhou, YidingX, stable, Singh, Aman Deep, Zhang, Yuying, Burakov,
Anatoly, Li, Xiaoyun, Alvin Zhang
在 2023/2/21 10:52, He, ShiyangX 写道:
>
>> -----Original Message-----
>> From: lihuisong (C) <lihuisong@huawei.com>
>> Sent: Monday, February 20, 2023 8:46 PM
>> To: He, ShiyangX <shiyangx.he@intel.com>; dev@dpdk.org
>> Cc: Zhou, YidingX <yidingx.zhou@intel.com>; stable@dpdk.org; Singh, Aman
>> Deep <aman.deep.singh@intel.com>; Zhang, Yuying
>> <yuying.zhang@intel.com>; Burakov, Anatoly <anatoly.burakov@intel.com>;
>> Li, Xiaoyun <xiaoyun.li@intel.com>; Alvin Zhang <alvinx.zhang@intel.com>
>> Subject: Re: [PATCH] app/testpmd: fix secondary process not forwarding
>>
>>
>> 在 2022/12/30 15:55, 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: a78040c990cb ("app/testpmd: update forward engine beginning")
>> should use this commit:
>> Fixes: 3c4426db54fc ("app/testpmd: do not poll stopped queues")
> Thanks for your comments, I will ask maintainer to help fix this problem.
>
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Shiyang He <shiyangx.he@intel.com>
>>> ---
>>> app/test-pmd/testpmd.c | 29 +++++++++++++++++++++++++++--
>>> 1 file changed, 27 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
>>> 134d79a555..2c73daf9eb 100644
>>> --- a/app/test-pmd/testpmd.c
>>> +++ b/app/test-pmd/testpmd.c
>>> @@ -2378,9 +2378,34 @@ 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_PRIMARY) {
>> directly use "rte_eal_process_type() == RTE_PROC_SECONDARY"?
> The following action should be executed for all non-primary processes.
"all non-primary processes" is which processes? it's only 'secondary'
here, not 'auto', right?
>
>>> + 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)
>>> + ports[fs->rx_port].rxq[fs-
>>> rx_queue].state =
>>> + rx_qinfo.queue_state;
>>> + 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)
>>> + ports[fs->tx_port].txq[fs-
>>> tx_queue].state =
>>> + tx_qinfo.queue_state;
>>> + else
>>> + TESTPMD_LOG(WARNING,
>>> + "Failed to get tx queue
>> info\n");
>> not all PMDs implement rte_eth_rx/tx_queue_info_get() to query the state,
>> right?
>> Can you set this state to 'START' if the return value is '-ENOTSUP'?
> If pmd doesn't implement "rte_eth_rx/tx_queue_info_get()" to query queue state, should use the default value instead of modifying the state, because it may be modified elsewhere.
The rx/tx_queue_start/stop() can change Rx/Tx queue state if PMD
supports this API.
In primary, if PMD doesn't support this API, this queue state do not be
changed and still be the original value(RTE_ETH_QUEUE_STATE_STARTED)
start_port() sets.
However, in secondary, Rx/Tx queue state have not been initialized, your
patch also do not it.
Namely, their default values are wrong.
Our plan needs to ensure that the PMDs that do not support
rx/tx_queue_start/stop() or rx/tx_queue_info_get() can forward in secondary.
I think we either add the initialization of queue state in start_port()
or somewhere else, or make sure it's ok here.
>>> + }
>>> stream_init(fwd_streams[i]);
>>> + }
>>> + }
>>>
>>> port_fwd_begin = cur_fwd_config.fwd_eng->port_fwd_begin;
>>> if (port_fwd_begin != NULL) {
^ permalink raw reply [flat|nested] 24+ messages in thread
* RE: [PATCH] app/testpmd: fix secondary process not forwarding
2023-02-21 6:37 ` lihuisong (C)
@ 2023-02-21 6:51 ` He, ShiyangX
0 siblings, 0 replies; 24+ messages in thread
From: He, ShiyangX @ 2023-02-21 6:51 UTC (permalink / raw)
To: lihuisong (C), dev
Cc: Zhou, YidingX, stable, Singh, Aman Deep, Zhang, Yuying, Burakov,
Anatoly, Li, Xiaoyun, Alvin Zhang
>-----Original Message-----
>From: lihuisong (C) <lihuisong@huawei.com>
>Sent: Tuesday, February 21, 2023 2:38 PM
>To: He, ShiyangX <shiyangx.he@intel.com>; dev@dpdk.org
>Cc: Zhou, YidingX <yidingx.zhou@intel.com>; stable@dpdk.org; Singh, Aman
>Deep <aman.deep.singh@intel.com>; Zhang, Yuying
><yuying.zhang@intel.com>; Burakov, Anatoly <anatoly.burakov@intel.com>;
>Li, Xiaoyun <xiaoyun.li@intel.com>; Alvin Zhang <alvinx.zhang@intel.com>
>Subject: Re: [PATCH] app/testpmd: fix secondary process not forwarding
>
>
>在 2023/2/21 10:52, He, ShiyangX 写道:
>>
>>> -----Original Message-----
>>> From: lihuisong (C) <lihuisong@huawei.com>
>>> Sent: Monday, February 20, 2023 8:46 PM
>>> To: He, ShiyangX <shiyangx.he@intel.com>; dev@dpdk.org
>>> Cc: Zhou, YidingX <yidingx.zhou@intel.com>; stable@dpdk.org; Singh,
>>> Aman Deep <aman.deep.singh@intel.com>; Zhang, Yuying
>>> <yuying.zhang@intel.com>; Burakov, Anatoly
>>> <anatoly.burakov@intel.com>; Li, Xiaoyun <xiaoyun.li@intel.com>;
>>> Alvin Zhang <alvinx.zhang@intel.com>
>>> Subject: Re: [PATCH] app/testpmd: fix secondary process not
>>> forwarding
>>>
>>>
>>> 在 2022/12/30 15:55, 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: a78040c990cb ("app/testpmd: update forward engine beginning")
>>> should use this commit:
>>> Fixes: 3c4426db54fc ("app/testpmd: do not poll stopped queues")
>> Thanks for your comments, I will ask maintainer to help fix this problem.
>>
>>>> Cc: stable@dpdk.org
>>>>
>>>> Signed-off-by: Shiyang He <shiyangx.he@intel.com>
>>>> ---
>>>> app/test-pmd/testpmd.c | 29 +++++++++++++++++++++++++++--
>>>> 1 file changed, 27 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
>>>> 134d79a555..2c73daf9eb 100644
>>>> --- a/app/test-pmd/testpmd.c
>>>> +++ b/app/test-pmd/testpmd.c
>>>> @@ -2378,9 +2378,34 @@ 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_PRIMARY) {
>>> directly use "rte_eal_process_type() == RTE_PROC_SECONDARY"?
>> The following action should be executed for all non-primary processes.
>"all non-primary processes" is which processes? it's only 'secondary'
>here, not 'auto', right?
>>
>>>> + 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)
>>>> + ports[fs->rx_port].rxq[fs-
>>>> rx_queue].state =
>>>> + rx_qinfo.queue_state;
>>>> + 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)
>>>> + ports[fs->tx_port].txq[fs-
>>>> tx_queue].state =
>>>> + tx_qinfo.queue_state;
>>>> + else
>>>> + TESTPMD_LOG(WARNING,
>>>> + "Failed to get tx queue
>>> info\n");
>>> not all PMDs implement rte_eth_rx/tx_queue_info_get() to query the
>>> state, right?
>>> Can you set this state to 'START' if the return value is '-ENOTSUP'?
>> If pmd doesn't implement "rte_eth_rx/tx_queue_info_get()" to query
>queue state, should use the default value instead of modifying the state,
>because it may be modified elsewhere.
>The rx/tx_queue_start/stop() can change Rx/Tx queue state if PMD supports
>this API.
>In primary, if PMD doesn't support this API, this queue state do not be
>changed and still be the original value(RTE_ETH_QUEUE_STATE_STARTED)
>start_port() sets.
>However, in secondary, Rx/Tx queue state have not been initialized, your
>patch also do not it.
>Namely, their default values are wrong.
>
>Our plan needs to ensure that the PMDs that do not support
>rx/tx_queue_start/stop() or rx/tx_queue_info_get() can forward in
>secondary.
>
>I think we either add the initialization of queue state in start_port() or
>somewhere else, or make sure it's ok here.
Thanks for your comment, the v2 patch will be sent soon!
>>>> + }
>>>> stream_init(fwd_streams[i]);
>>>> + }
>>>> + }
>>>>
>>>> port_fwd_begin = cur_fwd_config.fwd_eng->port_fwd_begin;
>>>> if (port_fwd_begin != NULL) {
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v2] app/testpmd: fix secondary process not forwarding
2022-12-30 7:55 [PATCH] app/testpmd: fix secondary process not forwarding Shiyang He
2022-12-30 17:23 ` Stephen Hemminger
2023-02-20 12:45 ` lihuisong (C)
@ 2023-02-21 15:44 ` Shiyang He
2023-02-22 6:20 ` lihuisong (C)
2023-02-23 14:41 ` [PATCH v3] " Shiyang He
3 siblings, 1 reply; 24+ messages in thread
From: Shiyang He @ 2023-02-21 15:44 UTC (permalink / raw)
To: dev
Cc: yidingx.zhou, Shiyang He, stable, Yuying Zhang, Aman Singh,
Anatoly Burakov, Matan Azrad, Dmitry Kozlyuk
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>
v2: Add function return value processing
---
app/test-pmd/testpmd.c | 35 +++++++++++++++++++++++++++++++++--
1 file changed, 33 insertions(+), 2 deletions(-)
diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 0c14325b8d..b450f3f6c4 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -2418,9 +2418,40 @@ 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)
+ ports[fs->rx_port].rxq[fs->rx_queue].state =
+ rx_qinfo.queue_state;
+ else if (-ENOTSUP == rc)
+ 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)
+ ports[fs->tx_port].txq[fs->tx_queue].state =
+ tx_qinfo.queue_state;
+ else if (-ENOTSUP == rc)
+ 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) {
--
2.37.2
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2] app/testpmd: fix secondary process not forwarding
2023-02-21 15:44 ` [PATCH v2] " Shiyang He
@ 2023-02-22 6:20 ` lihuisong (C)
0 siblings, 0 replies; 24+ messages in thread
From: lihuisong (C) @ 2023-02-22 6:20 UTC (permalink / raw)
To: Shiyang He, dev
Cc: yidingx.zhou, stable, Yuying Zhang, Aman Singh, Anatoly Burakov,
Matan Azrad, Dmitry Kozlyuk
在 2023/2/21 23:44, 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>
>
> v2: Add function return value processing
> ---
> app/test-pmd/testpmd.c | 35 +++++++++++++++++++++++++++++++++--
> 1 file changed, 33 insertions(+), 2 deletions(-)
>
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> index 0c14325b8d..b450f3f6c4 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -2418,9 +2418,40 @@ 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)
> + ports[fs->rx_port].rxq[fs->rx_queue].state =
> + rx_qinfo.queue_state;
> + else if (-ENOTSUP == rc)
"rc != 0" and "rc == -ENOTSUP" style are always welcome in dpdk.
Additionally, it is better to add some comments about '-ENOTSUP' here,
because this patch does not describe it anywhere.
> + 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)
> + ports[fs->tx_port].txq[fs->tx_queue].state =
> + tx_qinfo.queue_state;
> + else if (-ENOTSUP == rc)
> + 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) {
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3] app/testpmd: fix secondary process not forwarding
2023-02-23 14:41 ` [PATCH v3] " Shiyang He
@ 2023-02-23 8:08 ` lihuisong (C)
2023-03-06 15:16 ` Ferruh Yigit
2023-03-06 15:05 ` Ferruh Yigit
2023-03-08 16:19 ` [PATCH v4] " Shiyang He
2 siblings, 1 reply; 24+ messages in thread
From: lihuisong (C) @ 2023-02-23 8:08 UTC (permalink / raw)
To: Shiyang He, dev
Cc: yidingx.zhou, stable, Yuying Zhang, Aman Singh, Anatoly Burakov,
Matan Azrad, Dmitry Kozlyuk
在 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) {
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v3] app/testpmd: fix secondary process not forwarding
2022-12-30 7:55 [PATCH] app/testpmd: fix secondary process not forwarding Shiyang He
` (2 preceding siblings ...)
2023-02-21 15:44 ` [PATCH v2] " Shiyang He
@ 2023-02-23 14:41 ` Shiyang He
2023-02-23 8:08 ` lihuisong (C)
` (2 more replies)
3 siblings, 3 replies; 24+ messages in thread
From: Shiyang He @ 2023-02-23 14:41 UTC (permalink / raw)
To: dev
Cc: yidingx.zhou, Shiyang He, stable, Yuying Zhang, Aman Singh,
Anatoly Burakov, Matan Azrad, Dmitry Kozlyuk
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]);
+ }
+ }
port_fwd_begin = cur_fwd_config.fwd_eng->port_fwd_begin;
if (port_fwd_begin != NULL) {
--
2.37.2
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3] app/testpmd: fix secondary process not forwarding
2023-02-23 14:41 ` [PATCH v3] " Shiyang He
2023-02-23 8:08 ` lihuisong (C)
@ 2023-03-06 15:05 ` Ferruh Yigit
2023-03-07 3:25 ` He, ShiyangX
2023-03-08 16:19 ` [PATCH v4] " Shiyang He
2 siblings, 1 reply; 24+ messages in thread
From: Ferruh Yigit @ 2023-03-06 15:05 UTC (permalink / raw)
To: Shiyang He, dev
Cc: yidingx.zhou, stable, Yuying Zhang, Aman Singh, Anatoly Burakov,
Matan Azrad, Dmitry Kozlyuk
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?
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3] app/testpmd: fix secondary process not forwarding
2023-02-23 8:08 ` lihuisong (C)
@ 2023-03-06 15:16 ` Ferruh Yigit
0 siblings, 0 replies; 24+ messages in thread
From: Ferruh Yigit @ 2023-03-06 15:16 UTC (permalink / raw)
To: lihuisong (C), Shiyang He, dev
Cc: yidingx.zhou, stable, Yuying Zhang, Aman Singh, Anatoly Burakov,
Matan Azrad, Dmitry Kozlyuk
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) {
^ permalink raw reply [flat|nested] 24+ messages in thread
* RE: [PATCH v3] app/testpmd: fix secondary process not forwarding
2023-03-06 15:05 ` Ferruh Yigit
@ 2023-03-07 3:25 ` He, ShiyangX
2023-03-07 11:41 ` Ferruh Yigit
0 siblings, 1 reply; 24+ messages in thread
From: He, ShiyangX @ 2023-03-07 3:25 UTC (permalink / raw)
To: Ferruh Yigit, dev
Cc: Zhou, YidingX, stable, Zhang, Yuying, Singh, Aman Deep, Burakov,
Anatoly, Matan Azrad, Dmitry Kozlyuk
>-----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?
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3] app/testpmd: fix secondary process not forwarding
2023-03-07 3:25 ` He, ShiyangX
@ 2023-03-07 11:41 ` Ferruh Yigit
2023-03-08 2:05 ` He, ShiyangX
0 siblings, 1 reply; 24+ messages in thread
From: Ferruh Yigit @ 2023-03-07 11:41 UTC (permalink / raw)
To: He, ShiyangX, dev
Cc: Zhou, YidingX, stable, Zhang, Yuying, Singh, Aman Deep, Burakov,
Anatoly, Matan Azrad, Dmitry Kozlyuk
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?
^ permalink raw reply [flat|nested] 24+ messages in thread
* RE: [PATCH v3] app/testpmd: fix secondary process not forwarding
2023-03-07 11:41 ` Ferruh Yigit
@ 2023-03-08 2:05 ` He, ShiyangX
2023-03-08 2:54 ` lihuisong (C)
0 siblings, 1 reply; 24+ messages in thread
From: He, ShiyangX @ 2023-03-08 2:05 UTC (permalink / raw)
To: Ferruh Yigit, dev
Cc: Zhou, YidingX, stable, Zhang, Yuying, Singh, Aman Deep, Burakov,
Anatoly, Matan Azrad, Dmitry Kozlyuk
>-----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.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3] app/testpmd: fix secondary process not forwarding
2023-03-08 2:05 ` He, ShiyangX
@ 2023-03-08 2:54 ` lihuisong (C)
2023-03-08 9:54 ` Ferruh Yigit
0 siblings, 1 reply; 24+ messages in thread
From: lihuisong (C) @ 2023-03-08 2:54 UTC (permalink / raw)
To: He, ShiyangX, Ferruh Yigit, dev
Cc: Zhou, YidingX, stable, Zhang, Yuying, Singh, Aman Deep, Burakov,
Anatoly, Matan Azrad, Dmitry Kozlyuk
在 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.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3] app/testpmd: fix secondary process not forwarding
2023-03-08 2:54 ` lihuisong (C)
@ 2023-03-08 9:54 ` Ferruh Yigit
0 siblings, 0 replies; 24+ messages in thread
From: Ferruh Yigit @ 2023-03-08 9:54 UTC (permalink / raw)
To: lihuisong (C), He, ShiyangX, dev
Cc: Zhou, YidingX, stable, Zhang, Yuying, Singh, Aman Deep, Burakov,
Anatoly, Matan Azrad, Dmitry Kozlyuk
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.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v4] app/testpmd: fix secondary process not forwarding
2023-03-08 16:19 ` [PATCH v4] " Shiyang He
@ 2023-03-08 10:20 ` Ferruh Yigit
0 siblings, 0 replies; 24+ messages in thread
From: Ferruh Yigit @ 2023-03-08 10:20 UTC (permalink / raw)
To: Shiyang He, dev
Cc: yidingx.zhou, stable, Aman Singh, Yuying Zhang, Anatoly Burakov,
Matan Azrad, Dmitry Kozlyuk
On 3/8/2023 4:19 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>
Reviewed-by: Ferruh Yigit <ferruh.yigit@amd.com>
Applied to dpdk-next-net/main, thanks.
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v4] app/testpmd: fix secondary process not forwarding
2023-02-23 14:41 ` [PATCH v3] " Shiyang He
2023-02-23 8:08 ` lihuisong (C)
2023-03-06 15:05 ` Ferruh Yigit
@ 2023-03-08 16:19 ` Shiyang He
2023-03-08 10:20 ` Ferruh Yigit
2 siblings, 1 reply; 24+ messages in thread
From: Shiyang He @ 2023-03-08 16:19 UTC (permalink / raw)
To: dev
Cc: yidingx.zhou, Shiyang He, stable, Aman Singh, Yuying Zhang,
Anatoly Burakov, Matan Azrad, Dmitry Kozlyuk
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>
v2: Add function return value processing
v3: Add return value description
v4: Update queue state in 'start_port()'
---
app/test-pmd/testpmd.c | 72 +++++++++++++++++++++++++++++++++++++++++-
1 file changed, 71 insertions(+), 1 deletion(-)
diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 0c14325b8d..aa2a7b68ca 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -2379,6 +2379,70 @@ launch_packet_forwarding(lcore_function_t *pkt_fwd_on_lcore)
}
}
+static void
+update_rx_queue_state(uint16_t port_id, uint16_t queue_id)
+{
+ struct rte_eth_rxq_info rx_qinfo;
+ int32_t rc;
+
+ rc = rte_eth_rx_queue_info_get(port_id,
+ queue_id, &rx_qinfo);
+ if (rc == 0) {
+ ports[port_id].rxq[queue_id].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[port_id].rxq[queue_id].state =
+ RTE_ETH_QUEUE_STATE_STARTED;
+ } else {
+ TESTPMD_LOG(WARNING,
+ "Failed to get rx queue info\n");
+ }
+}
+
+static void
+update_tx_queue_state(uint16_t port_id, uint16_t queue_id)
+{
+ struct rte_eth_txq_info tx_qinfo;
+ int32_t rc;
+
+ rc = rte_eth_tx_queue_info_get(port_id,
+ queue_id, &tx_qinfo);
+ if (rc == 0) {
+ ports[port_id].txq[queue_id].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[port_id].txq[queue_id].state =
+ RTE_ETH_QUEUE_STATE_STARTED;
+ } else {
+ TESTPMD_LOG(WARNING,
+ "Failed to get tx queue info\n");
+ }
+}
+
+static void
+update_queue_state(void)
+{
+ portid_t pi;
+ queueid_t qi;
+
+ RTE_ETH_FOREACH_DEV(pi) {
+ for (qi = 0; qi < nb_rxq; qi++)
+ update_rx_queue_state(pi, qi);
+ for (qi = 0; qi < nb_txq; qi++)
+ update_tx_queue_state(pi, qi);
+ }
+}
+
/*
* Launch packet forwarding configuration.
*/
@@ -2418,9 +2482,12 @@ start_packet_forwarding(int with_tx_first)
if (!pkt_fwd_shared_rxq_check())
return;
- if (stream_init != NULL)
+ if (stream_init != NULL) {
+ if (rte_eal_process_type() == RTE_PROC_SECONDARY)
+ update_queue_state();
for (i = 0; i < cur_fwd_config.nb_fwd_streams; i++)
stream_init(fwd_streams[i]);
+ }
port_fwd_begin = cur_fwd_config.fwd_eng->port_fwd_begin;
if (port_fwd_begin != NULL) {
@@ -3180,6 +3247,9 @@ start_port(portid_t pid)
pl[cfg_pi++] = pi;
}
+ if (rte_eal_process_type() == RTE_PROC_SECONDARY)
+ update_queue_state();
+
if (at_least_one_port_successfully_started && !no_link_check)
check_all_ports_link_status(RTE_PORT_ALL);
else if (at_least_one_port_exist & all_ports_already_started)
--
2.37.2
^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2023-03-08 10:20 UTC | newest]
Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-12-30 7:55 [PATCH] app/testpmd: fix secondary process not forwarding Shiyang He
2022-12-30 17:23 ` Stephen Hemminger
2023-01-04 2:02 ` He, ShiyangX
2023-01-13 9:07 ` He, ShiyangX
2023-02-08 3:22 ` Zhang, Yuying
2023-02-08 6:38 ` He, ShiyangX
2023-02-20 5:39 ` Zhang, Yuying
2023-02-20 12:45 ` lihuisong (C)
2023-02-21 2:52 ` He, ShiyangX
2023-02-21 6:37 ` lihuisong (C)
2023-02-21 6:51 ` He, ShiyangX
2023-02-21 15:44 ` [PATCH v2] " Shiyang He
2023-02-22 6:20 ` lihuisong (C)
2023-02-23 14:41 ` [PATCH v3] " Shiyang He
2023-02-23 8:08 ` lihuisong (C)
2023-03-06 15:16 ` Ferruh Yigit
2023-03-06 15:05 ` Ferruh Yigit
2023-03-07 3:25 ` He, ShiyangX
2023-03-07 11:41 ` Ferruh Yigit
2023-03-08 2:05 ` He, ShiyangX
2023-03-08 2:54 ` lihuisong (C)
2023-03-08 9:54 ` Ferruh Yigit
2023-03-08 16:19 ` [PATCH v4] " Shiyang He
2023-03-08 10:20 ` Ferruh Yigit
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).