Re: [dpdk-dev] [PATCH] net/vhost: fix vhost invalid state

Re: [dpdk-dev] [PATCH] net/vhost: fix vhost invalid state

[Tan, Jianfeng]

Hi Junjie,

I think the code is still buggy. As vhost thread and master thread are 
separately invoking new_device() and dev_start().

On 4/10/2018 10:18 PM, Junjie Chen wrote:
> dev_start sets *dev_attached* after setup queues, this sets device to
> invalid state since no frontend is attached. Also destroy_device set
> *started* to zero which makes *allow_queuing* always zero until dev_start
> get called again. Actually, we should not determine queues existence by
> *dev_attached* but by queues pointers or other separated variable(s).
>
> Fixes: 30a701a53737 ("net/vhost: fix crash when creating vdev
> dynamically")
>
> Signed-off-by: Junjie Chen <junjie.j.chen@intel.com>
> ---
>   drivers/net/vhost/rte_eth_vhost.c | 64 +++++++++++++++++++++++----------------
>   1 file changed, 38 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c
> index 11b6076..6a2ff76 100644
> --- a/drivers/net/vhost/rte_eth_vhost.c
> +++ b/drivers/net/vhost/rte_eth_vhost.c
> @@ -118,6 +118,7 @@ struct pmd_internal {
>   	char *iface_name;
>   	uint16_t max_queues;
>   	uint16_t vid;
> +	uint16_t queue_ready;

If we can reuse below *started* field, instead of introducing a new 
variable?

>   	rte_atomic32_t started;
>   };
>   
> @@ -528,10 +529,13 @@ update_queuing_status(struct rte_eth_dev *dev)
>   	unsigned int i;
>   	int allow_queuing = 1;
>   
> -	if (rte_atomic32_read(&internal->dev_attached) == 0)
> +	if (!dev->data->rx_queues || !dev->data->tx_queues) {
> +		RTE_LOG(ERR, PMD, "RX/TX queues not setup yet\n");
>   		return;
> +	}
>   
> -	if (rte_atomic32_read(&internal->started) == 0)
> +	if (rte_atomic32_read(&internal->started) == 0 ||
> +	    rte_atomic32_read(&internal->dev_attached) == 0)
>   		allow_queuing = 0;
>   
>   	/* Wait until rx/tx_pkt_burst stops accessing vhost device */
> @@ -576,6 +580,8 @@ queue_setup(struct rte_eth_dev *eth_dev, struct pmd_internal *internal)
>   		vq->internal = internal;
>   		vq->port = eth_dev->data->port_id;
>   	}
> +
> +	internal->queue_ready = 1;
>   }
>   
>   static int
> @@ -607,13 +613,10 @@ new_device(int vid)
>   #endif
>   
>   	internal->vid = vid;
> -	if (eth_dev->data->rx_queues && eth_dev->data->tx_queues) {
> +	if (eth_dev->data->rx_queues && eth_dev->data->tx_queues)
>   		queue_setup(eth_dev, internal);
> -		rte_atomic32_set(&internal->dev_attached, 1);
> -	} else {
> -		RTE_LOG(INFO, PMD, "RX/TX queues have not setup yet\n");
> -		rte_atomic32_set(&internal->dev_attached, 0);
> -	}
> +	else

vhost thread (t1): goes here and before setting dev_attached ...

