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