[dpdk-dev] [PATCH v7 2/2] vhost: Add VHOST PMD

[Tetsuya Mukawa]

On 2016/02/08 18:42, Ferruh Yigit wrote:
> 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 at 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 at 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?

Yes, you are right.
Will fix it and submit again.

Thanks,
Tetsuya


>>>> +		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