> +		RTE_LOG(INFO, PMD, "RX/TX queues not setup yet\n");
>   
>   	for (i = 0; i < rte_vhost_get_vring_num(vid); i++)
>   		rte_vhost_enable_guest_notification(vid, i, 0);
> @@ -622,6 +625,7 @@ new_device(int vid)
>   
>   	eth_dev->data->dev_link.link_status = ETH_LINK_UP;
>   
> +	rte_atomic32_set(&internal->dev_attached, 1);
>   	update_queuing_status(eth_dev);
>   
>   	RTE_LOG(INFO, PMD, "Vhost device %d created\n", vid);
> @@ -657,17 +661,19 @@ destroy_device(int vid)
>   
>   	eth_dev->data->dev_link.link_status = ETH_LINK_DOWN;
>   
> -	for (i = 0; i < eth_dev->data->nb_rx_queues; i++) {
> -		vq = eth_dev->data->rx_queues[i];
> -		if (vq == NULL)
> -			continue;
> -		vq->vid = -1;
> -	}
> -	for (i = 0; i < eth_dev->data->nb_tx_queues; i++) {
> -		vq = eth_dev->data->tx_queues[i];
> -		if (vq == NULL)
> -			continue;
> -		vq->vid = -1;
> +	if (eth_dev->data->rx_queues && eth_dev->data->tx_queues) {
> +		for (i = 0; i < eth_dev->data->nb_rx_queues; i++) {
> +			vq = eth_dev->data->rx_queues[i];
> +			if (!vq)
> +				continue;
> +			vq->vid = -1;
> +		}
> +		for (i = 0; i < eth_dev->data->nb_tx_queues; i++) {
> +			vq = eth_dev->data->tx_queues[i];
> +			if (!vq)
> +				continue;
> +			vq->vid = -1;
> +		}
>   	}
>   
>   	state = vring_states[eth_dev->data->port_id];
> @@ -792,11 +798,14 @@ eth_dev_start(struct rte_eth_dev *eth_dev)
>   {
>   	struct pmd_internal *internal = eth_dev->data->dev_private;
>   
> -	if (unlikely(rte_atomic32_read(&internal->dev_attached) == 0)) {
> -		queue_setup(eth_dev, internal);
> -		rte_atomic32_set(&internal->dev_attached, 1);
> +	if (!eth_dev->data->rx_queues || !eth_dev->data->tx_queues) {
> +		RTE_LOG(ERR, PMD, "RX/TX queues not exist yet\n");
> +		return -1;
>   	}

I think in DPDK, we can make the assumption that dev_start() comes later 
than queue setup. So we don't need this check.

>   
> +	if (!internal->queue_ready)
> +		queue_setup(eth_dev, internal);

Master thread (t2): setting invalid values.

> +
>   	rte_atomic32_set(&internal->started, 1);
>   	update_queuing_status(eth_dev);
>   
> @@ -836,10 +845,13 @@ eth_dev_close(struct rte_eth_dev *dev)
>   	pthread_mutex_unlock(&internal_list_lock);
>   	rte_free(list);
>   
> -	for (i = 0; i < dev->data->nb_rx_queues; i++)
> -		rte_free(dev->data->rx_queues[i]);
> -	for (i = 0; i < dev->data->nb_tx_queues; i++)
> -		rte_free(dev->data->tx_queues[i]);
> +	if (dev->data->rx_queues)
> +		for (i = 0; i < dev->data->nb_rx_queues; i++)
> +			rte_free(dev->data->rx_queues[i]);
> +
> +	if (dev->data->tx_queues)
> +		for (i = 0; i < dev->data->nb_tx_queues; i++)
> +			rte_free(dev->data->tx_queues[i]);
>   
>   	rte_free(dev->data->mac_addrs);
>   	free(internal->dev_name);

Re: [dpdk-dev] [PATCH] net/vhost: fix vhost invalid state

[Jens Freimann]

On Tue, Apr 10, 2018 at 10:18:09AM -0400, Junjie Chen wrote:
>dev_start sets *dev_attached* after setup queues, this sets device to
>invalid state since no frontend is attached. Also destroy_device set
>*started* to zero which makes *allow_queuing* always zero until dev_start
>get called again. Actually, we should not determine queues existence by
>*dev_attached* but by queues pointers or other separated variable(s).
>
>Fixes: 30a701a53737 ("net/vhost: fix crash when creating vdev
>dynamically")
>
>Signed-off-by: Junjie Chen <junjie.j.chen@intel.com>

So this fixes the problem I saw with allow_queueing always being zero
and the error message "VHOST_CONFIG: (0) device not found". 

However with this patch on top of virtio-next/master no packets are
being forwarded to the guest and back anymore.

When I use virtio-next/master and revert 30a701a53737 both works fine. 

regards,
Jens 

>---
> drivers/net/vhost/rte_eth_vhost.c | 64 +++++++++++++++++++++++----------------
> 1 file changed, 38 insertions(+), 26 deletions(-)
>
>diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c
>index 11b6076..6a2ff76 100644
>--- a/drivers/net/vhost/rte_eth_vhost.c
>+++ b/drivers/net/vhost/rte_eth_vhost.c
>@@ -118,6 +118,7 @@ struct pmd_internal {
> 	char *iface_name;
> 	uint16_t max_queues;
> 	uint16_t vid;
>+	uint16_t queue_ready;
> 	rte_atomic32_t started;
> };
>
>@@ -528,10 +529,13 @@ update_queuing_status(struct rte_eth_dev *dev)
> 	unsigned int i;
> 	int allow_queuing = 1;
>
>-	if (rte_atomic32_read(&internal->dev_attached) == 0)
>+	if (!dev->data->rx_queues || !dev->data->tx_queues) {
>+		RTE_LOG(ERR, PMD, "RX/TX queues not setup yet\n");
> 		return;
>+	}
>
>-	if (rte_atomic32_read(&internal->started) == 0)
>+	if (rte_atomic32_read(&internal->started) == 0 ||
>+	    rte_atomic32_read(&internal->dev_attached) == 0)
> 		allow_queuing = 0;
>
> 	/* Wait until rx/tx_pkt_burst stops accessing vhost device */
>@@ -576,6 +580,8 @@ queue_setup(struct rte_eth_dev *eth_dev, struct pmd_internal *internal)
> 		vq->internal = internal;
> 		vq->port = eth_dev->data->port_id;
> 	}
>+
>+	internal->queue_ready = 1;
> }
>
> static int
>@@ -607,13 +613,10 @@ new_device(int vid)
> #endif
>
> 	internal->vid = vid;
>-	if (eth_dev->data->rx_queues && eth_dev->data->tx_queues) {
>+	if (eth_dev->data->rx_queues && eth_dev->data->tx_queues)
> 		queue_setup(eth_dev, internal);
>-		rte_atomic32_set(&internal->dev_attached, 1);
>-	} else {
>-		RTE_LOG(INFO, PMD, "RX/TX queues have not setup yet\n");
>-		rte_atomic32_set(&internal->dev_attached, 0);
>-	}
>+	else
>+		RTE_LOG(INFO, PMD, "RX/TX queues not setup yet\n");
>
> 	for (i = 0; i < rte_vhost_get_vring_num(vid); i++)
> 		rte_vhost_enable_guest_notification(vid, i, 0);
>@@ -622,6 +625,7 @@ new_device(int vid)
>
> 	eth_dev->data->dev_link.link_status = ETH_LINK_UP;
>
>+	rte_atomic32_set(&internal->dev_attached, 1);
> 	update_queuing_status(eth_dev);
>
> 	RTE_LOG(INFO, PMD, "Vhost device %d created\n", vid);
>@@ -657,17 +661,19 @@ destroy_device(int vid)
>
> 	eth_dev->data->dev_link.link_status = ETH_LINK_DOWN;
>
>-	for (i = 0; i < eth_dev->data->nb_rx_queues; i++) {
>-		vq = eth_dev->data->rx_queues[i];
>-		if (vq == NULL)
>-			continue;
>-		vq->vid = -1;
>-	}
>-	for (i = 0; i < eth_dev->data->nb_tx_queues; i++) {
>-		vq = eth_dev->data->tx_queues[i];
>-		if (vq == NULL)
>-			continue;
>-		vq->vid = -1;
>+	if (eth_dev->data->rx_queues && eth_dev->data->tx_queues) {
>+		for (i = 0; i < eth_dev->data->nb_rx_queues; i++) {
>+			vq = eth_dev->data->rx_queues[i];
>+			if (!vq)
>+				continue;
>+			vq->vid = -1;
>+		}
>+		for (i = 0; i < eth_dev->data->nb_tx_queues; i++) {
>+			vq = eth_dev->data->tx_queues[i];
>+			if (!vq)
>+				continue;
>+			vq->vid = -1;
>+		}
> 	}
>
> 	state = vring_states[eth_dev->data->port_id];
>@@ -792,11 +798,14 @@ eth_dev_start(struct rte_eth_dev *eth_dev)
> {
> 	struct pmd_internal *internal = eth_dev->data->dev_private;
>
>-	if (unlikely(rte_atomic32_read(&internal->dev_attached) == 0)) {
>-		queue_setup(eth_dev, internal);
>-		rte_atomic32_set(&internal->dev_attached, 1);
>+	if (!eth_dev->data->rx_queues || !eth_dev->data->tx_queues) {
>+		RTE_LOG(ERR, PMD, "RX/TX queues not exist yet\n");
>+		return -1;
> 	}
>
>+	if (!internal->queue_ready)
>+		queue_setup(eth_dev, internal);
>+
> 	rte_atomic32_set(&internal->started, 1);
> 	update_queuing_status(eth_dev);
>
>@@ -836,10 +845,13 @@ eth_dev_close(struct rte_eth_dev *dev)
> 	pthread_mutex_unlock(&internal_list_lock);
> 	rte_free(list);
>
>-	for (i = 0; i < dev->data->nb_rx_queues; i++)
>-		rte_free(dev->data->rx_queues[i]);
>-	for (i = 0; i < dev->data->nb_tx_queues; i++)
>-		rte_free(dev->data->tx_queues[i]);
>+	if (dev->data->rx_queues)
>+		for (i = 0; i < dev->data->nb_rx_queues; i++)
>+			rte_free(dev->data->rx_queues[i]);
>+
>+	if (dev->data->tx_queues)
>+		for (i = 0; i < dev->data->nb_tx_queues; i++)
>+			rte_free(dev->data->tx_queues[i]);
>
> 	rte_free(dev->data->mac_addrs);
> 	free(internal->dev_name);
>-- 
>2.0.1
>

[dpdk-dev] [PATCH] net/vhost: fix vhost invalid state

[Junjie Chen]

dev_start sets *dev_attached* after setup queues, this sets device to
invalid state since no frontend is attached. Also destroy_device set
*started* to zero which makes *allow_queuing* always zero until dev_start
get called again. Actually, we should not determine queues existence by
*dev_attached* but by queues pointers or other separated variable(s).

Fixes: 30a701a53737 ("net/vhost: fix crash when creating vdev
dynamically")

Signed-off-by: Junjie Chen <junjie.j.chen@intel.com>
---
 drivers/net/vhost/rte_eth_vhost.c | 64 +++++++++++++++++++++++----------------
 1 file changed, 38 insertions(+), 26 deletions(-)

diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c
index 11b6076..6a2ff76 100644
--- a/drivers/net/vhost/rte_eth_vhost.c
+++ b/drivers/net/vhost/rte_eth_vhost.c
@@ -118,6 +118,7 @@ struct pmd_internal {
 	char *iface_name;
 	uint16_t max_queues;
 	uint16_t vid;
+	uint16_t queue_ready;
 	rte_atomic32_t started;
 };
 
@@ -528,10 +529,13 @@ update_queuing_status(struct rte_eth_dev *dev)
 	unsigned int i;
 	int allow_queuing = 1;
 
-	if (rte_atomic32_read(&internal->dev_attached) == 0)
+	if (!dev->data->rx_queues || !dev->data->tx_queues) {
+		RTE_LOG(ERR, PMD, "RX/TX queues not setup yet\n");
 		return;
+	}
 
-	if (rte_atomic32_read(&internal->started) == 0)
+	if (rte_atomic32_read(&internal->started) == 0 ||
+	    rte_atomic32_read(&internal->dev_attached) == 0)
 		allow_queuing = 0;
 
 	/* Wait until rx/tx_pkt_burst stops accessing vhost device */
@@ -576,6 +580,8 @@ queue_setup(struct rte_eth_dev *eth_dev, struct pmd_internal *internal)
 		vq->internal = internal;
 		vq->port = eth_dev->data->port_id;
 	}
+
+	internal->queue_ready = 1;
 }
 
 static int
@@ -607,13 +613,10 @@ new_device(int vid)
 #endif
 
 	internal->vid = vid;
-	if (eth_dev->data->rx_queues && eth_dev->data->tx_queues) {
+	if (eth_dev->data->rx_queues && eth_dev->data->tx_queues)
 		queue_setup(eth_dev, internal);
-		rte_atomic32_set(&internal->dev_attached, 1);
-	} else {
-		RTE_LOG(INFO, PMD, "RX/TX queues have not setup yet\n");
-		rte_atomic32_set(&internal->dev_attached, 0);
-	}
+	else
+		RTE_LOG(INFO, PMD, "RX/TX queues not setup yet\n");
 
 	for (i = 0; i < rte_vhost_get_vring_num(vid); i++)
 		rte_vhost_enable_guest_notification(vid, i, 0);
@@ -622,6 +625,7 @@ new_device(int vid)
 
 	eth_dev->data->dev_link.link_status = ETH_LINK_UP;
 
+	rte_atomic32_set(&internal->dev_attached, 1);
 	update_queuing_status(eth_dev);
 
 	RTE_LOG(INFO, PMD, "Vhost device %d created\n", vid);
@@ -657,17 +661,19 @@ destroy_device(int vid)
 
 	eth_dev->data->dev_link.link_status = ETH_LINK_DOWN;
 
-	for (i = 0; i < eth_dev->data->nb_rx_queues; i++) {
-		vq = eth_dev->data->rx_queues[i];
-		if (vq == NULL)
-			continue;
-		vq->vid = -1;
-	}
-	for (i = 0; i < eth_dev->data->nb_tx_queues; i++) {
-		vq = eth_dev->data->tx_queues[i];
-		if (vq == NULL)
-			continue;
-		vq->vid = -1;
+	if (eth_dev->data->rx_queues && eth_dev->data->tx_queues) {
+		for (i = 0; i < eth_dev->data->nb_rx_queues; i++) {
+			vq = eth_dev->data->rx_queues[i];
+			if (!vq)
+				continue;
+			vq->vid = -1;
+		}
+		for (i = 0; i < eth_dev->data->nb_tx_queues; i++) {
+			vq = eth_dev->data->tx_queues[i];
+			if (!vq)
+				continue;
+			vq->vid = -1;
+		}
 	}
 
 	state = vring_states[eth_dev->data->port_id];
@@ -792,11 +798,14 @@ eth_dev_start(struct rte_eth_dev *eth_dev)
 {
 	struct pmd_internal *internal = eth_dev->data->dev_private;
 
-	if (unlikely(rte_atomic32_read(&internal->dev_attached) == 0)) {
-		queue_setup(eth_dev, internal);
-		rte_atomic32_set(&internal->dev_attached, 1);
+	if (!eth_dev->data->rx_queues || !eth_dev->data->tx_queues) {
+		RTE_LOG(ERR, PMD, "RX/TX queues not exist yet\n");
+		return -1;
 	}
 
+	if (!internal->queue_ready)
+		queue_setup(eth_dev, internal);
+
 	rte_atomic32_set(&internal->started, 1);
 	update_queuing_status(eth_dev);
 
@@ -836,10 +845,13 @@ eth_dev_close(struct rte_eth_dev *dev)
 	pthread_mutex_unlock(&internal_list_lock);
 	rte_free(list);
 
-	for (i = 0; i < dev->data->nb_rx_queues; i++)
-		rte_free(dev->data->rx_queues[i]);
-	for (i = 0; i < dev->data->nb_tx_queues; i++)
-		rte_free(dev->data->tx_queues[i]);
+	if (dev->data->rx_queues)
+		for (i = 0; i < dev->data->nb_rx_queues; i++)
+			rte_free(dev->data->rx_queues[i]);
+
+	if (dev->data->tx_queues)
+		for (i = 0; i < dev->data->nb_tx_queues; i++)
+			rte_free(dev->data->tx_queues[i]);
 
 	rte_free(dev->data->mac_addrs);
 	free(internal->dev_name);
-- 
2.0.1

Re: [dpdk-dev] [PATCH] net/vhost: fix vhost invalid state

[Chen, Junjie J]

Thanks to point this out, I forgot to run 'git add' before generating patch.

Need following change also: 
--- a/drivers/net/vhost/rte_eth_vhost.c
+++ b/drivers/net/vhost/rte_eth_vhost.c
@@ -655,9 +655,8 @@ destroy_device(int vid)
        eth_dev = list->eth_dev;
        internal = eth_dev->data->dev_private;

-       rte_atomic32_set(&internal->started, 0);
-       update_queuing_status(eth_dev);
        rte_atomic32_set(&internal->dev_attached, 0);
+       update_queuing_status(eth_dev);

        eth_dev->data->dev_link.link_status = ETH_LINK_DOWN;


Will send out V2 to include this.


Cheers
JJ


> -----Original Message-----
> From: Jens Freimann [mailto:jfreimann@redhat.com]
> Sent: Tuesday, April 10, 2018 7:13 PM
> To: Chen, Junjie J <junjie.j.chen@intel.com>
> Cc: Tan, Jianfeng <jianfeng.tan@intel.com>; maxime.coquelin@redhat.com;
> mtetsuyah@gmail.com; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] net/vhost: fix vhost invalid state
> 
> On Tue, Apr 10, 2018 at 10:18:09AM -0400, Junjie Chen wrote:
> >dev_start sets *dev_attached* after setup queues, this sets device to
> >invalid state since no frontend is attached. Also destroy_device set
> >*started* to zero which makes *allow_queuing* always zero until
> >dev_start get called again. Actually, we should not determine queues
> >existence by
> >*dev_attached* but by queues pointers or other separated variable(s).
> >
> >Fixes: 30a701a53737 ("net/vhost: fix crash when creating vdev
> >dynamically")
> >
> >Signed-off-by: Junjie Chen <junjie.j.chen@intel.com>
> 
> So this fixes the problem I saw with allow_queueing always being zero and the
> error message "VHOST_CONFIG: (0) device not found".
> 
> However with this patch on top of virtio-next/master no packets are being
> forwarded to the guest and back anymore.
> 
> When I use virtio-next/master and revert 30a701a53737 both works fine.
> 
> regards,
> Jens
> 
> >---
> > drivers/net/vhost/rte_eth_vhost.c | 64
> >+++++++++++++++++++++++----------------
> > 1 file changed, 38 insertions(+), 26 deletions(-)
> >
> >diff --git a/drivers/net/vhost/rte_eth_vhost.c
> >b/drivers/net/vhost/rte_eth_vhost.c
> >index 11b6076..6a2ff76 100644
> >--- a/drivers/net/vhost/rte_eth_vhost.c
> >+++ b/drivers/net/vhost/rte_eth_vhost.c
> >@@ -118,6 +118,7 @@ struct pmd_internal {
> > 	char *iface_name;
> > 	uint16_t max_queues;
> > 	uint16_t vid;
> >+	uint16_t queue_ready;
> > 	rte_atomic32_t started;
> > };
> >
> >@@ -528,10 +529,13 @@ update_queuing_status(struct rte_eth_dev *dev)
> > 	unsigned int i;
> > 	int allow_queuing = 1;
> >
> >-	if (rte_atomic32_read(&internal->dev_attached) == 0)
> >+	if (!dev->data->rx_queues || !dev->data->tx_queues) {
> >+		RTE_LOG(ERR, PMD, "RX/TX queues not setup yet\n");
> > 		return;
> >+	}
> >
> >-	if (rte_atomic32_read(&internal->started) == 0)
> >+	if (rte_atomic32_read(&internal->started) == 0 ||
> >+	    rte_atomic32_read(&internal->dev_attached) == 0)
> > 		allow_queuing = 0;
> >
> > 	/* Wait until rx/tx_pkt_burst stops accessing vhost device */ @@
> >-576,6 +580,8 @@ queue_setup(struct rte_eth_dev *eth_dev, struct
> pmd_internal *internal)
> > 		vq->internal = internal;
> > 		vq->port = eth_dev->data->port_id;
> > 	}
> >+
> >+	internal->queue_ready = 1;
> > }
> >
> > static int
> >@@ -607,13 +613,10 @@ new_device(int vid)  #endif
> >
> > 	internal->vid = vid;
> >-	if (eth_dev->data->rx_queues && eth_dev->data->tx_queues) {
> >+	if (eth_dev->data->rx_queues && eth_dev->data->tx_queues)
> > 		queue_setup(eth_dev, internal);
> >-		rte_atomic32_set(&internal->dev_attached, 1);
> >-	} else {
> >-		RTE_LOG(INFO, PMD, "RX/TX queues have not setup yet\n");
> >-		rte_atomic32_set(&internal->dev_attached, 0);
> >-	}
> >+	else
> >+		RTE_LOG(INFO, PMD, "RX/TX queues not setup yet\n");
> >
> > 	for (i = 0; i < rte_vhost_get_vring_num(vid); i++)
> > 		rte_vhost_enable_guest_notification(vid, i, 0); @@ -622,6 +625,7
> @@
> >new_device(int vid)
> >
> > 	eth_dev->data->dev_link.link_status = ETH_LINK_UP;
> >
> >+	rte_atomic32_set(&internal->dev_attached, 1);
> > 	update_queuing_status(eth_dev);
> >
> > 	RTE_LOG(INFO, PMD, "Vhost device %d created\n", vid); @@ -657,17
> >+661,19 @@ destroy_device(int vid)
> >
> > 	eth_dev->data->dev_link.link_status = ETH_LINK_DOWN;
> >
> >-	for (i = 0; i < eth_dev->data->nb_rx_queues; i++) {
> >-		vq = eth_dev->data->rx_queues[i];
> >-		if (vq == NULL)
> >-			continue;
> >-		vq->vid = -1;
> >-	}
> >-	for (i = 0; i < eth_dev->data->nb_tx_queues; i++) {
> >-		vq = eth_dev->data->tx_queues[i];
> >-		if (vq == NULL)
> >-			continue;
> >-		vq->vid = -1;
> >+	if (eth_dev->data->rx_queues && eth_dev->data->tx_queues) {
> >+		for (i = 0; i < eth_dev->data->nb_rx_queues; i++) {
> >+			vq = eth_dev->data->rx_queues[i];
> >+			if (!vq)
> >+				continue;
> >+			vq->vid = -1;
> >+		}
> >+		for (i = 0; i < eth_dev->data->nb_tx_queues; i++) {
> >+			vq = eth_dev->data->tx_queues[i];
> >+			if (!vq)
> >+				continue;
> >+			vq->vid = -1;
> >+		}
> > 	}
> >
> > 	state = vring_states[eth_dev->data->port_id];
> >@@ -792,11 +798,14 @@ eth_dev_start(struct rte_eth_dev *eth_dev)  {
> > 	struct pmd_internal *internal = eth_dev->data->dev_private;
> >
> >-	if (unlikely(rte_atomic32_read(&internal->dev_attached) == 0)) {
> >-		queue_setup(eth_dev, internal);
> >-		rte_atomic32_set(&internal->dev_attached, 1);
> >+	if (!eth_dev->data->rx_queues || !eth_dev->data->tx_queues) {
> >+		RTE_LOG(ERR, PMD, "RX/TX queues not exist yet\n");
> >+		return -1;
> > 	}
> >
> >+	if (!internal->queue_ready)
> >+		queue_setup(eth_dev, internal);
> >+
> > 	rte_atomic32_set(&internal->started, 1);
> > 	update_queuing_status(eth_dev);
> >
> >@@ -836,10 +845,13 @@ eth_dev_close(struct rte_eth_dev *dev)
> > 	pthread_mutex_unlock(&internal_list_lock);
> > 	rte_free(list);
> >
> >-	for (i = 0; i < dev->data->nb_rx_queues; i++)
> >-		rte_free(dev->data->rx_queues[i]);
> >-	for (i = 0; i < dev->data->nb_tx_queues; i++)
> >-		rte_free(dev->data->tx_queues[i]);
> >+	if (dev->data->rx_queues)
> >+		for (i = 0; i < dev->data->nb_rx_queues; i++)
> >+			rte_free(dev->data->rx_queues[i]);
> >+
> >+	if (dev->data->tx_queues)
> >+		for (i = 0; i < dev->data->nb_tx_queues; i++)
> >+			rte_free(dev->data->tx_queues[i]);
> >
> > 	rte_free(dev->data->mac_addrs);
> > 	free(internal->dev_name);
> >--
> >2.0.1
> >

Re: [dpdk-dev] [PATCH v2] net/vhost: fix vhost invalid state

[Tan, Jianfeng]

On 4/11/2018 6:53 PM, Junjie Chen wrote:
> dev_start sets *dev_attached* after setup queues, this sets device to
> invalid state since no frontend is attached. Also destroy_device set
> *started* to zero which makes *allow_queuing* always zero until dev_start
> get called again. Actually, we should not determine queues existence by
> *dev_attached* but by queues pointers or other separated variable(s).

I see two issues from your description:

- We shall fix the wrong use of dev_attached by commit 30a701a53737.
- The allow_queuing is not set correctly. But I doubt if it's a real 
issue, as we do update the queue status in dev_start(), dev_stop(), and 
new_device(). Can you explain more?

>
> Fixes: 30a701a53737 ("net/vhost: fix crash when creating vdev
> dynamically")
>
> Signed-off-by: Junjie Chen <junjie.j.chen@intel.com>
> ---
> Changes in v2:
> - use started to determine vhost queues readiness
> - revert setting started to zero in destroy_device
>   drivers/net/vhost/rte_eth_vhost.c | 61 ++++++++++++++++++++-------------------
>   1 file changed, 31 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c
> index 11b6076..ff462b3 100644
> --- a/drivers/net/vhost/rte_eth_vhost.c
> +++ b/drivers/net/vhost/rte_eth_vhost.c
> @@ -528,10 +528,13 @@ update_queuing_status(struct rte_eth_dev *dev)
>   	unsigned int i;
>   	int allow_queuing = 1;
>   
> -	if (rte_atomic32_read(&internal->dev_attached) == 0)
> +	if (!dev->data->rx_queues || !dev->data->tx_queues) {
> +		RTE_LOG(ERR, PMD, "RX/TX queues not exist yet\n");

Even it's not introduced in this patch, but I think we shall not report 
error log here.

>   		return;
> +	}
>   
> -	if (rte_atomic32_read(&internal->started) == 0)
> +	if (rte_atomic32_read(&internal->started) == 0 ||
> +	    rte_atomic32_read(&internal->dev_attached) == 0)
>   		allow_queuing = 0;
>   
>   	/* Wait until rx/tx_pkt_burst stops accessing vhost device */
> @@ -607,13 +610,10 @@ new_device(int vid)
>   #endif
>   
>   	internal->vid = vid;
> -	if (eth_dev->data->rx_queues && eth_dev->data->tx_queues) {
> +	if (rte_atomic32_read(&internal->started) == 1)
>   		queue_setup(eth_dev, internal);
> -		rte_atomic32_set(&internal->dev_attached, 1);
> -	} else {
> -		RTE_LOG(INFO, PMD, "RX/TX queues have not setup yet\n");
> -		rte_atomic32_set(&internal->dev_attached, 0);
> -	}
> +	else
> +		RTE_LOG(INFO, PMD, "RX/TX queues not exist yet\n");
>   
>   	for (i = 0; i < rte_vhost_get_vring_num(vid); i++)
>   		rte_vhost_enable_guest_notification(vid, i, 0);
> @@ -622,6 +622,7 @@ new_device(int vid)
>   
>   	eth_dev->data->dev_link.link_status = ETH_LINK_UP;
>   
> +	rte_atomic32_set(&internal->dev_attached, 1);
>   	update_queuing_status(eth_dev);
>   
>   	RTE_LOG(INFO, PMD, "Vhost device %d created\n", vid);
> @@ -651,23 +652,24 @@ destroy_device(int vid)
>   	eth_dev = list->eth_dev;
>   	internal = eth_dev->data->dev_private;
>   
> -	rte_atomic32_set(&internal->started, 0);
> -	update_queuing_status(eth_dev);
>   	rte_atomic32_set(&internal->dev_attached, 0);
> +	update_queuing_status(eth_dev);
>   
>   	eth_dev->data->dev_link.link_status = ETH_LINK_DOWN;
>   
> -	for (i = 0; i < eth_dev->data->nb_rx_queues; i++) {
> -		vq = eth_dev->data->rx_queues[i];
> -		if (vq == NULL)
> -			continue;
> -		vq->vid = -1;
> -	}
> -	for (i = 0; i < eth_dev->data->nb_tx_queues; i++) {
> -		vq = eth_dev->data->tx_queues[i];
> -		if (vq == NULL)
> -			continue;
> -		vq->vid = -1;
> +	if (eth_dev->data->rx_queues && eth_dev->data->tx_queues) {
> +		for (i = 0; i < eth_dev->data->nb_rx_queues; i++) {
> +			vq = eth_dev->data->rx_queues[i];
> +			if (!vq)
> +				continue;
> +			vq->vid = -1;
> +		}
> +		for (i = 0; i < eth_dev->data->nb_tx_queues; i++) {
> +			vq = eth_dev->data->tx_queues[i];
> +			if (!vq)
> +				continue;
> +			vq->vid = -1;
> +		}
>   	}
>   
>   	state = vring_states[eth_dev->data->port_id];
> @@ -792,11 +794,7 @@ eth_dev_start(struct rte_eth_dev *eth_dev)
>   {
>   	struct pmd_internal *internal = eth_dev->data->dev_private;
>   
> -	if (unlikely(rte_atomic32_read(&internal->dev_attached) == 0)) {
> -		queue_setup(eth_dev, internal);
> -		rte_atomic32_set(&internal->dev_attached, 1);
> -	}
> -
> +	queue_setup(eth_dev, internal);
>   	rte_atomic32_set(&internal->started, 1);
>   	update_queuing_status(eth_dev);
>   
> @@ -836,10 +834,13 @@ eth_dev_close(struct rte_eth_dev *dev)
>   	pthread_mutex_unlock(&internal_list_lock);
>   	rte_free(list);
>   
> -	for (i = 0; i < dev->data->nb_rx_queues; i++)
> -		rte_free(dev->data->rx_queues[i]);
> -	for (i = 0; i < dev->data->nb_tx_queues; i++)
> -		rte_free(dev->data->tx_queues[i]);
> +	if (dev->data->rx_queues)
> +		for (i = 0; i < dev->data->nb_rx_queues; i++)
> +			rte_free(dev->data->rx_queues[i]);
> +
> +	if (dev->data->tx_queues)
> +		for (i = 0; i < dev->data->nb_tx_queues; i++)
> +			rte_free(dev->data->tx_queues[i]);
>   
>   	rte_free(dev->data->mac_addrs);
>   	free(internal->dev_name);

Re: [dpdk-dev] [PATCH v2] net/vhost: fix vhost invalid state

[Chen, Junjie J]

> 
> 
> On 4/11/2018 6:53 PM, Junjie Chen wrote:
> > dev_start sets *dev_attached* after setup queues, this sets device to
> > invalid state since no frontend is attached. Also destroy_device set
> > *started* to zero which makes *allow_queuing* always zero until
> > dev_start get called again. Actually, we should not determine queues
> > existence by
> > *dev_attached* but by queues pointers or other separated variable(s).
> 
> I see two issues from your description:
> 
> - We shall fix the wrong use of dev_attached by commit 30a701a53737.
> - The allow_queuing is not set correctly. But I doubt if it's a real issue, as we do
> update the queue status in dev_start(), dev_stop(), and new_device(). Can you
> explain more?
The started is set to 0 in destroy_device in 30a701a53737 commit, so allow_queuing is always
set to 0 once update_queuing_status get called. We have to use "port start" to reset to 1.
> 
> >
> > Fixes: 30a701a53737 ("net/vhost: fix crash when creating vdev
> > dynamically")
> >
> > Signed-off-by: Junjie Chen <junjie.j.chen@intel.com>
> > ---
> > Changes in v2:
> > - use started to determine vhost queues readiness
> > - revert setting started to zero in destroy_device
> >   drivers/net/vhost/rte_eth_vhost.c | 61
> ++++++++++++++++++++-------------------
> >   1 file changed, 31 insertions(+), 30 deletions(-)
> >
> > diff --git a/drivers/net/vhost/rte_eth_vhost.c
> b/drivers/net/vhost/rte_eth_vhost.c
> > index 11b6076..ff462b3 100644
> > --- a/drivers/net/vhost/rte_eth_vhost.c
> > +++ b/drivers/net/vhost/rte_eth_vhost.c
> > @@ -528,10 +528,13 @@ update_queuing_status(struct rte_eth_dev *dev)
> >   	unsigned int i;
> >   	int allow_queuing = 1;
> >
> > -	if (rte_atomic32_read(&internal->dev_attached) == 0)
> > +	if (!dev->data->rx_queues || !dev->data->tx_queues) {
> > +		RTE_LOG(ERR, PMD, "RX/TX queues not exist yet\n");
> 
> Even it's not introduced in this patch, but I think we shall not report
> error log here.
if you attach one port with "port attach" (no vhost queue setup yet), and then execute 
"port stop", the queue status is not exist yet, right?
> 
> >   		return;
> > +	}
> >
> > -	if (rte_atomic32_read(&internal->started) == 0)
> > +	if (rte_atomic32_read(&internal->started) == 0 ||
> > +	    rte_atomic32_read(&internal->dev_attached) == 0)
> >   		allow_queuing = 0;
> >
> >   	/* Wait until rx/tx_pkt_burst stops accessing vhost device */
> > @@ -607,13 +610,10 @@ new_device(int vid)
> >   #endif
> >
> >   	internal->vid = vid;
> > -	if (eth_dev->data->rx_queues && eth_dev->data->tx_queues) {
> > +	if (rte_atomic32_read(&internal->started) == 1)
> >   		queue_setup(eth_dev, internal);
> > -		rte_atomic32_set(&internal->dev_attached, 1);
> > -	} else {
> > -		RTE_LOG(INFO, PMD, "RX/TX queues have not setup yet\n");
> > -		rte_atomic32_set(&internal->dev_attached, 0);
> > -	}
> > +	else
> > +		RTE_LOG(INFO, PMD, "RX/TX queues not exist yet\n");
> >
> >   	for (i = 0; i < rte_vhost_get_vring_num(vid); i++)
> >   		rte_vhost_enable_guest_notification(vid, i, 0);
> > @@ -622,6 +622,7 @@ new_device(int vid)
> >
> >   	eth_dev->data->dev_link.link_status = ETH_LINK_UP;
> >
> > +	rte_atomic32_set(&internal->dev_attached, 1);
> >   	update_queuing_status(eth_dev);
> >
> >   	RTE_LOG(INFO, PMD, "Vhost device %d created\n", vid);
> > @@ -651,23 +652,24 @@ destroy_device(int vid)
> >   	eth_dev = list->eth_dev;
> >   	internal = eth_dev->data->dev_private;
> >
> > -	rte_atomic32_set(&internal->started, 0);
> > -	update_queuing_status(eth_dev);
> >   	rte_atomic32_set(&internal->dev_attached, 0);
> > +	update_queuing_status(eth_dev);
> >
> >   	eth_dev->data->dev_link.link_status = ETH_LINK_DOWN;
> >
> > -	for (i = 0; i < eth_dev->data->nb_rx_queues; i++) {
> > -		vq = eth_dev->data->rx_queues[i];
> > -		if (vq == NULL)
> > -			continue;
> > -		vq->vid = -1;
> > -	}
> > -	for (i = 0; i < eth_dev->data->nb_tx_queues; i++) {
> > -		vq = eth_dev->data->tx_queues[i];
> > -		if (vq == NULL)
> > -			continue;
> > -		vq->vid = -1;
> > +	if (eth_dev->data->rx_queues && eth_dev->data->tx_queues) {
> > +		for (i = 0; i < eth_dev->data->nb_rx_queues; i++) {
> > +			vq = eth_dev->data->rx_queues[i];
> > +			if (!vq)
> > +				continue;
> > +			vq->vid = -1;
> > +		}
> > +		for (i = 0; i < eth_dev->data->nb_tx_queues; i++) {
> > +			vq = eth_dev->data->tx_queues[i];
> > +			if (!vq)
> > +				continue;
> > +			vq->vid = -1;
> > +		}
> >   	}
> >
> >   	state = vring_states[eth_dev->data->port_id];
> > @@ -792,11 +794,7 @@ eth_dev_start(struct rte_eth_dev *eth_dev)
> >   {
> >   	struct pmd_internal *internal = eth_dev->data->dev_private;
> >
> > -	if (unlikely(rte_atomic32_read(&internal->dev_attached) == 0)) {
> > -		queue_setup(eth_dev, internal);
> > -		rte_atomic32_set(&internal->dev_attached, 1);
> > -	}
> > -
> > +	queue_setup(eth_dev, internal);
> >   	rte_atomic32_set(&internal->started, 1);
> >   	update_queuing_status(eth_dev);
> >
> > @@ -836,10 +834,13 @@ eth_dev_close(struct rte_eth_dev *dev)
> >   	pthread_mutex_unlock(&internal_list_lock);
> >   	rte_free(list);
> >
> > -	for (i = 0; i < dev->data->nb_rx_queues; i++)
> > -		rte_free(dev->data->rx_queues[i]);
> > -	for (i = 0; i < dev->data->nb_tx_queues; i++)
> > -		rte_free(dev->data->tx_queues[i]);
> > +	if (dev->data->rx_queues)
> > +		for (i = 0; i < dev->data->nb_rx_queues; i++)
> > +			rte_free(dev->data->rx_queues[i]);
> > +
> > +	if (dev->data->tx_queues)
> > +		for (i = 0; i < dev->data->nb_tx_queues; i++)
> > +			rte_free(dev->data->tx_queues[i]);
> >
> >   	rte_free(dev->data->mac_addrs);
> >   	free(internal->dev_name);

Re: [dpdk-dev] [PATCH v2] net/vhost: fix vhost invalid state

[Tan, Jianfeng]

On 4/11/2018 4:35 PM, Chen, Junjie J wrote:
>>
>> On 4/11/2018 6:53 PM, Junjie Chen wrote:
>>> dev_start sets *dev_attached* after setup queues, this sets device to
>>> invalid state since no frontend is attached. Also destroy_device set
>>> *started* to zero which makes *allow_queuing* always zero until
>>> dev_start get called again. Actually, we should not determine queues
>>> existence by
>>> *dev_attached* but by queues pointers or other separated variable(s).
>> I see two issues from your description:
>>
>> - We shall fix the wrong use of dev_attached by commit 30a701a53737.
>> - The allow_queuing is not set correctly. But I doubt if it's a real issue, as we do
>> update the queue status in dev_start(), dev_stop(), and new_device(). Can you
>> explain more?
> The started is set to 0 in destroy_device in 30a701a53737 commit, so allow_queuing is always
> set to 0 once update_queuing_status get called. We have to use "port start" to reset to 1.

OK, that means it is also due to the wrong use of dev_attached?

>>> Fixes: 30a701a53737 ("net/vhost: fix crash when creating vdev
>>> dynamically")
>>>
>>> Signed-off-by: Junjie Chen <junjie.j.chen@intel.com>
>>> ---
>>> Changes in v2:
>>> - use started to determine vhost queues readiness
>>> - revert setting started to zero in destroy_device
>>>    drivers/net/vhost/rte_eth_vhost.c | 61
>> ++++++++++++++++++++-------------------
>>>    1 file changed, 31 insertions(+), 30 deletions(-)
>>>
>>> diff --git a/drivers/net/vhost/rte_eth_vhost.c
>> b/drivers/net/vhost/rte_eth_vhost.c
>>> index 11b6076..ff462b3 100644
>>> --- a/drivers/net/vhost/rte_eth_vhost.c
>>> +++ b/drivers/net/vhost/rte_eth_vhost.c
>>> @@ -528,10 +528,13 @@ update_queuing_status(struct rte_eth_dev *dev)
>>>    	unsigned int i;
>>>    	int allow_queuing = 1;
>>>
>>> -	if (rte_atomic32_read(&internal->dev_attached) == 0)
>>> +	if (!dev->data->rx_queues || !dev->data->tx_queues) {
>>> +		RTE_LOG(ERR, PMD, "RX/TX queues not exist yet\n");
>> Even it's not introduced in this patch, but I think we shall not report
>> error log here.
> if you attach one port with "port attach" (no vhost queue setup yet), and then execute
> "port stop", the queue status is not exist yet, right?

My point is that we allow a dev is not started but attached, so it's not 
an error, and we shall not print error log.

>>>    		return;
>>> +	}
>>>
>>> -	if (rte_atomic32_read(&internal->started) == 0)
>>> +	if (rte_atomic32_read(&internal->started) == 0 ||
>>> +	    rte_atomic32_read(&internal->dev_attached) == 0)
>>>    		allow_queuing = 0;
>>>
>>>    	/* Wait until rx/tx_pkt_burst stops accessing vhost device */
>>> @@ -607,13 +610,10 @@ new_device(int vid)
>>>    #endif
>>>
>>>    	internal->vid = vid;
>>> -	if (eth_dev->data->rx_queues && eth_dev->data->tx_queues) {
>>> +	if (rte_atomic32_read(&internal->started) == 1)
>>>    		queue_setup(eth_dev, internal);
>>> -		rte_atomic32_set(&internal->dev_attached, 1);
>>> -	} else {
>>> -		RTE_LOG(INFO, PMD, "RX/TX queues have not setup yet\n");
>>> -		rte_atomic32_set(&internal->dev_attached, 0);
>>> -	}
>>> +	else
>>> +		RTE_LOG(INFO, PMD, "RX/TX queues not exist yet\n");
>>>
>>>    	for (i = 0; i < rte_vhost_get_vring_num(vid); i++)
>>>    		rte_vhost_enable_guest_notification(vid, i, 0);
>>> @@ -622,6 +622,7 @@ new_device(int vid)
>>>
>>>    	eth_dev->data->dev_link.link_status = ETH_LINK_UP;
>>>
>>> +	rte_atomic32_set(&internal->dev_attached, 1);
>>>    	update_queuing_status(eth_dev);
>>>
>>>    	RTE_LOG(INFO, PMD, "Vhost device %d created\n", vid);
>>> @@ -651,23 +652,24 @@ destroy_device(int vid)
>>>    	eth_dev = list->eth_dev;
>>>    	internal = eth_dev->data->dev_private;
>>>
>>> -	rte_atomic32_set(&internal->started, 0);
>>> -	update_queuing_status(eth_dev);
>>>    	rte_atomic32_set(&internal->dev_attached, 0);
>>> +	update_queuing_status(eth_dev);
>>>
>>>    	eth_dev->data->dev_link.link_status = ETH_LINK_DOWN;
>>>
>>> -	for (i = 0; i < eth_dev->data->nb_rx_queues; i++) {
>>> -		vq = eth_dev->data->rx_queues[i];
>>> -		if (vq == NULL)
>>> -			continue;
>>> -		vq->vid = -1;
>>> -	}
>>> -	for (i = 0; i < eth_dev->data->nb_tx_queues; i++) {
>>> -		vq = eth_dev->data->tx_queues[i];
>>> -		if (vq == NULL)
>>> -			continue;
>>> -		vq->vid = -1;
>>> +	if (eth_dev->data->rx_queues && eth_dev->data->tx_queues) {
>>> +		for (i = 0; i < eth_dev->data->nb_rx_queues; i++) {
>>> +			vq = eth_dev->data->rx_queues[i];
>>> +			if (!vq)
>>> +				continue;
>>> +			vq->vid = -1;
>>> +		}
>>> +		for (i = 0; i < eth_dev->data->nb_tx_queues; i++) {
>>> +			vq = eth_dev->data->tx_queues[i];
>>> +			if (!vq)
>>> +				continue;
>>> +			vq->vid = -1;
>>> +		}
>>>    	}
>>>
>>>    	state = vring_states[eth_dev->data->port_id];
>>> @@ -792,11 +794,7 @@ eth_dev_start(struct rte_eth_dev *eth_dev)
>>>    {
>>>    	struct pmd_internal *internal = eth_dev->data->dev_private;
>>>
>>> -	if (unlikely(rte_atomic32_read(&internal->dev_attached) == 0)) {
>>> -		queue_setup(eth_dev, internal);
>>> -		rte_atomic32_set(&internal->dev_attached, 1);
>>> -	}
>>> -
>>> +	queue_setup(eth_dev, internal);
>>>    	rte_atomic32_set(&internal->started, 1);
>>>    	update_queuing_status(eth_dev);
>>>
>>> @@ -836,10 +834,13 @@ eth_dev_close(struct rte_eth_dev *dev)
>>>    	pthread_mutex_unlock(&internal_list_lock);
>>>    	rte_free(list);
>>>
>>> -	for (i = 0; i < dev->data->nb_rx_queues; i++)
>>> -		rte_free(dev->data->rx_queues[i]);
>>> -	for (i = 0; i < dev->data->nb_tx_queues; i++)
>>> -		rte_free(dev->data->tx_queues[i]);
>>> +	if (dev->data->rx_queues)
>>> +		for (i = 0; i < dev->data->nb_rx_queues; i++)
>>> +			rte_free(dev->data->rx_queues[i]);
>>> +
>>> +	if (dev->data->tx_queues)
>>> +		for (i = 0; i < dev->data->nb_tx_queues; i++)
>>> +			rte_free(dev->data->tx_queues[i]);
>>>
>>>    	rte_free(dev->data->mac_addrs);
>>>    	free(internal->dev_name);

Re: [dpdk-dev] [PATCH v2] net/vhost: fix vhost invalid state

[Jens Freimann]

On Wed, Apr 11, 2018 at 06:53:13AM -0400, Junjie Chen wrote:
>dev_start sets *dev_attached* after setup queues, this sets device to
>invalid state since no frontend is attached. Also destroy_device set
>*started* to zero which makes *allow_queuing* always zero until dev_start
>get called again. Actually, we should not determine queues existence by
>*dev_attached* but by queues pointers or other separated variable(s).
>
>Fixes: 30a701a53737 ("net/vhost: fix crash when creating vdev
>dynamically")
>
>Signed-off-by: Junjie Chen <junjie.j.chen@intel.com>
>---
>Changes in v2:
>- use started to determine vhost queues readiness
>- revert setting started to zero in destroy_device

This one works for me. Thanks!

Tested-by: Jens Freimann <jfreimann@redhat.com>

regards,
Jens 

Re: [dpdk-dev] [PATCH v2] net/vhost: fix vhost invalid state

[Chen, Junjie J]

Hi Jianfeng.

> On 4/11/2018 4:35 PM, Chen, Junjie J wrote:
> >>
> >> On 4/11/2018 6:53 PM, Junjie Chen wrote:
> >>> dev_start sets *dev_attached* after setup queues, this sets device
> >>> to invalid state since no frontend is attached. Also destroy_device
> >>> set
> >>> *started* to zero which makes *allow_queuing* always zero until
> >>> dev_start get called again. Actually, we should not determine queues
> >>> existence by
> >>> *dev_attached* but by queues pointers or other separated variable(s).
> >> I see two issues from your description:
> >>
> >> - We shall fix the wrong use of dev_attached by commit 30a701a53737.
> >> - The allow_queuing is not set correctly. But I doubt if it's a real
> >> issue, as we do update the queue status in dev_start(), dev_stop(),
> >> and new_device(). Can you explain more?
> > The started is set to 0 in destroy_device in 30a701a53737 commit, so
> > allow_queuing is always set to 0 once update_queuing_status get called. We
> have to use "port start" to reset to 1.
> 
> OK, that means it is also due to the wrong use of dev_attached?
Yes, a mixed wrong. That is why I tried to use new variable to track queue. One variable for one state.
> 
> >>> Fixes: 30a701a53737 ("net/vhost: fix crash when creating vdev
> >>> dynamically")
> >>>
> >>> Signed-off-by: Junjie Chen <junjie.j.chen@intel.com>
> >>> ---
> >>> Changes in v2:
> >>> - use started to determine vhost queues readiness
> >>> - revert setting started to zero in destroy_device
> >>>    drivers/net/vhost/rte_eth_vhost.c | 61
> >> ++++++++++++++++++++-------------------
> >>>    1 file changed, 31 insertions(+), 30 deletions(-)
> >>>
> >>> diff --git a/drivers/net/vhost/rte_eth_vhost.c
> >> b/drivers/net/vhost/rte_eth_vhost.c
> >>> index 11b6076..ff462b3 100644
> >>> --- a/drivers/net/vhost/rte_eth_vhost.c
> >>> +++ b/drivers/net/vhost/rte_eth_vhost.c
> >>> @@ -528,10 +528,13 @@ update_queuing_status(struct rte_eth_dev
> *dev)
> >>>    	unsigned int i;
> >>>    	int allow_queuing = 1;
> >>>
> >>> -	if (rte_atomic32_read(&internal->dev_attached) == 0)
> >>> +	if (!dev->data->rx_queues || !dev->data->tx_queues) {
> >>> +		RTE_LOG(ERR, PMD, "RX/TX queues not exist yet\n");
> >> Even it's not introduced in this patch, but I think we shall not
> >> report error log here.
> > if you attach one port with "port attach" (no vhost queue setup yet),
> > and then execute "port stop", the queue status is not exist yet, right?
> 
> My point is that we allow a dev is not started but attached, so it's not an error,
> and we shall not print error log.
OK, will remove it in V3. 

> 
> >>>    		return;
> >>> +	}
> >>>
> >>> -	if (rte_atomic32_read(&internal->started) == 0)
> >>> +	if (rte_atomic32_read(&internal->started) == 0 ||
> >>> +	    rte_atomic32_read(&internal->dev_attached) == 0)
> >>>    		allow_queuing = 0;
> >>>
> >>>    	/* Wait until rx/tx_pkt_burst stops accessing vhost device */ @@
> >>> -607,13 +610,10 @@ new_device(int vid)
> >>>    #endif
> >>>
> >>>    	internal->vid = vid;
> >>> -	if (eth_dev->data->rx_queues && eth_dev->data->tx_queues) {
> >>> +	if (rte_atomic32_read(&internal->started) == 1)
> >>>    		queue_setup(eth_dev, internal);
> >>> -		rte_atomic32_set(&internal->dev_attached, 1);
> >>> -	} else {
> >>> -		RTE_LOG(INFO, PMD, "RX/TX queues have not setup yet\n");
> >>> -		rte_atomic32_set(&internal->dev_attached, 0);
> >>> -	}
> >>> +	else
> >>> +		RTE_LOG(INFO, PMD, "RX/TX queues not exist yet\n");
> >>>
> >>>    	for (i = 0; i < rte_vhost_get_vring_num(vid); i++)
> >>>    		rte_vhost_enable_guest_notification(vid, i, 0); @@ -622,6
> >>> +622,7 @@ new_device(int vid)
> >>>
> >>>    	eth_dev->data->dev_link.link_status = ETH_LINK_UP;
> >>>
> >>> +	rte_atomic32_set(&internal->dev_attached, 1);
> >>>    	update_queuing_status(eth_dev);
> >>>
> >>>    	RTE_LOG(INFO, PMD, "Vhost device %d created\n", vid); @@
> -651,23
> >>> +652,24 @@ destroy_device(int vid)
> >>>    	eth_dev = list->eth_dev;
> >>>    	internal = eth_dev->data->dev_private;
> >>>
> >>> -	rte_atomic32_set(&internal->started, 0);
> >>> -	update_queuing_status(eth_dev);
> >>>    	rte_atomic32_set(&internal->dev_attached, 0);
> >>> +	update_queuing_status(eth_dev);
> >>>
> >>>    	eth_dev->data->dev_link.link_status = ETH_LINK_DOWN;
> >>>
> >>> -	for (i = 0; i < eth_dev->data->nb_rx_queues; i++) {
> >>> -		vq = eth_dev->data->rx_queues[i];
> >>> -		if (vq == NULL)
> >>> -			continue;
> >>> -		vq->vid = -1;
> >>> -	}
> >>> -	for (i = 0; i < eth_dev->data->nb_tx_queues; i++) {
> >>> -		vq = eth_dev->data->tx_queues[i];
> >>> -		if (vq == NULL)
> >>> -			continue;
> >>> -		vq->vid = -1;
> >>> +	if (eth_dev->data->rx_queues && eth_dev->data->tx_queues) {
> >>> +		for (i = 0; i < eth_dev->data->nb_rx_queues; i++) {
> >>> +			vq = eth_dev->data->rx_queues[i];
> >>> +			if (!vq)
> >>> +				continue;
> >>> +			vq->vid = -1;
> >>> +		}
> >>> +		for (i = 0; i < eth_dev->data->nb_tx_queues; i++) {
> >>> +			vq = eth_dev->data->tx_queues[i];
> >>> +			if (!vq)
> >>> +				continue;
> >>> +			vq->vid = -1;
> >>> +		}
> >>>    	}
> >>>
> >>>    	state = vring_states[eth_dev->data->port_id];
> >>> @@ -792,11 +794,7 @@ eth_dev_start(struct rte_eth_dev *eth_dev)
> >>>    {
> >>>    	struct pmd_internal *internal = eth_dev->data->dev_private;
> >>>
> >>> -	if (unlikely(rte_atomic32_read(&internal->dev_attached) == 0)) {
> >>> -		queue_setup(eth_dev, internal);
> >>> -		rte_atomic32_set(&internal->dev_attached, 1);
> >>> -	}
> >>> -
> >>> +	queue_setup(eth_dev, internal);
> >>>    	rte_atomic32_set(&internal->started, 1);
> >>>    	update_queuing_status(eth_dev);
> >>>
> >>> @@ -836,10 +834,13 @@ eth_dev_close(struct rte_eth_dev *dev)
> >>>    	pthread_mutex_unlock(&internal_list_lock);
> >>>    	rte_free(list);
> >>>
> >>> -	for (i = 0; i < dev->data->nb_rx_queues; i++)
> >>> -		rte_free(dev->data->rx_queues[i]);
> >>> -	for (i = 0; i < dev->data->nb_tx_queues; i++)
> >>> -		rte_free(dev->data->tx_queues[i]);
> >>> +	if (dev->data->rx_queues)
> >>> +		for (i = 0; i < dev->data->nb_rx_queues; i++)
> >>> +			rte_free(dev->data->rx_queues[i]);
> >>> +
> >>> +	if (dev->data->tx_queues)
> >>> +		for (i = 0; i < dev->data->nb_tx_queues; i++)
> >>> +			rte_free(dev->data->tx_queues[i]);
> >>>
> >>>    	rte_free(dev->data->mac_addrs);
> >>>    	free(internal->dev_name);

[dpdk-dev] [PATCH v2] net/vhost: fix vhost invalid state

[Junjie Chen]

dev_start sets *dev_attached* after setup queues, this sets device to
invalid state since no frontend is attached. Also destroy_device set
*started* to zero which makes *allow_queuing* always zero until dev_start
get called again. Actually, we should not determine queues existence by
*dev_attached* but by queues pointers or other separated variable(s).

Fixes: 30a701a53737 ("net/vhost: fix crash when creating vdev
dynamically")

Signed-off-by: Junjie Chen <junjie.j.chen@intel.com>
---
Changes in v2:
- use started to determine vhost queues readiness
- revert setting started to zero in destroy_device
 drivers/net/vhost/rte_eth_vhost.c | 61 ++++++++++++++++++++-------------------
 1 file changed, 31 insertions(+), 30 deletions(-)

diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c
index 11b6076..ff462b3 100644
--- a/drivers/net/vhost/rte_eth_vhost.c
+++ b/drivers/net/vhost/rte_eth_vhost.c
@@ -528,10 +528,13 @@ update_queuing_status(struct rte_eth_dev *dev)
 	unsigned int i;
 	int allow_queuing = 1;
 
-	if (rte_atomic32_read(&internal->dev_attached) == 0)
+	if (!dev->data->rx_queues || !dev->data->tx_queues) {
+		RTE_LOG(ERR, PMD, "RX/TX queues not exist yet\n");
 		return;
+	}
 
-	if (rte_atomic32_read(&internal->started) == 0)
+	if (rte_atomic32_read(&internal->started) == 0 ||
+	    rte_atomic32_read(&internal->dev_attached) == 0)
 		allow_queuing = 0;
 
 	/* Wait until rx/tx_pkt_burst stops accessing vhost device */
@@ -607,13 +610,10 @@ new_device(int vid)
 #endif
 
 	internal->vid = vid;
-	if (eth_dev->data->rx_queues && eth_dev->data->tx_queues) {
+	if (rte_atomic32_read(&internal->started) == 1)
 		queue_setup(eth_dev, internal);
-		rte_atomic32_set(&internal->dev_attached, 1);
-	} else {
-		RTE_LOG(INFO, PMD, "RX/TX queues have not setup yet\n");
-		rte_atomic32_set(&internal->dev_attached, 0);
-	}
+	else
+		RTE_LOG(INFO, PMD, "RX/TX queues not exist yet\n");
 
 	for (i = 0; i < rte_vhost_get_vring_num(vid); i++)
 		rte_vhost_enable_guest_notification(vid, i, 0);
@@ -622,6 +622,7 @@ new_device(int vid)
 
 	eth_dev->data->dev_link.link_status = ETH_LINK_UP;
 
+	rte_atomic32_set(&internal->dev_attached, 1);
 	update_queuing_status(eth_dev);
 
 	RTE_LOG(INFO, PMD, "Vhost device %d created\n", vid);
@@ -651,23 +652,24 @@ destroy_device(int vid)
 	eth_dev = list->eth_dev;
 	internal = eth_dev->data->dev_private;
 
-	rte_atomic32_set(&internal->started, 0);
-	update_queuing_status(eth_dev);
 	rte_atomic32_set(&internal->dev_attached, 0);
+	update_queuing_status(eth_dev);
 
 	eth_dev->data->dev_link.link_status = ETH_LINK_DOWN;
 
-	for (i = 0; i < eth_dev->data->nb_rx_queues; i++) {
-		vq = eth_dev->data->rx_queues[i];
-		if (vq == NULL)
-			continue;
-		vq->vid = -1;
-	}
-	for (i = 0; i < eth_dev->data->nb_tx_queues; i++) {
-		vq = eth_dev->data->tx_queues[i];
-		if (vq == NULL)
-			continue;
-		vq->vid = -1;
+	if (eth_dev->data->rx_queues && eth_dev->data->tx_queues) {
+		for (i = 0; i < eth_dev->data->nb_rx_queues; i++) {
+			vq = eth_dev->data->rx_queues[i];
+			if (!vq)
+				continue;
+			vq->vid = -1;
+		}
+		for (i = 0; i < eth_dev->data->nb_tx_queues; i++) {
+			vq = eth_dev->data->tx_queues[i];
+			if (!vq)
+				continue;
+			vq->vid = -1;
+		}
 	}
 
 	state = vring_states[eth_dev->data->port_id];
@@ -792,11 +794,7 @@ eth_dev_start(struct rte_eth_dev *eth_dev)
 {
 	struct pmd_internal *internal = eth_dev->data->dev_private;
 
-	if (unlikely(rte_atomic32_read(&internal->dev_attached) == 0)) {
-		queue_setup(eth_dev, internal);
-		rte_atomic32_set(&internal->dev_attached, 1);
-	}
-
+	queue_setup(eth_dev, internal);
 	rte_atomic32_set(&internal->started, 1);
 	update_queuing_status(eth_dev);
 
@@ -836,10 +834,13 @@ eth_dev_close(struct rte_eth_dev *dev)
 	pthread_mutex_unlock(&internal_list_lock);
 	rte_free(list);
 
-	for (i = 0; i < dev->data->nb_rx_queues; i++)
-		rte_free(dev->data->rx_queues[i]);
-	for (i = 0; i < dev->data->nb_tx_queues; i++)
-		rte_free(dev->data->tx_queues[i]);
+	if (dev->data->rx_queues)
+		for (i = 0; i < dev->data->nb_rx_queues; i++)
+			rte_free(dev->data->rx_queues[i]);
+
+	if (dev->data->tx_queues)
+		for (i = 0; i < dev->data->nb_tx_queues; i++)
+			rte_free(dev->data->tx_queues[i]);
 
 	rte_free(dev->data->mac_addrs);
 	free(internal->dev_name);
-- 
2.0.1

[dpdk-dev] [PATCH v3] net/vhost: fix vhost invalid state

[Junjie Chen]

dev_start sets *dev_attached* after setup queues, this sets device to
invalid state since no frontend is attached. Also destroy_device set
*started* to zero which makes *allow_queuing* always zero until dev_start
get called again. Actually, we should not determine queues existence by
*dev_attached* but by queues pointers or other separated variable(s).

Fixes: 30a701a53737 ("net/vhost: fix crash when creating vdev
dynamically")

Signed-off-by: Junjie Chen <junjie.j.chen@intel.com>
Tested-by: Jens Freimann <jfreimann@redhat.com>
---
Changes in v3:
- remove useless log in queue status showing
Changes in v2:
- use started to determine vhost queues readiness
- revert setting started to zero in destroy_device
 drivers/net/vhost/rte_eth_vhost.c | 59 +++++++++++++++++++--------------------
 1 file changed, 29 insertions(+), 30 deletions(-)

diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c
index 11b6076..e392d71 100644
--- a/drivers/net/vhost/rte_eth_vhost.c
+++ b/drivers/net/vhost/rte_eth_vhost.c
@@ -528,10 +528,11 @@ update_queuing_status(struct rte_eth_dev *dev)
 	unsigned int i;
 	int allow_queuing = 1;
 
-	if (rte_atomic32_read(&internal->dev_attached) == 0)
+	if (!dev->data->rx_queues || !dev->data->tx_queues)
 		return;
 
-	if (rte_atomic32_read(&internal->started) == 0)
+	if (rte_atomic32_read(&internal->started) == 0 ||
+	    rte_atomic32_read(&internal->dev_attached) == 0)
 		allow_queuing = 0;
 
 	/* Wait until rx/tx_pkt_burst stops accessing vhost device */
@@ -607,13 +608,10 @@ new_device(int vid)
 #endif
 
 	internal->vid = vid;
-	if (eth_dev->data->rx_queues && eth_dev->data->tx_queues) {
+	if (rte_atomic32_read(&internal->started) == 1)
 		queue_setup(eth_dev, internal);
-		rte_atomic32_set(&internal->dev_attached, 1);
-	} else {
-		RTE_LOG(INFO, PMD, "RX/TX queues have not setup yet\n");
-		rte_atomic32_set(&internal->dev_attached, 0);
-	}
+	else
+		RTE_LOG(INFO, PMD, "RX/TX queues not exist yet\n");
 
 	for (i = 0; i < rte_vhost_get_vring_num(vid); i++)
 		rte_vhost_enable_guest_notification(vid, i, 0);
@@ -622,6 +620,7 @@ new_device(int vid)
 
 	eth_dev->data->dev_link.link_status = ETH_LINK_UP;
 
+	rte_atomic32_set(&internal->dev_attached, 1);
 	update_queuing_status(eth_dev);
 
 	RTE_LOG(INFO, PMD, "Vhost device %d created\n", vid);
@@ -651,23 +650,24 @@ destroy_device(int vid)
 	eth_dev = list->eth_dev;
 	internal = eth_dev->data->dev_private;
 
-	rte_atomic32_set(&internal->started, 0);
-	update_queuing_status(eth_dev);
 	rte_atomic32_set(&internal->dev_attached, 0);
+	update_queuing_status(eth_dev);
 
 	eth_dev->data->dev_link.link_status = ETH_LINK_DOWN;
 
-	for (i = 0; i < eth_dev->data->nb_rx_queues; i++) {
-		vq = eth_dev->data->rx_queues[i];
-		if (vq == NULL)
-			continue;
-		vq->vid = -1;
-	}
-	for (i = 0; i < eth_dev->data->nb_tx_queues; i++) {
-		vq = eth_dev->data->tx_queues[i];
-		if (vq == NULL)
-			continue;
-		vq->vid = -1;
+	if (eth_dev->data->rx_queues && eth_dev->data->tx_queues) {
+		for (i = 0; i < eth_dev->data->nb_rx_queues; i++) {
+			vq = eth_dev->data->rx_queues[i];
+			if (!vq)
+				continue;
+			vq->vid = -1;
+		}
+		for (i = 0; i < eth_dev->data->nb_tx_queues; i++) {
+			vq = eth_dev->data->tx_queues[i];
+			if (!vq)
+				continue;
+			vq->vid = -1;
+		}
 	}
 
 	state = vring_states[eth_dev->data->port_id];
@@ -792,11 +792,7 @@ eth_dev_start(struct rte_eth_dev *eth_dev)
 {
 	struct pmd_internal *internal = eth_dev->data->dev_private;
 
-	if (unlikely(rte_atomic32_read(&internal->dev_attached) == 0)) {
-		queue_setup(eth_dev, internal);
-		rte_atomic32_set(&internal->dev_attached, 1);
-	}
-
+	queue_setup(eth_dev, internal);
 	rte_atomic32_set(&internal->started, 1);
 	update_queuing_status(eth_dev);
 
@@ -836,10 +832,13 @@ eth_dev_close(struct rte_eth_dev *dev)
 	pthread_mutex_unlock(&internal_list_lock);
 	rte_free(list);
 
-	for (i = 0; i < dev->data->nb_rx_queues; i++)
-		rte_free(dev->data->rx_queues[i]);
-	for (i = 0; i < dev->data->nb_tx_queues; i++)
-		rte_free(dev->data->tx_queues[i]);
+	if (dev->data->rx_queues)
+		for (i = 0; i < dev->data->nb_rx_queues; i++)
+			rte_free(dev->data->rx_queues[i]);
+
+	if (dev->data->tx_queues)
+		for (i = 0; i < dev->data->nb_tx_queues; i++)
+			rte_free(dev->data->tx_queues[i]);
 
 	rte_free(dev->data->mac_addrs);
 	free(internal->dev_name);
-- 
2.0.1

Re: [dpdk-dev] [PATCH v3] net/vhost: fix vhost invalid state

[Tan, Jianfeng]

On 4/12/2018 1:02 AM, Junjie Chen wrote:
> dev_start sets *dev_attached* after setup queues, this sets device to
> invalid state since no frontend is attached. Also destroy_device set
> *started* to zero which makes *allow_queuing* always zero until dev_start
> get called again. Actually, we should not determine queues existence by
> *dev_attached* but by queues pointers or other separated variable(s).
>
> Fixes: 30a701a53737 ("net/vhost: fix crash when creating vdev
> dynamically")
>
> Signed-off-by: Junjie Chen <junjie.j.chen@intel.com>
> Tested-by: Jens Freimann <jfreimann@redhat.com>

Overall, looks great to me except a nit below.

Reviewed-by: Jianfeng Tan <jianfeng.tan@intel.com>

> ---
> Changes in v3:
> - remove useless log in queue status showing
> Changes in v2:
> - use started to determine vhost queues readiness
> - revert setting started to zero in destroy_device
>   drivers/net/vhost/rte_eth_vhost.c | 59 +++++++++++++++++++--------------------
>   1 file changed, 29 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c
> index 11b6076..e392d71 100644
> --- a/drivers/net/vhost/rte_eth_vhost.c
> +++ b/drivers/net/vhost/rte_eth_vhost.c
> @@ -528,10 +528,11 @@ update_queuing_status(struct rte_eth_dev *dev)
>   	unsigned int i;
>   	int allow_queuing = 1;
>   
> -	if (rte_atomic32_read(&internal->dev_attached) == 0)
> +	if (!dev->data->rx_queues || !dev->data->tx_queues)
>   		return;
>   
> -	if (rte_atomic32_read(&internal->started) == 0)
> +	if (rte_atomic32_read(&internal->started) == 0 ||
> +	    rte_atomic32_read(&internal->dev_attached) == 0)
>   		allow_queuing = 0;
>   
>   	/* Wait until rx/tx_pkt_burst stops accessing vhost device */
> @@ -607,13 +608,10 @@ new_device(int vid)
>   #endif
>   
>   	internal->vid = vid;
> -	if (eth_dev->data->rx_queues && eth_dev->data->tx_queues) {
> +	if (rte_atomic32_read(&internal->started) == 1)
>   		queue_setup(eth_dev, internal);
> -		rte_atomic32_set(&internal->dev_attached, 1);
> -	} else {
> -		RTE_LOG(INFO, PMD, "RX/TX queues have not setup yet\n");
> -		rte_atomic32_set(&internal->dev_attached, 0);
> -	}
> +	else
> +		RTE_LOG(INFO, PMD, "RX/TX queues not exist yet\n");
>   
>   	for (i = 0; i < rte_vhost_get_vring_num(vid); i++)
>   		rte_vhost_enable_guest_notification(vid, i, 0);
> @@ -622,6 +620,7 @@ new_device(int vid)
>   
>   	eth_dev->data->dev_link.link_status = ETH_LINK_UP;
>   
> +	rte_atomic32_set(&internal->dev_attached, 1);
>   	update_queuing_status(eth_dev);
>   
>   	RTE_LOG(INFO, PMD, "Vhost device %d created\n", vid);
> @@ -651,23 +650,24 @@ destroy_device(int vid)
>   	eth_dev = list->eth_dev;
>   	internal = eth_dev->data->dev_private;
>   
> -	rte_atomic32_set(&internal->started, 0);
> -	update_queuing_status(eth_dev);
>   	rte_atomic32_set(&internal->dev_attached, 0);
> +	update_queuing_status(eth_dev);
>   
>   	eth_dev->data->dev_link.link_status = ETH_LINK_DOWN;
>   
> -	for (i = 0; i < eth_dev->data->nb_rx_queues; i++) {
> -		vq = eth_dev->data->rx_queues[i];
> -		if (vq == NULL)
> -			continue;
> -		vq->vid = -1;
> -	}
> -	for (i = 0; i < eth_dev->data->nb_tx_queues; i++) {
> -		vq = eth_dev->data->tx_queues[i];
> -		if (vq == NULL)
> -			continue;
> -		vq->vid = -1;
> +	if (eth_dev->data->rx_queues && eth_dev->data->tx_queues) {
> +		for (i = 0; i < eth_dev->data->nb_rx_queues; i++) {
> +			vq = eth_dev->data->rx_queues[i];
> +			if (!vq)
> +				continue;
> +			vq->vid = -1;
> +		}
> +		for (i = 0; i < eth_dev->data->nb_tx_queues; i++) {
> +			vq = eth_dev->data->tx_queues[i];
> +			if (!vq)
> +				continue;
> +			vq->vid = -1;
> +		}
>   	}
>   
>   	state = vring_states[eth_dev->data->port_id];
> @@ -792,11 +792,7 @@ eth_dev_start(struct rte_eth_dev *eth_dev)
>   {
>   	struct pmd_internal *internal = eth_dev->data->dev_private;
>   
> -	if (unlikely(rte_atomic32_read(&internal->dev_attached) == 0)) {
> -		queue_setup(eth_dev, internal);
> -		rte_atomic32_set(&internal->dev_attached, 1);
> -	}
> -
> +	queue_setup(eth_dev, internal);
>   	rte_atomic32_set(&internal->started, 1);
>   	update_queuing_status(eth_dev);
>   
> @@ -836,10 +832,13 @@ eth_dev_close(struct rte_eth_dev *dev)
>   	pthread_mutex_unlock(&internal_list_lock);
>   	rte_free(list);
>   
> -	for (i = 0; i < dev->data->nb_rx_queues; i++)
> -		rte_free(dev->data->rx_queues[i]);
> -	for (i = 0; i < dev->data->nb_tx_queues; i++)
> -		rte_free(dev->data->tx_queues[i]);
> +	if (dev->data->rx_queues)

This is implied that rx_queues is already allocated. So I don't think we 
need this.

> +		for (i = 0; i < dev->data->nb_rx_queues; i++)
> +			rte_free(dev->data->rx_queues[i]);
> +
> +	if (dev->data->tx_queues)
> +		for (i = 0; i < dev->data->nb_tx_queues; i++)
> +			rte_free(dev->data->tx_queues[i]);
>   
>   	rte_free(dev->data->mac_addrs);
>   	free(internal->dev_name);

Re: [dpdk-dev] [PATCH v3] net/vhost: fix vhost invalid state

[Maxime Coquelin]

On 04/12/2018 09:21 AM, Tan, Jianfeng wrote:
> 
> 
> On 4/12/2018 1:02 AM, Junjie Chen wrote:
>> dev_start sets *dev_attached* after setup queues, this sets device to
>> invalid state since no frontend is attached. Also destroy_device set
>> *started* to zero which makes *allow_queuing* always zero until dev_start
>> get called again. Actually, we should not determine queues existence by
>> *dev_attached* but by queues pointers or other separated variable(s).
>>
>> Fixes: 30a701a53737 ("net/vhost: fix crash when creating vdev
>> dynamically")
>>
>> Signed-off-by: Junjie Chen <junjie.j.chen@intel.com>
>> Tested-by: Jens Freimann <jfreimann@redhat.com>
> 
> Overall, looks great to me except a nit below.
> 
> Reviewed-by: Jianfeng Tan <jianfeng.tan@intel.com>

Thanks Jianfeng, I can handle the small change while applying.

Can you confirm that it is implied that the queue are already allocated,
else we wouldn't find the internal resource and quit earlier (in case of
eth_dev_close called twice for example)?

Thanks,
Maxime

> 
>> ---
>> Changes in v3:
>> - remove useless log in queue status showing
>> Changes in v2:
>> - use started to determine vhost queues readiness
>> - revert setting started to zero in destroy_device
>>   drivers/net/vhost/rte_eth_vhost.c | 59 
>> +++++++++++++++++++--------------------
>>   1 file changed, 29 insertions(+), 30 deletions(-)
>>
>> diff --git a/drivers/net/vhost/rte_eth_vhost.c 
>> b/drivers/net/vhost/rte_eth_vhost.c
>> index 11b6076..e392d71 100644
>> --- a/drivers/net/vhost/rte_eth_vhost.c
>> +++ b/drivers/net/vhost/rte_eth_vhost.c
>> @@ -528,10 +528,11 @@ update_queuing_status(struct rte_eth_dev *dev)
>>       unsigned int i;
>>       int allow_queuing = 1;
>> -    if (rte_atomic32_read(&internal->dev_attached) == 0)
>> +    if (!dev->data->rx_queues || !dev->data->tx_queues)
>>           return;
>> -    if (rte_atomic32_read(&internal->started) == 0)
>> +    if (rte_atomic32_read(&internal->started) == 0 ||
>> +        rte_atomic32_read(&internal->dev_attached) == 0)
>>           allow_queuing = 0;
>>       /* Wait until rx/tx_pkt_burst stops accessing vhost device */
>> @@ -607,13 +608,10 @@ new_device(int vid)
>>   #endif
>>       internal->vid = vid;
>> -    if (eth_dev->data->rx_queues && eth_dev->data->tx_queues) {
>> +    if (rte_atomic32_read(&internal->started) == 1)
>>           queue_setup(eth_dev, internal);
>> -        rte_atomic32_set(&internal->dev_attached, 1);
>> -    } else {
>> -        RTE_LOG(INFO, PMD, "RX/TX queues have not setup yet\n");
>> -        rte_atomic32_set(&internal->dev_attached, 0);
>> -    }
>> +    else
>> +        RTE_LOG(INFO, PMD, "RX/TX queues not exist yet\n");
>>       for (i = 0; i < rte_vhost_get_vring_num(vid); i++)
>>           rte_vhost_enable_guest_notification(vid, i, 0);
>> @@ -622,6 +620,7 @@ new_device(int vid)
>>       eth_dev->data->dev_link.link_status = ETH_LINK_UP;
>> +    rte_atomic32_set(&internal->dev_attached, 1);
>>       update_queuing_status(eth_dev);
>>       RTE_LOG(INFO, PMD, "Vhost device %d created\n", vid);
>> @@ -651,23 +650,24 @@ destroy_device(int vid)
>>       eth_dev = list->eth_dev;
>>       internal = eth_dev->data->dev_private;
>> -    rte_atomic32_set(&internal->started, 0);
>> -    update_queuing_status(eth_dev);
>>       rte_atomic32_set(&internal->dev_attached, 0);
>> +    update_queuing_status(eth_dev);
>>       eth_dev->data->dev_link.link_status = ETH_LINK_DOWN;
>> -    for (i = 0; i < eth_dev->data->nb_rx_queues; i++) {
>> -        vq = eth_dev->data->rx_queues[i];
>> -        if (vq == NULL)
>> -            continue;
>> -        vq->vid = -1;
>> -    }
>> -    for (i = 0; i < eth_dev->data->nb_tx_queues; i++) {
>> -        vq = eth_dev->data->tx_queues[i];
>> -        if (vq == NULL)
>> -            continue;
>> -        vq->vid = -1;
>> +    if (eth_dev->data->rx_queues && eth_dev->data->tx_queues) {
>> +        for (i = 0; i < eth_dev->data->nb_rx_queues; i++) {
>> +            vq = eth_dev->data->rx_queues[i];
>> +            if (!vq)
>> +                continue;
>> +            vq->vid = -1;
>> +        }
>> +        for (i = 0; i < eth_dev->data->nb_tx_queues; i++) {
>> +            vq = eth_dev->data->tx_queues[i];
>> +            if (!vq)
>> +                continue;
>> +            vq->vid = -1;
>> +        }
>>       }
>>       state = vring_states[eth_dev->data->port_id];
>> @@ -792,11 +792,7 @@ eth_dev_start(struct rte_eth_dev *eth_dev)
>>   {
>>       struct pmd_internal *internal = eth_dev->data->dev_private;
>> -    if (unlikely(rte_atomic32_read(&internal->dev_attached) == 0)) {
>> -        queue_setup(eth_dev, internal);
>> -        rte_atomic32_set(&internal->dev_attached, 1);
>> -    }
>> -
>> +    queue_setup(eth_dev, internal);
>>       rte_atomic32_set(&internal->started, 1);
>>       update_queuing_status(eth_dev);
>> @@ -836,10 +832,13 @@ eth_dev_close(struct rte_eth_dev *dev)
>>       pthread_mutex_unlock(&internal_list_lock);
>>       rte_free(list);
>> -    for (i = 0; i < dev->data->nb_rx_queues; i++)
>> -        rte_free(dev->data->rx_queues[i]);
>> -    for (i = 0; i < dev->data->nb_tx_queues; i++)
>> -        rte_free(dev->data->tx_queues[i]);
>> +    if (dev->data->rx_queues)
> 
> This is implied that rx_queues is already allocated. So I don't think we 
> need this.
> 
>> +        for (i = 0; i < dev->data->nb_rx_queues; i++)
>> +            rte_free(dev->data->rx_queues[i]);
>> +
>> +    if (dev->data->tx_queues)
>> +        for (i = 0; i < dev->data->nb_tx_queues; i++)
>> +            rte_free(dev->data->tx_queues[i]);
>>       rte_free(dev->data->mac_addrs);
>>       free(internal->dev_name);
> 

Re: [dpdk-dev] [PATCH v3] net/vhost: fix vhost invalid state

[Chen, Junjie J]

> 
> 
> 
> On 04/12/2018 09:21 AM, Tan, Jianfeng wrote:
> >
> >
> > On 4/12/2018 1:02 AM, Junjie Chen wrote:
> >> dev_start sets *dev_attached* after setup queues, this sets device to
> >> invalid state since no frontend is attached. Also destroy_device set
> >> *started* to zero which makes *allow_queuing* always zero until
> >> dev_start get called again. Actually, we should not determine queues
> >> existence by
> >> *dev_attached* but by queues pointers or other separated variable(s).
> >>
> >> Fixes: 30a701a53737 ("net/vhost: fix crash when creating vdev
> >> dynamically")
> >>
> >> Signed-off-by: Junjie Chen <junjie.j.chen@intel.com>
> >> Tested-by: Jens Freimann <jfreimann@redhat.com>
> >
> > Overall, looks great to me except a nit below.
> >
> > Reviewed-by: Jianfeng Tan <jianfeng.tan@intel.com>
> 
> Thanks Jianfeng, I can handle the small change while applying.
> 
> Can you confirm that it is implied that the queue are already allocated, else we
> wouldn't find the internal resource and quit earlier (in case of eth_dev_close
> called twice for example)?

That is required, otherwise it generate segfault if we close device before queue setup. For example we
execute following steps in testpmd:
1. port attach
2. ctrl+D

> 
> Thanks,
> Maxime
> 
> >
> >> ---
> >> Changes in v3:
> >> - remove useless log in queue status showing Changes in v2:
> >> - use started to determine vhost queues readiness
> >> - revert setting started to zero in destroy_device
> >>   drivers/net/vhost/rte_eth_vhost.c | 59
> >> +++++++++++++++++++--------------------
> >>   1 file changed, 29 insertions(+), 30 deletions(-)
> >>
> >> diff --git a/drivers/net/vhost/rte_eth_vhost.c
> >> b/drivers/net/vhost/rte_eth_vhost.c
> >> index 11b6076..e392d71 100644
> >> --- a/drivers/net/vhost/rte_eth_vhost.c
> >> +++ b/drivers/net/vhost/rte_eth_vhost.c
> >> @@ -528,10 +528,11 @@ update_queuing_status(struct rte_eth_dev
> *dev)
> >>       unsigned int i;
> >>       int allow_queuing = 1;
> >> -    if (rte_atomic32_read(&internal->dev_attached) == 0)
> >> +    if (!dev->data->rx_queues || !dev->data->tx_queues)
> >>           return;
> >> -    if (rte_atomic32_read(&internal->started) == 0)
> >> +    if (rte_atomic32_read(&internal->started) == 0 ||
> >> +        rte_atomic32_read(&internal->dev_attached) == 0)
> >>           allow_queuing = 0;
> >>       /* Wait until rx/tx_pkt_burst stops accessing vhost device */
> >> @@ -607,13 +608,10 @@ new_device(int vid)
> >>   #endif
> >>       internal->vid = vid;
> >> -    if (eth_dev->data->rx_queues && eth_dev->data->tx_queues) {
> >> +    if (rte_atomic32_read(&internal->started) == 1)
> >>           queue_setup(eth_dev, internal);
> >> -        rte_atomic32_set(&internal->dev_attached, 1);
> >> -    } else {
> >> -        RTE_LOG(INFO, PMD, "RX/TX queues have not setup yet\n");
> >> -        rte_atomic32_set(&internal->dev_attached, 0);
> >> -    }
> >> +    else
> >> +        RTE_LOG(INFO, PMD, "RX/TX queues not exist yet\n");
> >>       for (i = 0; i < rte_vhost_get_vring_num(vid); i++)
> >>           rte_vhost_enable_guest_notification(vid, i, 0); @@ -622,6
> >> +620,7 @@ new_device(int vid)
> >>       eth_dev->data->dev_link.link_status = ETH_LINK_UP;
> >> +    rte_atomic32_set(&internal->dev_attached, 1);
> >>       update_queuing_status(eth_dev);
> >>       RTE_LOG(INFO, PMD, "Vhost device %d created\n", vid); @@
> >> -651,23 +650,24 @@ destroy_device(int vid)
> >>       eth_dev = list->eth_dev;
> >>       internal = eth_dev->data->dev_private;
> >> -    rte_atomic32_set(&internal->started, 0);
> >> -    update_queuing_status(eth_dev);
> >>       rte_atomic32_set(&internal->dev_attached, 0);
> >> +    update_queuing_status(eth_dev);
> >>       eth_dev->data->dev_link.link_status = ETH_LINK_DOWN;
> >> -    for (i = 0; i < eth_dev->data->nb_rx_queues; i++) {
> >> -        vq = eth_dev->data->rx_queues[i];
> >> -        if (vq == NULL)
> >> -            continue;
> >> -        vq->vid = -1;
> >> -    }
> >> -    for (i = 0; i < eth_dev->data->nb_tx_queues; i++) {
> >> -        vq = eth_dev->data->tx_queues[i];
> >> -        if (vq == NULL)
> >> -            continue;
> >> -        vq->vid = -1;
> >> +    if (eth_dev->data->rx_queues && eth_dev->data->tx_queues) {
> >> +        for (i = 0; i < eth_dev->data->nb_rx_queues; i++) {
> >> +            vq = eth_dev->data->rx_queues[i];
> >> +            if (!vq)
> >> +                continue;
> >> +            vq->vid = -1;
> >> +        }
> >> +        for (i = 0; i < eth_dev->data->nb_tx_queues; i++) {
> >> +            vq = eth_dev->data->tx_queues[i];
> >> +            if (!vq)
> >> +                continue;
> >> +            vq->vid = -1;
> >> +        }
> >>       }
> >>       state = vring_states[eth_dev->data->port_id];
> >> @@ -792,11 +792,7 @@ eth_dev_start(struct rte_eth_dev *eth_dev)
> >>   {
> >>       struct pmd_internal *internal = eth_dev->data->dev_private;
> >> -    if (unlikely(rte_atomic32_read(&internal->dev_attached) == 0)) {
> >> -        queue_setup(eth_dev, internal);
> >> -        rte_atomic32_set(&internal->dev_attached, 1);
> >> -    }
> >> -
> >> +    queue_setup(eth_dev, internal);
> >>       rte_atomic32_set(&internal->started, 1);
> >>       update_queuing_status(eth_dev); @@ -836,10 +832,13 @@
> >> eth_dev_close(struct rte_eth_dev *dev)
> >>       pthread_mutex_unlock(&internal_list_lock);
> >>       rte_free(list);
> >> -    for (i = 0; i < dev->data->nb_rx_queues; i++)
> >> -        rte_free(dev->data->rx_queues[i]);
> >> -    for (i = 0; i < dev->data->nb_tx_queues; i++)
> >> -        rte_free(dev->data->tx_queues[i]);
> >> +    if (dev->data->rx_queues)
> >
> > This is implied that rx_queues is already allocated. So I don't think
> > we need this.
> >
> >> +        for (i = 0; i < dev->data->nb_rx_queues; i++)
> >> +            rte_free(dev->data->rx_queues[i]);
> >> +
> >> +    if (dev->data->tx_queues)
> >> +        for (i = 0; i < dev->data->nb_tx_queues; i++)
> >> +            rte_free(dev->data->tx_queues[i]);
> >>       rte_free(dev->data->mac_addrs);
> >>       free(internal->dev_name);
> >

Re: [dpdk-dev] [PATCH v3] net/vhost: fix vhost invalid state

[Maxime Coquelin]

On 04/12/2018 09:34 AM, Chen, Junjie J wrote:
>>
>>
>>
>> On 04/12/2018 09:21 AM, Tan, Jianfeng wrote:
>>>
>>>
>>> On 4/12/2018 1:02 AM, Junjie Chen wrote:
>>>> dev_start sets *dev_attached* after setup queues, this sets device to
>>>> invalid state since no frontend is attached. Also destroy_device set
>>>> *started* to zero which makes *allow_queuing* always zero until
>>>> dev_start get called again. Actually, we should not determine queues
>>>> existence by
>>>> *dev_attached* but by queues pointers or other separated variable(s).
>>>>
>>>> Fixes: 30a701a53737 ("net/vhost: fix crash when creating vdev
>>>> dynamically")
>>>>
>>>> Signed-off-by: Junjie Chen <junjie.j.chen@intel.com>
>>>> Tested-by: Jens Freimann <jfreimann@redhat.com>
>>>
>>> Overall, looks great to me except a nit below.
>>>
>>> Reviewed-by: Jianfeng Tan <jianfeng.tan@intel.com>
>>
>> Thanks Jianfeng, I can handle the small change while applying.
>>
>> Can you confirm that it is implied that the queue are already allocated, else we
>> wouldn't find the internal resource and quit earlier (in case of eth_dev_close
>> called twice for example)?
> 
> That is required, otherwise it generate segfault if we close device before queue setup. For example we
> execute following steps in testpmd:
> 1. port attach
> 2. ctrl+D

Thanks for confirming Junjie, I will apply it as is then.

Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime

>>
>> Thanks,
>> Maxime
>>
>>>
>>>> ---
>>>> Changes in v3:
>>>> - remove useless log in queue status showing Changes in v2:
>>>> - use started to determine vhost queues readiness
>>>> - revert setting started to zero in destroy_device
>>>>    drivers/net/vhost/rte_eth_vhost.c | 59
>>>> +++++++++++++++++++--------------------
>>>>    1 file changed, 29 insertions(+), 30 deletions(-)
>>>>
>>>> diff --git a/drivers/net/vhost/rte_eth_vhost.c
>>>> b/drivers/net/vhost/rte_eth_vhost.c
>>>> index 11b6076..e392d71 100644
>>>> --- a/drivers/net/vhost/rte_eth_vhost.c
>>>> +++ b/drivers/net/vhost/rte_eth_vhost.c
>>>> @@ -528,10 +528,11 @@ update_queuing_status(struct rte_eth_dev
>> *dev)
>>>>        unsigned int i;
>>>>        int allow_queuing = 1;
>>>> -    if (rte_atomic32_read(&internal->dev_attached) == 0)
>>>> +    if (!dev->data->rx_queues || !dev->data->tx_queues)
>>>>            return;
>>>> -    if (rte_atomic32_read(&internal->started) == 0)
>>>> +    if (rte_atomic32_read(&internal->started) == 0 ||
>>>> +        rte_atomic32_read(&internal->dev_attached) == 0)
>>>>            allow_queuing = 0;
>>>>        /* Wait until rx/tx_pkt_burst stops accessing vhost device */
>>>> @@ -607,13 +608,10 @@ new_device(int vid)
>>>>    #endif
>>>>        internal->vid = vid;
>>>> -    if (eth_dev->data->rx_queues && eth_dev->data->tx_queues) {
>>>> +    if (rte_atomic32_read(&internal->started) == 1)
>>>>            queue_setup(eth_dev, internal);
>>>> -        rte_atomic32_set(&internal->dev_attached, 1);
>>>> -    } else {
>>>> -        RTE_LOG(INFO, PMD, "RX/TX queues have not setup yet\n");
>>>> -        rte_atomic32_set(&internal->dev_attached, 0);
>>>> -    }
>>>> +    else
>>>> +        RTE_LOG(INFO, PMD, "RX/TX queues not exist yet\n");
>>>>        for (i = 0; i < rte_vhost_get_vring_num(vid); i++)
>>>>            rte_vhost_enable_guest_notification(vid, i, 0); @@ -622,6
>>>> +620,7 @@ new_device(int vid)
>>>>        eth_dev->data->dev_link.link_status = ETH_LINK_UP;
>>>> +    rte_atomic32_set(&internal->dev_attached, 1);
>>>>        update_queuing_status(eth_dev);
>>>>        RTE_LOG(INFO, PMD, "Vhost device %d created\n", vid); @@
>>>> -651,23 +650,24 @@ destroy_device(int vid)
>>>>        eth_dev = list->eth_dev;
>>>>        internal = eth_dev->data->dev_private;
>>>> -    rte_atomic32_set(&internal->started, 0);
>>>> -    update_queuing_status(eth_dev);
>>>>        rte_atomic32_set(&internal->dev_attached, 0);
>>>> +    update_queuing_status(eth_dev);
>>>>        eth_dev->data->dev_link.link_status = ETH_LINK_DOWN;
>>>> -    for (i = 0; i < eth_dev->data->nb_rx_queues; i++) {
>>>> -        vq = eth_dev->data->rx_queues[i];
>>>> -        if (vq == NULL)
>>>> -            continue;
>>>> -        vq->vid = -1;
>>>> -    }
>>>> -    for (i = 0; i < eth_dev->data->nb_tx_queues; i++) {
>>>> -        vq = eth_dev->data->tx_queues[i];
>>>> -        if (vq == NULL)
>>>> -            continue;
>>>> -        vq->vid = -1;
>>>> +    if (eth_dev->data->rx_queues && eth_dev->data->tx_queues) {
>>>> +        for (i = 0; i < eth_dev->data->nb_rx_queues; i++) {
>>>> +            vq = eth_dev->data->rx_queues[i];
>>>> +            if (!vq)
>>>> +                continue;
>>>> +            vq->vid = -1;
>>>> +        }
>>>> +        for (i = 0; i < eth_dev->data->nb_tx_queues; i++) {
>>>> +            vq = eth_dev->data->tx_queues[i];
>>>> +            if (!vq)
>>>> +                continue;
>>>> +            vq->vid = -1;
>>>> +        }
>>>>        }
>>>>        state = vring_states[eth_dev->data->port_id];
>>>> @@ -792,11 +792,7 @@ eth_dev_start(struct rte_eth_dev *eth_dev)
>>>>    {
>>>>        struct pmd_internal *internal = eth_dev->data->dev_private;
>>>> -    if (unlikely(rte_atomic32_read(&internal->dev_attached) == 0)) {
>>>> -        queue_setup(eth_dev, internal);
>>>> -        rte_atomic32_set(&internal->dev_attached, 1);
>>>> -    }
>>>> -
>>>> +    queue_setup(eth_dev, internal);
>>>>        rte_atomic32_set(&internal->started, 1);
>>>>        update_queuing_status(eth_dev); @@ -836,10 +832,13 @@
>>>> eth_dev_close(struct rte_eth_dev *dev)
>>>>        pthread_mutex_unlock(&internal_list_lock);
>>>>        rte_free(list);
>>>> -    for (i = 0; i < dev->data->nb_rx_queues; i++)
>>>> -        rte_free(dev->data->rx_queues[i]);
>>>> -    for (i = 0; i < dev->data->nb_tx_queues; i++)
>>>> -        rte_free(dev->data->tx_queues[i]);
>>>> +    if (dev->data->rx_queues)
>>>
>>> This is implied that rx_queues is already allocated. So I don't think
>>> we need this.
>>>
>>>> +        for (i = 0; i < dev->data->nb_rx_queues; i++)
>>>> +            rte_free(dev->data->rx_queues[i]);
>>>> +
>>>> +    if (dev->data->tx_queues)
>>>> +        for (i = 0; i < dev->data->nb_tx_queues; i++)
>>>> +            rte_free(dev->data->tx_queues[i]);
>>>>        rte_free(dev->data->mac_addrs);
>>>>        free(internal->dev_name);
>>>

Re: [dpdk-dev] [PATCH v3] net/vhost: fix vhost invalid state

[Tan, Jianfeng]

On 4/12/2018 3:29 PM, Maxime Coquelin wrote:
>
>
> On 04/12/2018 09:21 AM, Tan, Jianfeng wrote:
>>
>>
>> On 4/12/2018 1:02 AM, Junjie Chen wrote:
>>> dev_start sets *dev_attached* after setup queues, this sets device to
>>> invalid state since no frontend is attached. Also destroy_device set
>>> *started* to zero which makes *allow_queuing* always zero until 
>>> dev_start
>>> get called again. Actually, we should not determine queues existence by
>>> *dev_attached* but by queues pointers or other separated variable(s).
>>>
>>> Fixes: 30a701a53737 ("net/vhost: fix crash when creating vdev
>>> dynamically")
>>>
>>> Signed-off-by: Junjie Chen <junjie.j.chen@intel.com>
>>> Tested-by: Jens Freimann <jfreimann@redhat.com>
>>
>> Overall, looks great to me except a nit below.
>>
>> Reviewed-by: Jianfeng Tan <jianfeng.tan@intel.com>
>
> Thanks Jianfeng, I can handle the small change while applying.
>
> Can you confirm that it is implied that the queue are already allocated,
> else we wouldn't find the internal resource and quit earlier (in case of
> eth_dev_close called twice for example)?

I was referring to i40e_dev_free_queues() and ixgbe_dev_free_queues(). 
But a 2nd thought, no harm to keep the check.


>
> Thanks,
> Maxime
>
>>
>>> ---
>>> Changes in v3:
>>> - remove useless log in queue status showing
>>> Changes in v2:
>>> - use started to determine vhost queues readiness
>>> - revert setting started to zero in destroy_device
>>>   drivers/net/vhost/rte_eth_vhost.c | 59 
>>> +++++++++++++++++++--------------------
>>>   1 file changed, 29 insertions(+), 30 deletions(-)
>>>
>>> diff --git a/drivers/net/vhost/rte_eth_vhost.c 
>>> b/drivers/net/vhost/rte_eth_vhost.c
>>> index 11b6076..e392d71 100644
>>> --- a/drivers/net/vhost/rte_eth_vhost.c
>>> +++ b/drivers/net/vhost/rte_eth_vhost.c
>>> @@ -528,10 +528,11 @@ update_queuing_status(struct rte_eth_dev *dev)
>>>       unsigned int i;
>>>       int allow_queuing = 1;
>>> -    if (rte_atomic32_read(&internal->dev_attached) == 0)
>>> +    if (!dev->data->rx_queues || !dev->data->tx_queues)
>>>           return;
>>> -    if (rte_atomic32_read(&internal->started) == 0)
>>> +    if (rte_atomic32_read(&internal->started) == 0 ||
>>> +        rte_atomic32_read(&internal->dev_attached) == 0)
>>>           allow_queuing = 0;
>>>       /* Wait until rx/tx_pkt_burst stops accessing vhost device */
>>> @@ -607,13 +608,10 @@ new_device(int vid)
>>>   #endif
>>>       internal->vid = vid;
>>> -    if (eth_dev->data->rx_queues && eth_dev->data->tx_queues) {
>>> +    if (rte_atomic32_read(&internal->started) == 1)
>>>           queue_setup(eth_dev, internal);
>>> -        rte_atomic32_set(&internal->dev_attached, 1);
>>> -    } else {
>>> -        RTE_LOG(INFO, PMD, "RX/TX queues have not setup yet\n");
>>> -        rte_atomic32_set(&internal->dev_attached, 0);
>>> -    }
>>> +    else
>>> +        RTE_LOG(INFO, PMD, "RX/TX queues not exist yet\n");
>>>       for (i = 0; i < rte_vhost_get_vring_num(vid); i++)
>>>           rte_vhost_enable_guest_notification(vid, i, 0);
>>> @@ -622,6 +620,7 @@ new_device(int vid)
>>>       eth_dev->data->dev_link.link_status = ETH_LINK_UP;
>>> +    rte_atomic32_set(&internal->dev_attached, 1);
>>>       update_queuing_status(eth_dev);
>>>       RTE_LOG(INFO, PMD, "Vhost device %d created\n", vid);
>>> @@ -651,23 +650,24 @@ destroy_device(int vid)
>>>       eth_dev = list->eth_dev;
>>>       internal = eth_dev->data->dev_private;
>>> -    rte_atomic32_set(&internal->started, 0);
>>> -    update_queuing_status(eth_dev);
>>>       rte_atomic32_set(&internal->dev_attached, 0);
>>> +    update_queuing_status(eth_dev);
>>>       eth_dev->data->dev_link.link_status = ETH_LINK_DOWN;
>>> -    for (i = 0; i < eth_dev->data->nb_rx_queues; i++) {
>>> -        vq = eth_dev->data->rx_queues[i];
>>> -        if (vq == NULL)
>>> -            continue;
>>> -        vq->vid = -1;
>>> -    }
>>> -    for (i = 0; i < eth_dev->data->nb_tx_queues; i++) {
>>> -        vq = eth_dev->data->tx_queues[i];
>>> -        if (vq == NULL)
>>> -            continue;
>>> -        vq->vid = -1;
>>> +    if (eth_dev->data->rx_queues && eth_dev->data->tx_queues) {
>>> +        for (i = 0; i < eth_dev->data->nb_rx_queues; i++) {
>>> +            vq = eth_dev->data->rx_queues[i];
>>> +            if (!vq)
>>> +                continue;
>>> +            vq->vid = -1;
>>> +        }
>>> +        for (i = 0; i < eth_dev->data->nb_tx_queues; i++) {
>>> +            vq = eth_dev->data->tx_queues[i];
>>> +            if (!vq)
>>> +                continue;
>>> +            vq->vid = -1;
>>> +        }
>>>       }
>>>       state = vring_states[eth_dev->data->port_id];
>>> @@ -792,11 +792,7 @@ eth_dev_start(struct rte_eth_dev *eth_dev)
>>>   {
>>>       struct pmd_internal *internal = eth_dev->data->dev_private;
>>> -    if (unlikely(rte_atomic32_read(&internal->dev_attached) == 0)) {
>>> -        queue_setup(eth_dev, internal);
>>> -        rte_atomic32_set(&internal->dev_attached, 1);
>>> -    }
>>> -
>>> +    queue_setup(eth_dev, internal);
>>>       rte_atomic32_set(&internal->started, 1);
>>>       update_queuing_status(eth_dev);
>>> @@ -836,10 +832,13 @@ eth_dev_close(struct rte_eth_dev *dev)
>>>       pthread_mutex_unlock(&internal_list_lock);
>>>       rte_free(list);
>>> -    for (i = 0; i < dev->data->nb_rx_queues; i++)
>>> -        rte_free(dev->data->rx_queues[i]);
>>> -    for (i = 0; i < dev->data->nb_tx_queues; i++)
>>> -        rte_free(dev->data->tx_queues[i]);
>>> +    if (dev->data->rx_queues)
>>
>> This is implied that rx_queues is already allocated. So I don't think 
>> we need this.
>>
>>> +        for (i = 0; i < dev->data->nb_rx_queues; i++)
>>> +            rte_free(dev->data->rx_queues[i]);
>>> +
>>> +    if (dev->data->tx_queues)
>>> +        for (i = 0; i < dev->data->nb_tx_queues; i++)
>>> +            rte_free(dev->data->tx_queues[i]);
>>>       rte_free(dev->data->mac_addrs);
>>>       free(internal->dev_name);
>>

Re: [dpdk-dev] [PATCH v3] net/vhost: fix vhost invalid state

[Maxime Coquelin]

On 04/12/2018 09:35 AM, Maxime Coquelin wrote:
> 
> 
> On 04/12/2018 09:34 AM, Chen, Junjie J wrote:
>>>
>>>
>>>
>>> On 04/12/2018 09:21 AM, Tan, Jianfeng wrote:
>>>>
>>>>
>>>> On 4/12/2018 1:02 AM, Junjie Chen wrote:
>>>>> dev_start sets *dev_attached* after setup queues, this sets device to
>>>>> invalid state since no frontend is attached. Also destroy_device set
>>>>> *started* to zero which makes *allow_queuing* always zero until
>>>>> dev_start get called again. Actually, we should not determine queues
>>>>> existence by
>>>>> *dev_attached* but by queues pointers or other separated variable(s).
>>>>>
>>>>> Fixes: 30a701a53737 ("net/vhost: fix crash when creating vdev
>>>>> dynamically")
>>>>>
>>>>> Signed-off-by: Junjie Chen <junjie.j.chen@intel.com>
>>>>> Tested-by: Jens Freimann <jfreimann@redhat.com>
>>>>
>>>> Overall, looks great to me except a nit below.
>>>>
>>>> Reviewed-by: Jianfeng Tan <jianfeng.tan@intel.com>
>>>
>>> Thanks Jianfeng, I can handle the small change while applying.
>>>
>>> Can you confirm that it is implied that the queue are already 
>>> allocated, else we
>>> wouldn't find the internal resource and quit earlier (in case of 
>>> eth_dev_close
>>> called twice for example)?
>>
>> That is required, otherwise it generate segfault if we close device 
>> before queue setup. For example we
>> execute following steps in testpmd:
>> 1. port attach
>> 2. ctrl+D
> 
> Thanks for confirming Junjie, I will apply it as is then.
> 
> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> 

Applied to dpdk-next-virtio/master

Thanks,
Maxime