From mboxrd@z Thu Jan  1 00:00:00 1970
From: Tetsuya Mukawa <mukawa@igel.co.jp>
Subject: Re: [PATCH v7 2/2] vhost: Add VHOST PMD
Date: Fri, 5 Feb 2016 15:28:37 +0900
Message-ID: <56B44115.6090808@igel.co.jp>
References: <1448355603-21275-2-git-send-email-mukawa@igel.co.jp>
 <1454570791-19131-3-git-send-email-mukawa@igel.co.jp>
 <20160204111735.GA30426@sivlogin002.ir.intel.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=windows-1252
Content-Transfer-Encoding: 7bit
Cc: ann.zhuangyanying@huawei.com, yuanhan.liu@intel.com
To: dev@dpdk.org, Ferruh Yigit <ferruh.yigit@intel.com>
Return-path: <dev-bounces@dpdk.org>
Received: from mail-pf0-f170.google.com (mail-pf0-f170.google.com
 [209.85.192.170]) by dpdk.org (Postfix) with ESMTP id C4EA62A5F
 for <dev@dpdk.org>; Fri,  5 Feb 2016 07:28:41 +0100 (CET)
Received: by mail-pf0-f170.google.com with SMTP id 65so63601197pfd.2
 for <dev@dpdk.org>; Thu, 04 Feb 2016 22:28:41 -0800 (PST)
In-Reply-To: <20160204111735.GA30426@sivlogin002.ir.intel.com>
List-Id: patches and discussions about DPDK <dev.dpdk.org>
List-Unsubscribe: <http://dpdk.org/ml/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <http://dpdk.org/ml/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>

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.

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