Re: [dpdk-dev] [RFC PATCH v2] vhost: Add VHOST PMD

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

On 2015/09/24 2:47, Loftus, Ciara 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 PMD can have 'iface' parameter like below to specify a path to connect
>> to a virtio-net device.
>>
>> $ ./testpmd -c f -n 4 --vdev 'eth_vhost0,iface=/tmp/sock0' -- -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>
>> ---
>>  config/common_linuxapp                      |   6 +
>>  drivers/net/Makefile                        |   4 +
>>  drivers/net/vhost/Makefile                  |  61 +++
>>  drivers/net/vhost/rte_eth_vhost.c           | 640
>> ++++++++++++++++++++++++++++
>>  drivers/net/vhost/rte_pmd_vhost_version.map |   4 +
>>  mk/rte.app.mk                               |   8 +-
>>  6 files changed, 722 insertions(+), 1 deletion(-)
>>  create mode 100644 drivers/net/vhost/Makefile
>>  create mode 100644 drivers/net/vhost/rte_eth_vhost.c
>>  create mode 100644 drivers/net/vhost/rte_pmd_vhost_version.map
>>
>> +struct pmd_internal {
>> +	TAILQ_ENTRY(pmd_internal) next;
>> +	char *dev_name;
>> +	char *iface_name;
>> +	unsigned nb_rx_queues;
>> +	unsigned nb_tx_queues;
>> +	rte_atomic16_t xfer;
> Is this flag just used to indicate the state of the virtio_net device?
> Ie. if =0 then virtio_dev=NULL and if =1 then virtio_net !=NULL & the VIRTIO_DEV_RUNNING flag is set?

Hi Clara,

I am sorry for very late reply.

Yes, it is. Probably we can optimize it more.
I will change this implementation a bit in next patches.
Could you please check it?

>> +
>> +static uint16_t
>> +eth_vhost_tx(void *q, struct rte_mbuf **bufs, uint16_t nb_bufs)
>> +{
>> +	struct vhost_queue *r = q;
>> +	uint16_t i, nb_tx = 0;
>> +
>> +	if (unlikely(r->internal == NULL))
>> +		return 0;
>> +
>> +	if (unlikely(rte_atomic16_read(&r->internal->xfer) == 0))
>> +		return 0;
>> +
>> +	rte_atomic16_set(&r->tx_executing, 1);
>> +
>> +	if (unlikely(rte_atomic16_read(&r->internal->xfer) == 0))
>> +		goto out;
>> +
>> +	nb_tx = (uint16_t)rte_vhost_enqueue_burst(r->device,
>> +			VIRTIO_RXQ, bufs, nb_bufs);
>> +
>> +	rte_atomic64_add(&(r->tx_pkts), nb_tx);
>> +	rte_atomic64_add(&(r->err_pkts), nb_bufs - nb_tx);
>> +
>> +	for (i = 0; likely(i < nb_tx); i++)
>> +		rte_pktmbuf_free(bufs[i]);
> We may not always want to free these mbufs. For example, if a call is made to rte_eth_tx_burst with buffers from another (non DPDK) source, they may not be ours to free.

Sorry, I am not sure what type of buffer you want to transfer.

This is a PMD that wraps librte_vhost.
And I guess other PMDs cannot handle buffers from another non DPDK source.
Should we take care such buffers?

I have also checked af_packet PMD.
It seems the tx function of af_packet PMD just frees mbuf.

>> +
>> +
>> +	eth_dev = rte_eth_dev_allocated(internal->dev_name);
>> +	if (eth_dev == NULL) {
>> +		RTE_LOG(INFO, PMD, "failuer to find ethdev\n");
> Typo: Failure. Same for the destroy_device function

Thanks, I will fix it in next patches.

>> +		return -1;
>> +	}
>> +
>> +	internal = eth_dev->data->dev_private;
>> +
>> +	for (i = 0; i < internal->nb_rx_queues; i++) {
>> +		vq = &internal->rx_vhost_queues[i];
>> +		vq->device = dev;
>> +		vq->internal = internal;
>> +	}
>> +	for (i = 0; i < internal->nb_tx_queues; i++) {
>> +		vq = &internal->tx_vhost_queues[i];
>> +		vq->device = dev;
>> +		vq->internal = internal;
>> +	}
>> +
>> +	dev->flags |= VIRTIO_DEV_RUNNING;
>> +	dev->priv = eth_dev;
>> +
>> +	eth_dev->data->dev_link.link_status = 1;
>> +	rte_atomic16_set(&internal->xfer, 1);
>> +
>> +	RTE_LOG(INFO, PMD, "New connection established\n");
>> +
>> +	return 0;
> Some freedom is taken away if the new_device and destroy_device callbacks are implemented in the driver.
> For example if one wishes to  call the rte_vhost_enable_guest_notification function when a new device is brought up. They cannot now as there is no scope to modify these callbacks, as is done in for example the vHost sample app. Is this correct?

So how about adding one more parameter to be able to choose guest
notification behavior?

ex)
./testpmd --vdev 'eth_vhost0,iface=/tmp/sock0,guest_notification=0'

In above case, all queues in this device will have VRING_USED_F_NO_NOTIFY.

Thanks,
Tetsuya