* [dpdk-dev] [PATCH] virtio: Fix crash issue for secondary process @ 2015-03-19 1:45 Ouyang Changchun 2015-03-20 21:58 ` Thomas Monjalon 2015-03-27 6:35 ` [dpdk-dev] [PATCH v2] " Ouyang Changchun 0 siblings, 2 replies; 8+ messages in thread From: Ouyang Changchun @ 2015-03-19 1:45 UTC (permalink / raw) To: dev It definitely needs Rx function even in the case of secondary process, so put the assignment a bit earlier to make sure of it. Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com> --- lib/librte_pmd_virtio/virtio_ethdev.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/lib/librte_pmd_virtio/virtio_ethdev.c b/lib/librte_pmd_virtio/virtio_ethdev.c index 603be2d..ad24cf2 100644 --- a/lib/librte_pmd_virtio/virtio_ethdev.c +++ b/lib/librte_pmd_virtio/virtio_ethdev.c @@ -1113,6 +1113,7 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev) RTE_BUILD_BUG_ON(RTE_PKTMBUF_HEADROOM < sizeof(struct virtio_net_hdr)); eth_dev->dev_ops = &virtio_eth_dev_ops; + eth_dev->rx_pkt_burst = &virtio_recv_pkts; eth_dev->tx_pkt_burst = &virtio_xmit_pkts; if (rte_eal_process_type() == RTE_PROC_SECONDARY) @@ -1148,10 +1149,8 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev) if (vtpci_with_feature(hw, VIRTIO_NET_F_MRG_RXBUF)) { eth_dev->rx_pkt_burst = &virtio_recv_mergeable_pkts; hw->vtnet_hdr_size = sizeof(struct virtio_net_hdr_mrg_rxbuf); - } else { - eth_dev->rx_pkt_burst = &virtio_recv_pkts; + } else hw->vtnet_hdr_size = sizeof(struct virtio_net_hdr); - } /* Copy the permanent MAC address to: virtio_hw */ virtio_get_hwaddr(hw); -- 1.8.4.2 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [dpdk-dev] [PATCH] virtio: Fix crash issue for secondary process 2015-03-19 1:45 [dpdk-dev] [PATCH] virtio: Fix crash issue for secondary process Ouyang Changchun @ 2015-03-20 21:58 ` Thomas Monjalon 2015-03-23 14:33 ` Xie, Huawei 2015-03-27 6:35 ` [dpdk-dev] [PATCH v2] " Ouyang Changchun 1 sibling, 1 reply; 8+ messages in thread From: Thomas Monjalon @ 2015-03-20 21:58 UTC (permalink / raw) To: Ouyang Changchun; +Cc: dev 2015-03-19 09:45, Ouyang Changchun: > It definitely needs Rx function even in the case of secondary process, so put > the assignment a bit earlier to make sure of it. > > Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com> > --- > lib/librte_pmd_virtio/virtio_ethdev.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/lib/librte_pmd_virtio/virtio_ethdev.c b/lib/librte_pmd_virtio/virtio_ethdev.c > index 603be2d..ad24cf2 100644 > --- a/lib/librte_pmd_virtio/virtio_ethdev.c > +++ b/lib/librte_pmd_virtio/virtio_ethdev.c > @@ -1113,6 +1113,7 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev) > RTE_BUILD_BUG_ON(RTE_PKTMBUF_HEADROOM < sizeof(struct virtio_net_hdr)); > > eth_dev->dev_ops = &virtio_eth_dev_ops; > + eth_dev->rx_pkt_burst = &virtio_recv_pkts; > eth_dev->tx_pkt_burst = &virtio_xmit_pkts; > > if (rte_eal_process_type() == RTE_PROC_SECONDARY) > @@ -1148,10 +1149,8 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev) > if (vtpci_with_feature(hw, VIRTIO_NET_F_MRG_RXBUF)) { > eth_dev->rx_pkt_burst = &virtio_recv_mergeable_pkts; Why the mergeable buffers case is not handled for secondary processes? > hw->vtnet_hdr_size = sizeof(struct virtio_net_hdr_mrg_rxbuf); > - } else { > - eth_dev->rx_pkt_burst = &virtio_recv_pkts; > + } else > hw->vtnet_hdr_size = sizeof(struct virtio_net_hdr); > - } > > /* Copy the permanent MAC address to: virtio_hw */ > virtio_get_hwaddr(hw); ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [dpdk-dev] [PATCH] virtio: Fix crash issue for secondary process 2015-03-20 21:58 ` Thomas Monjalon @ 2015-03-23 14:33 ` Xie, Huawei 2015-03-26 1:27 ` Xie, Huawei 0 siblings, 1 reply; 8+ messages in thread From: Xie, Huawei @ 2015-03-23 14:33 UTC (permalink / raw) To: Thomas Monjalon; +Cc: dev On 3/21/2015 5:59 AM, Thomas Monjalon wrote: 2015-03-19 09:45, Ouyang Changchun: It definitely needs Rx function even in the case of secondary process, so put the assignment a bit earlier to make sure of it. Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com><mailto:changchun.ouyang@intel.com> --- lib/librte_pmd_virtio/virtio_ethdev.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/lib/librte_pmd_virtio/virtio_ethdev.c b/lib/librte_pmd_virtio/virtio_ethdev.c index 603be2d..ad24cf2 100644 --- a/lib/librte_pmd_virtio/virtio_ethdev.c +++ b/lib/librte_pmd_virtio/virtio_ethdev.c @@ -1113,6 +1113,7 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev) RTE_BUILD_BUG_ON(RTE_PKTMBUF_HEADROOM < sizeof(struct virtio_net_hdr)); eth_dev->dev_ops = &virtio_eth_dev_ops; + eth_dev->rx_pkt_burst = &virtio_recv_pkts; eth_dev->tx_pkt_burst = &virtio_xmit_pkts; if (rte_eal_process_type() == RTE_PROC_SECONDARY) @@ -1148,10 +1149,8 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev) if (vtpci_with_feature(hw, VIRTIO_NET_F_MRG_RXBUF)) { eth_dev->rx_pkt_burst = &virtio_recv_mergeable_pkts; Why the mergeable buffers case is not handled for secondary processes? Forgot to CC my comment to dpdk.org. There are many parts of eth_dev for the secondary process still uninitialized, even like eth_dev->data->mac_addrs isn't allocated., also like mergeable, feature, etc. Secondary process will not work unless they never touch those fields. Prefer we have a clean fix. Customer could apply that one line of fix if they need this urgently. I am wondering whether other PMDS have the same issue for the second process. hw->vtnet_hdr_size = sizeof(struct virtio_net_hdr_mrg_rxbuf); - } else { - eth_dev->rx_pkt_burst = &virtio_recv_pkts; + } else hw->vtnet_hdr_size = sizeof(struct virtio_net_hdr); - } /* Copy the permanent MAC address to: virtio_hw */ virtio_get_hwaddr(hw); ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [dpdk-dev] [PATCH] virtio: Fix crash issue for secondary process 2015-03-23 14:33 ` Xie, Huawei @ 2015-03-26 1:27 ` Xie, Huawei 0 siblings, 0 replies; 8+ messages in thread From: Xie, Huawei @ 2015-03-26 1:27 UTC (permalink / raw) To: Xie, Huawei; +Cc: dev On 3/23/2015 10:33 PM, Xie, Huawei wrote: > On 3/21/2015 5:59 AM, Thomas Monjalon wrote: > > 2015-03-19 09:45, Ouyang Changchun: > > > It definitely needs Rx function even in the case of secondary process, so put > the assignment a bit earlier to make sure of it. > > Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com><mailto:changchun.ouyang@intel.com> > --- > lib/librte_pmd_virtio/virtio_ethdev.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/lib/librte_pmd_virtio/virtio_ethdev.c b/lib/librte_pmd_virtio/virtio_ethdev.c > index 603be2d..ad24cf2 100644 > --- a/lib/librte_pmd_virtio/virtio_ethdev.c > +++ b/lib/librte_pmd_virtio/virtio_ethdev.c > @@ -1113,6 +1113,7 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev) > RTE_BUILD_BUG_ON(RTE_PKTMBUF_HEADROOM < sizeof(struct virtio_net_hdr)); > > eth_dev->dev_ops = &virtio_eth_dev_ops; > + eth_dev->rx_pkt_burst = &virtio_recv_pkts; > eth_dev->tx_pkt_burst = &virtio_xmit_pkts; > > if (rte_eal_process_type() == RTE_PROC_SECONDARY) > @@ -1148,10 +1149,8 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev) > if (vtpci_with_feature(hw, VIRTIO_NET_F_MRG_RXBUF)) { > eth_dev->rx_pkt_burst = &virtio_recv_mergeable_pkts; > > > > Why the mergeable buffers case is not handled for secondary processes? > > Forgot to CC my comment to dpdk.org. > There are many parts of eth_dev for the secondary process still > > uninitialized, even like eth_dev->data->mac_addrs isn't allocated., also > > like mergeable, feature, etc. > > Secondary process will not work unless they never touch those fields. > This comment is not right, but there is indeed a possible minor issue here. eth_dev_data array is shared across multiple processes. The eth_dev_data is the same for the same eth device unless the port id allocated to it is the same, i.e, the devices are scanned in the same order in all processes. For instance, if we blacklist a device in one process, the port id will be then different. > > Prefer we have a clean fix. Customer could apply that one line of fix if > > they need this urgently. > > I am wondering whether other PMDS have the same issue for the second process. > > > > > > hw->vtnet_hdr_size = sizeof(struct virtio_net_hdr_mrg_rxbuf); > - } else { > - eth_dev->rx_pkt_burst = &virtio_recv_pkts; > + } else > hw->vtnet_hdr_size = sizeof(struct virtio_net_hdr); > - } > > /* Copy the permanent MAC address to: virtio_hw */ > virtio_get_hwaddr(hw); > > > > > > > > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* [dpdk-dev] [PATCH v2] virtio: Fix crash issue for secondary process 2015-03-19 1:45 [dpdk-dev] [PATCH] virtio: Fix crash issue for secondary process Ouyang Changchun 2015-03-20 21:58 ` Thomas Monjalon @ 2015-03-27 6:35 ` Ouyang Changchun 2015-03-27 8:57 ` Thomas Monjalon 2015-03-27 13:23 ` [dpdk-dev] [PATCH v3] " Ouyang Changchun 1 sibling, 2 replies; 8+ messages in thread From: Ouyang Changchun @ 2015-03-27 6:35 UTC (permalink / raw) To: dev It needs Rx function even in the case of secondary process, and it also needs check if it supports mergeable feature or not. Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com> --- Changes in v2: -- Check if it supports mergeable or not for the secondary process. lib/librte_pmd_virtio/virtio_ethdev.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/lib/librte_pmd_virtio/virtio_ethdev.c b/lib/librte_pmd_virtio/virtio_ethdev.c index 603be2d..0156b56 100644 --- a/lib/librte_pmd_virtio/virtio_ethdev.c +++ b/lib/librte_pmd_virtio/virtio_ethdev.c @@ -1115,8 +1115,13 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev) eth_dev->dev_ops = &virtio_eth_dev_ops; eth_dev->tx_pkt_burst = &virtio_xmit_pkts; - if (rte_eal_process_type() == RTE_PROC_SECONDARY) + if (rte_eal_process_type() == RTE_PROC_SECONDARY) { + if (vtpci_with_feature(hw, VIRTIO_NET_F_MRG_RXBUF)) + eth_dev->rx_pkt_burst = &virtio_recv_mergeable_pkts; + else + eth_dev->rx_pkt_burst = &virtio_recv_pkts; return 0; + } /* Allocate memory for storing MAC addresses */ eth_dev->data->mac_addrs = rte_zmalloc("virtio", ETHER_ADDR_LEN, 0); -- 1.8.4.2 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [dpdk-dev] [PATCH v2] virtio: Fix crash issue for secondary process 2015-03-27 6:35 ` [dpdk-dev] [PATCH v2] " Ouyang Changchun @ 2015-03-27 8:57 ` Thomas Monjalon 2015-03-27 13:23 ` [dpdk-dev] [PATCH v3] " Ouyang Changchun 1 sibling, 0 replies; 8+ messages in thread From: Thomas Monjalon @ 2015-03-27 8:57 UTC (permalink / raw) To: Ouyang Changchun; +Cc: dev 2015-03-27 14:35, Ouyang Changchun: > It needs Rx function even in the case of secondary process, and it also needs check if > it supports mergeable feature or not. > > Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com> > --- > > Changes in v2: > -- Check if it supports mergeable or not for the secondary process. > --- a/lib/librte_pmd_virtio/virtio_ethdev.c > +++ b/lib/librte_pmd_virtio/virtio_ethdev.c > @@ -1115,8 +1115,13 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev) > eth_dev->dev_ops = &virtio_eth_dev_ops; > eth_dev->tx_pkt_burst = &virtio_xmit_pkts; > > - if (rte_eal_process_type() == RTE_PROC_SECONDARY) > + if (rte_eal_process_type() == RTE_PROC_SECONDARY) { > + if (vtpci_with_feature(hw, VIRTIO_NET_F_MRG_RXBUF)) > + eth_dev->rx_pkt_burst = &virtio_recv_mergeable_pkts; > + else > + eth_dev->rx_pkt_burst = &virtio_recv_pkts; You are duplicating code, making it error prone for later maintenance. Please merge primary and secondary cases above the if() block. > return 0; > + } > > /* Allocate memory for storing MAC addresses */ > eth_dev->data->mac_addrs = rte_zmalloc("virtio", ETHER_ADDR_LEN, 0); > ^ permalink raw reply [flat|nested] 8+ messages in thread
* [dpdk-dev] [PATCH v3] virtio: Fix crash issue for secondary process 2015-03-27 6:35 ` [dpdk-dev] [PATCH v2] " Ouyang Changchun 2015-03-27 8:57 ` Thomas Monjalon @ 2015-03-27 13:23 ` Ouyang Changchun 2015-03-30 20:09 ` Thomas Monjalon 1 sibling, 1 reply; 8+ messages in thread From: Ouyang Changchun @ 2015-03-27 13:23 UTC (permalink / raw) To: dev It needs Rx function even in the case of secondary process, and it also needs check if it supports mergeable feature or not. Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com> --- Changes in v3 -- Extract a function to remove the duplicated codes; Changes in v2: -- Check if it supports mergeable or not for the secondary process. lib/librte_pmd_virtio/virtio_ethdev.c | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/lib/librte_pmd_virtio/virtio_ethdev.c b/lib/librte_pmd_virtio/virtio_ethdev.c index 603be2d..d462b9d 100644 --- a/lib/librte_pmd_virtio/virtio_ethdev.c +++ b/lib/librte_pmd_virtio/virtio_ethdev.c @@ -1097,6 +1097,16 @@ virtio_interrupt_handler(__rte_unused struct rte_intr_handle *handle, } +static void +rx_func_get(struct rte_eth_dev *eth_dev) +{ + struct virtio_hw *hw = eth_dev->data->dev_private; + if (vtpci_with_feature(hw, VIRTIO_NET_F_MRG_RXBUF)) + eth_dev->rx_pkt_burst = &virtio_recv_mergeable_pkts; + else + eth_dev->rx_pkt_burst = &virtio_recv_pkts; +} + /* * This function is based on probe() function in virtio_pci.c * It returns 0 on success. @@ -1115,8 +1125,10 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev) eth_dev->dev_ops = &virtio_eth_dev_ops; eth_dev->tx_pkt_burst = &virtio_xmit_pkts; - if (rte_eal_process_type() == RTE_PROC_SECONDARY) + if (rte_eal_process_type() == RTE_PROC_SECONDARY) { + rx_func_get(eth_dev); return 0; + } /* Allocate memory for storing MAC addresses */ eth_dev->data->mac_addrs = rte_zmalloc("virtio", ETHER_ADDR_LEN, 0); @@ -1144,14 +1156,13 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev) vtpci_set_status(hw, VIRTIO_CONFIG_STATUS_DRIVER); virtio_negotiate_features(hw); + rx_func_get(eth_dev); + /* Setting up rx_header size for the device */ - if (vtpci_with_feature(hw, VIRTIO_NET_F_MRG_RXBUF)) { - eth_dev->rx_pkt_burst = &virtio_recv_mergeable_pkts; + if (vtpci_with_feature(hw, VIRTIO_NET_F_MRG_RXBUF)) hw->vtnet_hdr_size = sizeof(struct virtio_net_hdr_mrg_rxbuf); - } else { - eth_dev->rx_pkt_burst = &virtio_recv_pkts; + else hw->vtnet_hdr_size = sizeof(struct virtio_net_hdr); - } /* Copy the permanent MAC address to: virtio_hw */ virtio_get_hwaddr(hw); -- 1.8.4.2 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [dpdk-dev] [PATCH v3] virtio: Fix crash issue for secondary process 2015-03-27 13:23 ` [dpdk-dev] [PATCH v3] " Ouyang Changchun @ 2015-03-30 20:09 ` Thomas Monjalon 0 siblings, 0 replies; 8+ messages in thread From: Thomas Monjalon @ 2015-03-30 20:09 UTC (permalink / raw) To: Ouyang Changchun; +Cc: dev 2015-03-27 21:23, Ouyang Changchun: > It needs Rx function even in the case of secondary process, and it also needs check if > it supports mergeable feature or not. > > Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com> > --- > > Changes in v3 > -- Extract a function to remove the duplicated codes; > > Changes in v2: > -- Check if it supports mergeable or not for the secondary process. Applied, thanks ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2015-03-30 20:09 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-03-19 1:45 [dpdk-dev] [PATCH] virtio: Fix crash issue for secondary process Ouyang Changchun 2015-03-20 21:58 ` Thomas Monjalon 2015-03-23 14:33 ` Xie, Huawei 2015-03-26 1:27 ` Xie, Huawei 2015-03-27 6:35 ` [dpdk-dev] [PATCH v2] " Ouyang Changchun 2015-03-27 8:57 ` Thomas Monjalon 2015-03-27 13:23 ` [dpdk-dev] [PATCH v3] " Ouyang Changchun 2015-03-30 20:09 ` Thomas Monjalon
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).