* [dpdk-dev] [PATCH] net/e1000: fix nic ops function was no initialized in secondary process @ 2021-06-18 17:26 Tengfei Zhang 2021-06-21 7:31 ` Wang, Haiyue 0 siblings, 1 reply; 5+ messages in thread From: Tengfei Zhang @ 2021-06-18 17:26 UTC (permalink / raw) To: Haiyue Wang; +Cc: dev, Tengfei Zhang 'e1000_setup_init_funcs' was not called in secondary process, it initialize mac,phy,nvm ops. when secondary process get link status,it will coredump. Signed-off-by: Tengfei Zhang <zypscode@outlook.com> --- drivers/net/e1000/em_ethdev.c | 1 + drivers/net/e1000/igb_ethdev.c | 2 ++ 2 files changed, 3 insertions(+) diff --git a/drivers/net/e1000/em_ethdev.c b/drivers/net/e1000/em_ethdev.c index a0ca371b02..cd5faa4228 100644 --- a/drivers/net/e1000/em_ethdev.c +++ b/drivers/net/e1000/em_ethdev.c @@ -258,6 +258,7 @@ eth_em_dev_init(struct rte_eth_dev *eth_dev) * has already done this work. Only check we don't need a different * RX function */ if (rte_eal_process_type() != RTE_PROC_PRIMARY){ + e1000_setup_init_funcs(hw, TRUE); if (eth_dev->data->scattered_rx) eth_dev->rx_pkt_burst = (eth_rx_burst_t)ð_em_recv_scattered_pkts; diff --git a/drivers/net/e1000/igb_ethdev.c b/drivers/net/e1000/igb_ethdev.c index 10ee0f3341..7d9d60497d 100644 --- a/drivers/net/e1000/igb_ethdev.c +++ b/drivers/net/e1000/igb_ethdev.c @@ -737,6 +737,7 @@ eth_igb_dev_init(struct rte_eth_dev *eth_dev) * has already done this work. Only check we don't need a different * RX function */ if (rte_eal_process_type() != RTE_PROC_PRIMARY){ + e1000_setup_init_funcs(hw, TRUE); if (eth_dev->data->scattered_rx) eth_dev->rx_pkt_burst = ð_igb_recv_scattered_pkts; return 0; @@ -931,6 +932,7 @@ eth_igbvf_dev_init(struct rte_eth_dev *eth_dev) * has already done this work. Only check we don't need a different * RX function */ if (rte_eal_process_type() != RTE_PROC_PRIMARY){ + e1000_setup_init_funcs(hw, TRUE); if (eth_dev->data->scattered_rx) eth_dev->rx_pkt_burst = ð_igb_recv_scattered_pkts; return 0; -- 2.26.2 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [dpdk-dev] [PATCH] net/e1000: fix nic ops function was no initialized in secondary process 2021-06-18 17:26 [dpdk-dev] [PATCH] net/e1000: fix nic ops function was no initialized in secondary process Tengfei Zhang @ 2021-06-21 7:31 ` Wang, Haiyue 2021-06-21 8:34 ` 张 杨 0 siblings, 1 reply; 5+ messages in thread From: Wang, Haiyue @ 2021-06-21 7:31 UTC (permalink / raw) To: Tengfei Zhang; +Cc: dev, Zhang, Qi Z, Lin, Xueqin > -----Original Message----- > From: Tengfei Zhang <zypscode@outlook.com> > Sent: Saturday, June 19, 2021 01:27 > To: Wang, Haiyue <haiyue.wang@intel.com> > Cc: dev@dpdk.org; Tengfei Zhang <zypscode@outlook.com> > Subject: [PATCH] net/e1000: fix nic ops function was no initialized in secondary process > > 'e1000_setup_init_funcs' was not called in secondary process, > it initialize mac,phy,nvm ops. > when secondary process get link status,it will coredump. Thanks, Tengfei. Since primary / secondary is so complicated, AFAIK, the control path is in primary, the secondary is mainly for rx/tx ops officially, like new Intel ice PMD: if (rte_eal_process_type() != RTE_PROC_PRIMARY) { ice_set_rx_function(dev); ice_set_tx_function(dev); return 0; } So you can keep your patch as private for special secondary usage. ;-) > > Signed-off-by: Tengfei Zhang <zypscode@outlook.com> > --- > drivers/net/e1000/em_ethdev.c | 1 + > drivers/net/e1000/igb_ethdev.c | 2 ++ > 2 files changed, 3 insertions(+) > > diff --git a/drivers/net/e1000/em_ethdev.c b/drivers/net/e1000/em_ethdev.c > index a0ca371b02..cd5faa4228 100644 > --- a/drivers/net/e1000/em_ethdev.c > +++ b/drivers/net/e1000/em_ethdev.c > @@ -258,6 +258,7 @@ eth_em_dev_init(struct rte_eth_dev *eth_dev) > * has already done this work. Only check we don't need a different > * RX function */ > if (rte_eal_process_type() != RTE_PROC_PRIMARY){ > + e1000_setup_init_funcs(hw, TRUE); > if (eth_dev->data->scattered_rx) > eth_dev->rx_pkt_burst = > (eth_rx_burst_t)ð_em_recv_scattered_pkts; > diff --git a/drivers/net/e1000/igb_ethdev.c b/drivers/net/e1000/igb_ethdev.c > index 10ee0f3341..7d9d60497d 100644 > --- a/drivers/net/e1000/igb_ethdev.c > +++ b/drivers/net/e1000/igb_ethdev.c > @@ -737,6 +737,7 @@ eth_igb_dev_init(struct rte_eth_dev *eth_dev) > * has already done this work. Only check we don't need a different > * RX function */ > if (rte_eal_process_type() != RTE_PROC_PRIMARY){ > + e1000_setup_init_funcs(hw, TRUE); > if (eth_dev->data->scattered_rx) > eth_dev->rx_pkt_burst = ð_igb_recv_scattered_pkts; > return 0; > @@ -931,6 +932,7 @@ eth_igbvf_dev_init(struct rte_eth_dev *eth_dev) > * has already done this work. Only check we don't need a different > * RX function */ > if (rte_eal_process_type() != RTE_PROC_PRIMARY){ > + e1000_setup_init_funcs(hw, TRUE); > if (eth_dev->data->scattered_rx) > eth_dev->rx_pkt_burst = ð_igb_recv_scattered_pkts; > return 0; > -- > 2.26.2 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [dpdk-dev] [PATCH] net/e1000: fix nic ops function was no initialized in secondary process 2021-06-21 7:31 ` Wang, Haiyue @ 2021-06-21 8:34 ` 张 杨 2021-06-22 2:16 ` Wang, Haiyue 0 siblings, 1 reply; 5+ messages in thread From: 张 杨 @ 2021-06-21 8:34 UTC (permalink / raw) To: Wang, Haiyue; +Cc: dev, Zhang, Qi Z, Lin, Xueqin 发件人: Wang, Haiyue <haiyue.wang@intel.com> 发送时间: 2021年6月21日星期一 15:31 收件人: Tengfei Zhang 抄送: dev@dpdk.org; Zhang, Qi Z; Lin, Xueqin 主题: RE: [PATCH] net/e1000: fix nic ops function was no initialized in secondary process > -----Original Message----- > From: Tengfei Zhang <zypscode@outlook.com> > Sent: Saturday, June 19, 2021 01:27 > To: Wang, Haiyue <haiyue.wang@intel.com> > Cc: dev@dpdk.org; Tengfei Zhang <zypscode@outlook.com> > Subject: [PATCH] net/e1000: fix nic ops function was no initialized in secondary process > > 'e1000_setup_init_funcs' was not called in secondary process, > it initialize mac,phy,nvm ops. > when secondary process get link status,it will coredump. Thanks, Tengfei. Since primary / secondary is so complicated, AFAIK, the control path is in primary, the secondary is mainly for rx/tx ops officially, like new Intel ice PMD: if (rte_eal_process_type() != RTE_PROC_PRIMARY) { ice_set_rx_function(dev); ice_set_tx_function(dev); return 0; } So you can keep your patch as private for special secondary usage. ;-) > > Signed-off-by: Tengfei Zhang <zypscode@outlook.com> > --- > drivers/net/e1000/em_ethdev.c | 1 + > drivers/net/e1000/igb_ethdev.c | 2 ++ > 2 files changed, 3 insertions(+) > > diff --git a/drivers/net/e1000/em_ethdev.c b/drivers/net/e1000/em_ethdev.c > index a0ca371b02..cd5faa4228 100644 > --- a/drivers/net/e1000/em_ethdev.c > +++ b/drivers/net/e1000/em_ethdev.c > @@ -258,6 +258,7 @@ eth_em_dev_init(struct rte_eth_dev *eth_dev) > * has already done this work. Only check we don't need a different > * RX function */ > if (rte_eal_process_type() != RTE_PROC_PRIMARY){ > + e1000_setup_init_funcs(hw, TRUE); > if (eth_dev->data->scattered_rx) > eth_dev->rx_pkt_burst = > (eth_rx_burst_t)ð_em_recv_scattered_pkts; > diff --git a/drivers/net/e1000/igb_ethdev.c b/drivers/net/e1000/igb_ethdev.c > index 10ee0f3341..7d9d60497d 100644 > --- a/drivers/net/e1000/igb_ethdev.c > +++ b/drivers/net/e1000/igb_ethdev.c > @@ -737,6 +737,7 @@ eth_igb_dev_init(struct rte_eth_dev *eth_dev) > * has already done this work. Only check we don't need a different > * RX function */ > if (rte_eal_process_type() != RTE_PROC_PRIMARY){ > + e1000_setup_init_funcs(hw, TRUE); > if (eth_dev->data->scattered_rx) > eth_dev->rx_pkt_burst = ð_igb_recv_scattered_pkts; > return 0; > @@ -931,6 +932,7 @@ eth_igbvf_dev_init(struct rte_eth_dev *eth_dev) > * has already done this work. Only check we don't need a different > * RX function */ > if (rte_eal_process_type() != RTE_PROC_PRIMARY){ > + e1000_setup_init_funcs(hw, TRUE); > if (eth_dev->data->scattered_rx) > eth_dev->rx_pkt_burst = ð_igb_recv_scattered_pkts; > return 0; > -- > 2.26.2 this issue does not appear in ice, i40e, vmxnet3 PMD drivers. Only in e1000 , ixgbe drivers. Ice pmd driver gets link status by read reg directly. I agree with what you said "primary, the secondary is mainly for rx/tx ops officially". My opinion is the "set actions" shouldn't called in secondary process, but "get actions" was very common operation, they shouldn't be banned. Thanks for your reply ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [dpdk-dev] [PATCH] net/e1000: fix nic ops function was no initialized in secondary process 2021-06-21 8:34 ` 张 杨 @ 2021-06-22 2:16 ` Wang, Haiyue 2021-06-22 13:56 ` Tengfei Zhang 0 siblings, 1 reply; 5+ messages in thread From: Wang, Haiyue @ 2021-06-22 2:16 UTC (permalink / raw) To: 张 杨; +Cc: dev, Zhang, Qi Z, Lin, Xueqin From: 张 杨 <zypscode@outlook.com> Sent: Monday, June 21, 2021 16:35 To: Wang, Haiyue <haiyue.wang@intel.com> Cc: dev@dpdk.org; Zhang, Qi Z <qi.z.zhang@intel.com>; Lin, Xueqin <xueqin.lin@intel.com> Subject: Re: [PATCH] net/e1000: fix nic ops function was no initialized in secondary process 发件人: Wang, Haiyue <mailto:haiyue.wang@intel.com> 发送时间: 2021年6月21日星期一 15:31 收件人: Tengfei Zhang 抄送: mailto:dev@dpdk.org; Zhang, Qi Z; Lin, Xueqin 主题: RE: [PATCH] net/e1000: fix nic ops function was no initialized in secondary process > -----Original Message----- > From: Tengfei Zhang <mailto:zypscode@outlook.com> > Sent: Saturday, June 19, 2021 01:27 > To: Wang, Haiyue <mailto:haiyue.wang@intel.com> > Cc: mailto:dev@dpdk.org; Tengfei Zhang <mailto:zypscode@outlook.com> > Subject: [PATCH] net/e1000: fix nic ops function was no initialized in secondary process > > 'e1000_setup_init_funcs' was not called in secondary process, > it initialize mac,phy,nvm ops. > when secondary process get link status,it will coredump. > Thanks, Tengfei. > Since primary / secondary is so complicated, AFAIK, the control path is in > primary, the secondary is mainly for rx/tx ops officially, like new Intel > ice PMD: > if (rte_eal_process_type() != RTE_PROC_PRIMARY) { > ice_set_rx_function(dev); > ice_set_tx_function(dev); > return 0; > } > > So you can keep your patch as private for special secondary usage. ;-) > > Signed-off-by: Tengfei Zhang <mailto:zypscode@outlook.com> > --- > drivers/net/e1000/em_ethdev.c | 1 + > drivers/net/e1000/igb_ethdev.c | 2 ++ > 2 files changed, 3 insertions(+) > > diff --git a/drivers/net/e1000/em_ethdev.c b/drivers/net/e1000/em_ethdev.c > index a0ca371b02..cd5faa4228 100644 > --- a/drivers/net/e1000/em_ethdev.c > +++ b/drivers/net/e1000/em_ethdev.c > @@ -258,6 +258,7 @@ eth_em_dev_init(struct rte_eth_dev *eth_dev) > * has already done this work. Only check we don't need a different > * RX function */ > if (rte_eal_process_type() != RTE_PROC_PRIMARY){ > + e1000_setup_init_funcs(hw, TRUE); > if (eth_dev->data->scattered_rx) > eth_dev->rx_pkt_burst = > (eth_rx_burst_t)ð_em_recv_scattered_pkts; > diff --git a/drivers/net/e1000/igb_ethdev.c b/drivers/net/e1000/igb_ethdev.c > index 10ee0f3341..7d9d60497d 100644 > --- a/drivers/net/e1000/igb_ethdev.c > +++ b/drivers/net/e1000/igb_ethdev.c > @@ -737,6 +737,7 @@ eth_igb_dev_init(struct rte_eth_dev *eth_dev) > * has already done this work. Only check we don't need a different > * RX function */ > if (rte_eal_process_type() != RTE_PROC_PRIMARY){ > + e1000_setup_init_funcs(hw, TRUE); > if (eth_dev->data->scattered_rx) > eth_dev->rx_pkt_burst = ð_igb_recv_scattered_pkts; > return 0; > @@ -931,6 +932,7 @@ eth_igbvf_dev_init(struct rte_eth_dev *eth_dev) > * has already done this work. Only check we don't need a different > * RX function */ > if (rte_eal_process_type() != RTE_PROC_PRIMARY){ > + e1000_setup_init_funcs(hw, TRUE); > if (eth_dev->data->scattered_rx) > eth_dev->rx_pkt_burst = ð_igb_recv_scattered_pkts; > return 0; > -- > 2.26.2 > this issue does not appear in ice, i40e, vmxnet3 PMD drivers. Only in e1000 , ixgbe drivers. > Ice pmd driver gets link status by read reg directly. For making primary & secondary working well, these drivers try to avoid save the global data and ops function in shared data at the design of beginning. > I agree with what you said "primary, the secondary is mainly for rx/tx ops officially". > My opinion is the "set actions" shouldn't called in secondary process, but "get actions" was very common operation, they shouldn't be banned. It's not banned, just because e1000's design introduces the global data ops in the shared data, which is not good for sharing and accessing, since the address of the global data ops is changed in secondary process. As you can see in " e1000_setup_init_funcs", they not only set function pointers, but also call them, not sure this will break other things or not. ;-) > Thanks for your reply ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [dpdk-dev] [PATCH] net/e1000: fix nic ops function was no initialized in secondary process 2021-06-22 2:16 ` Wang, Haiyue @ 2021-06-22 13:56 ` Tengfei Zhang 0 siblings, 0 replies; 5+ messages in thread From: Tengfei Zhang @ 2021-06-22 13:56 UTC (permalink / raw) To: Wang, Haiyue; +Cc: dev, Zhang, Qi Z, Lin, Xueqin On 2021/6/22 上午10:16, Wang, Haiyue wrote: > From: 张 杨 <zypscode@outlook.com> > Sent: Monday, June 21, 2021 16:35 > To: Wang, Haiyue <haiyue.wang@intel.com> > Cc: dev@dpdk.org; Zhang, Qi Z <qi.z.zhang@intel.com>; Lin, Xueqin <xueqin.lin@intel.com> > Subject: Re: [PATCH] net/e1000: fix nic ops function was no initialized in secondary process > > 发件人: Wang, Haiyue <mailto:haiyue.wang@intel.com> > 发送时间: 2021年6月21日星期一 15:31 > 收件人: Tengfei Zhang > 抄送: mailto:dev@dpdk.org; Zhang, Qi Z; Lin, Xueqin > 主题: RE: [PATCH] net/e1000: fix nic ops function was no initialized in secondary process > > >> -----Original Message----- >> From: Tengfei Zhang <mailto:zypscode@outlook.com> >> Sent: Saturday, June 19, 2021 01:27 >> To: Wang, Haiyue <mailto:haiyue.wang@intel.com> >> Cc: mailto:dev@dpdk.org; Tengfei Zhang <mailto:zypscode@outlook.com> >> Subject: [PATCH] net/e1000: fix nic ops function was no initialized in secondary process >> >> 'e1000_setup_init_funcs' was not called in secondary process, >> it initialize mac,phy,nvm ops. >> when secondary process get link status,it will coredump. >> Thanks, Tengfei. >> Since primary / secondary is so complicated, AFAIK, the control path is in >> primary, the secondary is mainly for rx/tx ops officially, like new Intel >> ice PMD: >> if (rte_eal_process_type() != RTE_PROC_PRIMARY) { >> ice_set_rx_function(dev); >> ice_set_tx_function(dev); >> return 0; >> } >> >> So you can keep your patch as private for special secondary usage. ;-) >> Signed-off-by: Tengfei Zhang <mailto:zypscode@outlook.com> >> --- >> drivers/net/e1000/em_ethdev.c | 1 + >> drivers/net/e1000/igb_ethdev.c | 2 ++ >> 2 files changed, 3 insertions(+) >> >> diff --git a/drivers/net/e1000/em_ethdev.c b/drivers/net/e1000/em_ethdev.c >> index a0ca371b02..cd5faa4228 100644 >> --- a/drivers/net/e1000/em_ethdev.c >> +++ b/drivers/net/e1000/em_ethdev.c >> @@ -258,6 +258,7 @@ eth_em_dev_init(struct rte_eth_dev *eth_dev) >> * has already done this work. Only check we don't need a different >> * RX function */ >> if (rte_eal_process_type() != RTE_PROC_PRIMARY){ >> + e1000_setup_init_funcs(hw, TRUE); >> if (eth_dev->data->scattered_rx) >> eth_dev->rx_pkt_burst = >> (eth_rx_burst_t)ð_em_recv_scattered_pkts; >> diff --git a/drivers/net/e1000/igb_ethdev.c b/drivers/net/e1000/igb_ethdev.c >> index 10ee0f3341..7d9d60497d 100644 >> --- a/drivers/net/e1000/igb_ethdev.c >> +++ b/drivers/net/e1000/igb_ethdev.c >> @@ -737,6 +737,7 @@ eth_igb_dev_init(struct rte_eth_dev *eth_dev) >> * has already done this work. Only check we don't need a different >> * RX function */ >> if (rte_eal_process_type() != RTE_PROC_PRIMARY){ >> + e1000_setup_init_funcs(hw, TRUE); >> if (eth_dev->data->scattered_rx) >> eth_dev->rx_pkt_burst = ð_igb_recv_scattered_pkts; >> return 0; >> @@ -931,6 +932,7 @@ eth_igbvf_dev_init(struct rte_eth_dev *eth_dev) >> * has already done this work. Only check we don't need a different >> * RX function */ >> if (rte_eal_process_type() != RTE_PROC_PRIMARY){ >> + e1000_setup_init_funcs(hw, TRUE); >> if (eth_dev->data->scattered_rx) >> eth_dev->rx_pkt_burst = ð_igb_recv_scattered_pkts; >> return 0; >> -- >> 2.26.2 > > > >> this issue does not appear in ice, i40e, vmxnet3 PMD drivers. Only in e1000 , ixgbe drivers. >> Ice pmd driver gets link status by read reg directly. > For making primary & secondary working well, these drivers try to avoid save > the global data and ops function in shared data at the design of beginning. > > >> I agree with what you said "primary, the secondary is mainly for rx/tx ops officially". >> My opinion is the "set actions" shouldn't called in secondary process, but "get actions" was very common operation, they shouldn't be banned. > It's not banned, just because e1000's design introduces the global data ops in the > shared data, which is not good for sharing and accessing, since the address of the > global data ops is changed in secondary process. > > As you can see in " e1000_setup_init_funcs", they not only set function pointers, > but also call them, not sure this will break other things or not. ;-) > >> Thanks for your reply > > I think "get" operation should not crash in any process. and on different nics , this operation should be consistent. Yes , "e1000_setup_init_funcs" not only set function pointers, this patch is not good enough, just happens to work. I look forward to a better solution ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-06-22 13:59 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-06-18 17:26 [dpdk-dev] [PATCH] net/e1000: fix nic ops function was no initialized in secondary process Tengfei Zhang 2021-06-21 7:31 ` Wang, Haiyue 2021-06-21 8:34 ` 张 杨 2021-06-22 2:16 ` Wang, Haiyue 2021-06-22 13:56 ` Tengfei Zhang
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).