Re: [PATCH v7 2/2] vhost: Add VHOST PMD

[Ferruh Yigit <ferruh.yigit@intel.com>]

On Fri, Feb 05, 2016 at 03:28:37PM +0900, Tetsuya Mukawa wrote:
> On 2016/02/04 20:17, Ferruh Yigit wrote:
> > On Thu, Feb 04, 2016 at 04:26:31PM +0900, Tetsuya Mukawa wrote:
> >
> > Hi Tetsuya,
> >
> >> The patch introduces a new PMD. This PMD is implemented as thin wrapper
> >> of librte_vhost. It means librte_vhost is also needed to compile the PMD.
> >> The vhost messages will be handled only when a port is started. So start
> >> a port first, then invoke QEMU.
> >>
> >> The PMD has 2 parameters.
> >>  - iface:  The parameter is used to specify a path to connect to a
> >>            virtio-net device.
> >>  - queues: The parameter is used to specify the number of the queues
> >>            virtio-net device has.
> >>            (Default: 1)
> >>
> >> Here is an example.
> >> $ ./testpmd -c f -n 4 --vdev 'eth_vhost0,iface=/tmp/sock0,queues=1' -- -i
> >>
> >> To connect above testpmd, here is qemu command example.
> >>
> >> $ qemu-system-x86_64 \
> >>         <snip>
> >>         -chardev socket,id=chr0,path=/tmp/sock0 \
> >>         -netdev vhost-user,id=net0,chardev=chr0,vhostforce,queues=1 \
> >>         -device virtio-net-pci,netdev=net0,mq=on
> >>
> >> Signed-off-by: Tetsuya Mukawa <mukawa@igel.co.jp>
> > Please find some more comments, mostly minor nits,
> >
> > please feel free to add my ack for next version of this patch:
> > Acked-by: Ferruh Yigit <ferruh.yigit@intel.com>
> >
> > <...>
> >> diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c
> >> new file mode 100644
> >> index 0000000..b2305c2
> >> --- /dev/null
> >> +++ b/drivers/net/vhost/rte_eth_vhost.c
> > <...>
> >> +
> >> +struct pmd_internal {
> >> +	TAILQ_ENTRY(pmd_internal) next;
> >> +	char *dev_name;
> >> +	char *iface_name;
> >> +	uint8_t port_id;
> > You can also get rid of port_id too, if you keep list of rte_eth_dev.
> > But this is not so important, keep as it is if you want to.
> 
> Thank you so much for checking and good suggestions.
> I will follow your comments without below.
> 
> >> +
> >> +	volatile uint16_t once;
> >> +};
> >> +
> > <...>
> >> +
> >> +static int
> >> +new_device(struct virtio_net *dev)
> >> +{
> > <...>
> >> +
> >> +	for (i = 0; i < eth_dev->data->nb_rx_queues; i++) {
> >> +		vq = eth_dev->data->rx_queues[i];
> >> +		if (vq == NULL)
> > can vq be NULL? It is allocated in rx/tx_queue_setup() and there is already a NULL check there?
> 
> I doubt user may not setup all queues.
> In this case, we need above checking.
> 
> >
> >> +			continue;
> >> +		vq->device = dev;
> >> +		vq->internal = internal;
> >> +		rte_vhost_enable_guest_notification(dev, vq->virtqueue_id, 0);
> >> +	}
> >> +	for (i = 0; i < eth_dev->data->nb_tx_queues; i++) {
> >> +		vq = eth_dev->data->tx_queues[i];
> >> +		if (vq == NULL)
> >> +			continue;
> 
> Same here.
> 
> >> +		vq->device = dev;
> >> +		vq->internal = internal;
> >> +		rte_vhost_enable_guest_notification(dev, vq->virtqueue_id, 0);
> >> +	}
> >> +
> >> +	dev->flags |= VIRTIO_DEV_RUNNING;
> >> +	dev->priv = eth_dev;
> >> +	eth_dev->data->dev_link.link_status = 1;
> >> +
> >> +	for (i = 0; i < eth_dev->data->nb_rx_queues; i++) {
> >> +		vq = eth_dev->data->rx_queues[i];
> >> +		if (vq == NULL)
> >> +			continue;
> 
> But we can remove this.

If in above loop, vq can be NULL because user not setup the queue, it will be NULL in here too, isn't it?
Why we can remove NULL check here?

> 
> >> +		rte_atomic32_set(&vq->allow_queuing, 1);
> >> +	}
> >> +	for (i = 0; i < eth_dev->data->nb_tx_queues; i++) {
> >> +		vq = eth_dev->data->tx_queues[i];
> >> +		if (vq == NULL)
> >> +			continue;
> 
> And this.
> 
> > <...>
> >> +		if (dev->data->rx_queues[i] == NULL)
> >> +			continue;
> >> +		vq = dev->data->rx_queues[i];
> >> +		stats->q_ipackets[i] = vq->rx_pkts;
> >> +		rx_total += stats->q_ipackets[i];
> >> +
> >> +		stats->q_ibytes[i] = vq->rx_bytes;
> >> +		rx_total_bytes += stats->q_ibytes[i];
> >> +	}
> >> +
> >> +	for (i = 0; i < RTE_ETHDEV_QUEUE_STAT_CNTRS &&
> >> +	     i < dev->data->nb_tx_queues; i++) {
> >> +		if (dev->data->tx_queues[i] == NULL)
> > more queue NULL check here, I am not sure if these are necessary
> 
> Same here, in the case user doesn't setup all queues, I will leave this.
> 
> Anyway, I will fix below code.
>  - Manage ether devices list instead of internal structures list.
>  - Remove needless NULL checking.
>  - Replace "pthread_exit" to "return NULL".
>  - Replace rte_panic to RTE_LOG, also add error handling.
>  - Remove duplicated lines.
>  - Remove needless casting.
>  - Follow coding style.
>  - Remove needless parenthesis.
> 
> And leave below.
>  - some NULL checking before accessing a queue.
> 
> Tetsuya