DPDK patches and discussions
 help / color / mirror / Atom feed
* [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

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git