Re: [RFC PATCH v3 2/2] vhost: Add VHOST PMD

[Tetsuya Mukawa <mukawa@igel.co.jp>]

On 2015/10/22 21:49, Bruce Richardson wrote:
> On Thu, Oct 22, 2015 at 06:45:50PM +0900, Tetsuya Mukawa wrote:
>> 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 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=3' -- -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 \
>>         -device virtio-net-pci,netdev=net0
>>
>> Signed-off-by: Tetsuya Mukawa <mukawa@igel.co.jp>
> Hi Tetsuya,
>
> a few comments inline below.
>
> /Bruce
>
>> ---
>>  config/common_linuxapp                      |   6 +
> <snip>
>> index 0000000..66bfc2b
>> --- /dev/null
>> +++ b/drivers/net/vhost/rte_eth_vhost.c
>> @@ -0,0 +1,735 @@
>> +/*-
>> + *   BSD LICENSE
>> + *
>> + *   Copyright (c) 2010-2015 Intel Corporation.
> This is probably not the copyright line you want on your new files.

Hi Bruce,

I appreciate your comments.
Yes, I will change above.

>> +
>> +static uint16_t
>> +eth_vhost_rx(void *q, struct rte_mbuf **bufs, uint16_t nb_bufs)
>> +{
>> +	struct vhost_queue *r = q;
>> +	uint16_t nb_rx = 0;
>> +
>> +	if (unlikely(r->internal == NULL))
>> +		return 0;
>> +
>> +	if (unlikely(rte_atomic32_read(&r->allow_queuing) == 0))
>> +		return 0;
>> +
>> +	rte_atomic32_set(&r->while_queuing, 1);
>> +
>> +	if (unlikely(rte_atomic32_read(&r->allow_queuing) == 0))
>> +		goto out;
>> +
>> +	/* Dequeue packets from guest TX queue */
>> +	nb_rx = (uint16_t)rte_vhost_dequeue_burst(r->device,
>> +			VIRTIO_TXQ, r->mb_pool, bufs, nb_bufs);
>> +
>> +	rte_atomic64_add(&(r->rx_pkts), nb_rx);
> Do we really need to use atomics here? It will slow things down a lot. For
> other PMDs the assumption is always that only a single thread can access each
> queue at a time - it's up to the app to use locks to enforce that restriction
> if necessary.

I agree we don't need to use atomic here.
I will change it in next patches.

>> +static int
>> +new_device(struct virtio_net *dev)
>> +{
>> +	struct rte_eth_dev *eth_dev;
>> +	struct pmd_internal *internal;
>> +	struct vhost_queue *vq;
>> +	uint16_t queues;
>> +	unsigned i;
>> +
>> +	if (dev == NULL) {
>> +		RTE_LOG(INFO, PMD, "invalid argument\n");
>> +		return -1;
>> +	}
>> +
>> +	internal = find_internal_resource(dev->ifname);
>> +	if (internal == NULL) {
>> +		RTE_LOG(INFO, PMD, "invalid device name\n");
>> +		return -1;
>> +	}
>> +
>> +	/*
>> +	 * Todo: To support multi queue, get the number of queues here.
>> +	 * So far, vhost provides only one queue.
>> +	 */
>> +	queues = 1;
>> +
>> +	if ((queues < internal->nb_rx_queues) ||
>> +			(queues < internal->nb_tx_queues)) {
>> +		RTE_LOG(INFO, PMD, "Not enough queues\n");
>> +		return -1;
>> +	}
>> +
>> +	eth_dev = rte_eth_dev_allocated(internal->dev_name);
>> +	if (eth_dev == NULL) {
>> +		RTE_LOG(INFO, PMD, "failuer to find ethdev\n");
> typo "failure". Probably shoudl also be written just as "Failed to find ethdev".

Thanks, I will fix it.

>> +static struct eth_driver rte_vhost_pmd = {
>> +	.pci_drv = {
>> +		.name = "rte_vhost_pmd",
>> +		.drv_flags = RTE_PCI_DRV_DETACHABLE,
>> +	},
>> +};
> If you base this patchset on top of Bernard's patchset to remove the PCI devices
> then you shouldn't need these pci_dev and id_table structures.

Sure, I will check him latest patches. And will rebase on it.

Regards,
Tetsuya