Re: [dpdk-dev] [PATCH v2] net/vhost: fix segfault when creating vdev dynamically

[Maxime Coquelin <maxime.coquelin@redhat.com>]

Hi Junjie,

Thanks for the patch, it looks like the right thing to do.
Please find my comments below:

On 03/29/2018 05:35 PM, Junjie Chen wrote:
> when creating vdev dynamically, vhost pmd driver start directly without
s/when/When/

s/start/starts/
> checking TX/RX queues ready or not, and thus cause segmentation fault when
s/ready/are ready/
s/cause/causes/
> vhost library accessing queues. This patch add flag to check whether queues
s/accessing/accesses/
s/add flag/adds a flag/
> setup or not, and add driver start call into dev_start to allow user start
s/setup/are setup/
s/add/adds/
s/start/ to start/
> it after setting up queue.
s/queue/queues/
> 
> Signed-off-by: Junjie Chen <junjie.j.chen@intel.com>
> ---
> Changes in v2:
> - check queue status in new_device, create queue in dev_start if not setup yet
>   drivers/net/vhost/rte_eth_vhost.c | 73 ++++++++++++++++++++++++++++-----------
>   1 file changed, 53 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c
> index 3aae01c39..41410fa5a 100644
> --- a/drivers/net/vhost/rte_eth_vhost.c
> +++ b/drivers/net/vhost/rte_eth_vhost.c
> @@ -117,7 +117,9 @@ struct pmd_internal {
>   	char *dev_name;
>   	char *iface_name;
>   	uint16_t max_queues;
> +	uint16_t vid;
>   	rte_atomic32_t started;
> +	rte_atomic32_t once;

The name is a bit vague.
Also, I wonder if we need a new variable.
Couldn't we rely on dev_attached? In new_device(), only set
dev_attached=1 if queues are allocated. And in eth_dev_start(),
attach queues if !dev_attached and then set dev_attached to 1.

>   };
>   
>   struct internal_list {
> @@ -580,21 +582,27 @@ new_device(int vid)
>   		eth_dev->data->numa_node = newnode;
>   #endif
>   
> -	for (i = 0; i < eth_dev->data->nb_rx_queues; i++) {
> -		vq = eth_dev->data->rx_queues[i];
> -		if (vq == NULL)
> -			continue;
> -		vq->vid = vid;
> -		vq->internal = internal;
> -		vq->port = eth_dev->data->port_id;
> -	}
> -	for (i = 0; i < eth_dev->data->nb_tx_queues; i++) {
> -		vq = eth_dev->data->tx_queues[i];
> -		if (vq == NULL)
> -			continue;
> -		vq->vid = vid;
> -		vq->internal = internal;
> -		vq->port = eth_dev->data->port_id;
> +	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 = vid;
> +			vq->internal = internal;
> +			vq->port = eth_dev->data->port_id;
> +		}
> +		for (i = 0; i < eth_dev->data->nb_tx_queues; i++) {
> +			vq = eth_dev->data->tx_queues[i];
> +			if (!vq)
> +				continue;
> +			vq->vid = vid;
> +			vq->internal = internal;
> +			vq->port = eth_dev->data->port_id;
> +		}
> +	} else {
> +		RTE_LOG(INFO, PMD, "RX/TX queues have not setup yet\n");
> +		internal->vid = vid;
> +		rte_atomic32_set(&internal->once, 0);
>   	}
>   
>   	for (i = 0; i < rte_vhost_get_vring_num(vid); i++)
> @@ -605,7 +613,8 @@ 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);
> +	if (likely(rte_atomic32_read(&internal->once) == 1))
> +		update_queuing_status(eth_dev);
>   
>   	RTE_LOG(INFO, PMD, "Vhost device %d created\n", vid);
>   
> @@ -770,12 +779,34 @@ rte_eth_vhost_get_vid_from_port_id(uint16_t port_id)
>   }
>   
>   static int
> -eth_dev_start(struct rte_eth_dev *dev)
> +eth_dev_start(struct rte_eth_dev *eth_dev)
>   {
> -	struct pmd_internal *internal = dev->data->dev_private;
> +	struct pmd_internal *internal = eth_dev->data->dev_private;
> +	struct vhost_queue *vq;
> +	int i;
> +
> +	if (unlikely(rte_atomic32_read(&internal->once) == 0)) {
> +		for (i = 0; i < eth_dev->data->nb_rx_queues; i++) {
> +			vq = eth_dev->data->rx_queues[i];
> +			if (!vq)
> +				continue;
> +			vq->vid = internal->vid;
> +			vq->internal = internal;
> +			vq->port = eth_dev->data->port_id;
> +		}
> +		for (i = 0; i < eth_dev->data->nb_tx_queues; i++) {
> +			vq = eth_dev->data->tx_queues[i];
> +			if (!vq)
> +				continue;
> +			vq->vid = internal->vid;
> +			vq->internal = internal;
> +			vq->port = eth_dev->data->port_id;
> +		}

I think you could avoid the code duplication by creating a new function 
to setup queues:

void queue_setup(struct rte_eth_dev *eth_dev,
		 struct pmd_internal *internal)
{
	int i;

	for (i = 0; i < eth_dev->data->nb_rx_queues; i++) {
		vq = eth_dev->data->rx_queues[i];
		if (!vq)
			continue;
		vq->vid = internal->vid;
		vq->internal = internal;
		vq->port = eth_dev->data->port_id;
	}
	for (i = 0; i < eth_dev->data->nb_tx_queues; i++) {
		vq = eth_dev->data->tx_queues[i];
		if (!vq)
			continue;
		vq->vid = internal->vid;
		vq->internal = internal;
		vq->port = eth_dev->data->port_id;
	}
}

> +			rte_atomic32_set(&internal->once, 1);

Indentation looks wrong here.

> +	}
>   
>   	rte_atomic32_set(&internal->started, 1);
> -	update_queuing_status(dev);
> +	update_queuing_status(eth_dev);
>   
>   	return 0;
>   }
> @@ -786,7 +817,9 @@ eth_dev_stop(struct rte_eth_dev *dev)
>   	struct pmd_internal *internal = dev->data->dev_private;
>   
>   	rte_atomic32_set(&internal->started, 0);
> -	update_queuing_status(dev);
> +
> +	if (likely(rte_atomic32_read(&internal->once) == 1))
> +		update_queuing_status(dev);
>   }
>   
>   static void
>