[PATCH] virtio: Fix crash issue for secondary process

[PATCH] virtio: Fix crash issue for secondary process

[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-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 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

Re: [PATCH] virtio: Fix crash issue for secondary process

[Thomas Monjalon]

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-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
>  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);

Re: [PATCH] virtio: Fix crash issue for secondary process

[Xie, Huawei]

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-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org><mailto:changchun.ouyang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 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);

Re: [PATCH] virtio: Fix crash issue for secondary process

[Xie, Huawei]

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-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org><mailto:changchun.ouyang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
>  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);
>
>
>
>
>
>
>
>
>

[PATCH v2] virtio: Fix crash issue for secondary process

[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-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---

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

Re: [PATCH v2] virtio: Fix crash issue for secondary process

[Xie, Huawei]

On 3/27/2015 2:35 PM, Ouyang, Changchun wrote:
> 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-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
>
> 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);
Acked-by: Huawei Xie <huawei.xie-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

Re: [PATCH v2] virtio: Fix crash issue for secondary process

[Thomas Monjalon]

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-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
> 
> 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);
> 

[PATCH v3] virtio: Fix crash issue for secondary process

[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-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---

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

Re: [PATCH v3] virtio: Fix crash issue for secondary process

[Thomas Monjalon]

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-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
> 
> 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