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

2016-02-09 Thread 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 \
 
 -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 
>>> 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 
>>>
>>> <...>
 diff --git a/drivers/net/vhost/rte_eth_vhost.c 
 b/drivers/net/vhost/rte_eth_vhost.c
 new file mode 100644
 index 000..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(>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

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

2016-02-08 Thread Ferruh Yigit 
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 \
> >> 
> >> -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 
> > 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 
> >
> > <...>
> >> diff --git a/drivers/net/vhost/rte_eth_vhost.c 
> >> b/drivers/net/vhost/rte_eth_vhost.c
> >> new file mode 100644
> >> index 000..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?

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

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

2016-02-05 Thread Tetsuya Mukawa 
On 2016/02/05 15:35, Yuanhan Liu 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 \
 
 -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 
>>> 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 
>>>
>>> <...>
 diff --git a/drivers/net/vhost/rte_eth_vhost.c 
 b/drivers/net/vhost/rte_eth_vhost.c
 new file mode 100644
 index 000..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.
> You might need update the MAINTAINERS file as well.
>
>   --yliu

Sure thanks!

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

2016-02-05 Thread Tetsuya Mukawa 
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 \
>> 
>> -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 
> 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 
>
> <...>
>> diff --git a/drivers/net/vhost/rte_eth_vhost.c 
>> b/drivers/net/vhost/rte_eth_vhost.c
>> new file mode 100644
>> index 000..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(>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

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

2016-02-04 Thread Tetsuya Mukawa 
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 \

-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 
---
 config/common_linuxapp  |   6 +
 doc/guides/nics/index.rst   |   1 +
 doc/guides/rel_notes/release_2_3.rst|   3 +
 drivers/net/Makefile|   4 +
 drivers/net/vhost/Makefile  |  62 ++
 drivers/net/vhost/rte_eth_vhost.c   | 898 
 drivers/net/vhost/rte_eth_vhost.h   | 109 
 drivers/net/vhost/rte_pmd_vhost_version.map |  11 +
 mk/rte.app.mk   |   6 +
 9 files changed, 1100 insertions(+)
 create mode 100644 drivers/net/vhost/Makefile
 create mode 100644 drivers/net/vhost/rte_eth_vhost.c
 create mode 100644 drivers/net/vhost/rte_eth_vhost.h
 create mode 100644 drivers/net/vhost/rte_pmd_vhost_version.map

diff --git a/config/common_linuxapp b/config/common_linuxapp
index 74bc515..357b557 100644
--- a/config/common_linuxapp
+++ b/config/common_linuxapp
@@ -514,6 +514,12 @@ CONFIG_RTE_LIBRTE_VHOST_NUMA=n
 CONFIG_RTE_LIBRTE_VHOST_DEBUG=n

 #
+# Compile vhost PMD
+# To compile, CONFIG_RTE_LIBRTE_VHOST should be enabled.
+#
+CONFIG_RTE_LIBRTE_PMD_VHOST=y
+
+#
 #Compile Xen domain0 support
 #
 CONFIG_RTE_LIBRTE_XEN_DOM0=n
diff --git a/doc/guides/nics/index.rst b/doc/guides/nics/index.rst
index 33c9cea..5819cdb 100644
--- a/doc/guides/nics/index.rst
+++ b/doc/guides/nics/index.rst
@@ -47,6 +47,7 @@ Network Interface Controller Drivers
 nfp
 szedata2
 virtio
+vhost
 vmxnet3
 pcap_ring

diff --git a/doc/guides/rel_notes/release_2_3.rst 
b/doc/guides/rel_notes/release_2_3.rst
index 99de186..21a38c7 100644
--- a/doc/guides/rel_notes/release_2_3.rst
+++ b/doc/guides/rel_notes/release_2_3.rst
@@ -4,6 +4,9 @@ DPDK Release 2.3
 New Features
 

+* **Added vhost PMD.**
+
+  Added virtual PMD that wraps librte_vhost.

 Resolved Issues
 ---
diff --git a/drivers/net/Makefile b/drivers/net/Makefile
index 6e4497e..4300b93 100644
--- a/drivers/net/Makefile
+++ b/drivers/net/Makefile
@@ -52,5 +52,9 @@ DIRS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) += virtio
 DIRS-$(CONFIG_RTE_LIBRTE_VMXNET3_PMD) += vmxnet3
 DIRS-$(CONFIG_RTE_LIBRTE_PMD_XENVIRT) += xenvirt

+ifeq ($(CONFIG_RTE_LIBRTE_VHOST),y)
+DIRS-$(CONFIG_RTE_LIBRTE_PMD_VHOST) += vhost
+endif # $(CONFIG_RTE_LIBRTE_VHOST)
+
 include $(RTE_SDK)/mk/rte.sharelib.mk
 include $(RTE_SDK)/mk/rte.subdir.mk
diff --git a/drivers/net/vhost/Makefile b/drivers/net/vhost/Makefile
new file mode 100644
index 000..f49a69b
--- /dev/null
+++ b/drivers/net/vhost/Makefile
@@ -0,0 +1,62 @@
+#   BSD LICENSE
+#
+#   Copyright (c) 2010-2016 Intel Corporation.
+#   All rights reserved.
+#
+#   Redistribution and use in source and binary forms, with or without
+#   modification, are permitted provided that the following conditions
+#   are met:
+#
+# * Redistributions of source code must retain the above copyright
+#   notice, this list of conditions and the following disclaimer.
+# * Redistributions in binary form must reproduce the above copyright
+#   notice, this list of conditions and the following disclaimer in
+#   the documentation and/or other materials provided with the
+#   distribution.
+# * Neither the name of Intel corporation nor the names of its
+#   contributors may be used to endorse or promote products derived
+#   from this software without specific prior written permission.
+#
+#   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+#   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+#   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+#   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+#   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+#   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+#   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+#   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+#   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+#   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY

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

2016-02-04 Thread Ferruh Yigit 
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 \
> 
> -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 

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 

<...>
> diff --git a/drivers/net/vhost/rte_eth_vhost.c 
> b/drivers/net/vhost/rte_eth_vhost.c
> new file mode 100644
> index 000..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.

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

> + 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;
> + 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;
> + rte_atomic32_set(>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;
> + rte_atomic32_set(>allow_queuing, 1);
> + }
> +
> + RTE_LOG(INFO, PMD, "New connection established\n");
> +
> + _rte_eth_dev_callback_process(eth_dev, RTE_ETH_EVENT_INTR_LSC);
> +
> + return 0;
> +}
> +

<...>
> +
> +static int
> +vring_state_changed(struct virtio_net *dev, uint16_t vring, int enable)
> +{
> + struct rte_vhost_vring_state *state;
> + struct pmd_internal *internal;
> +#ifdef RTE_LIBRTE_VHOST_NUMA
> + int newnode, ret;
> +#endif
> +
> + if (dev == NULL) {
> + RTE_LOG(ERR, PMD, "Invalid argument\n");
> + return -1;
> + }
> +
> + internal = find_internal_resource(dev->ifname);
> + if (internal == NULL) {
> + RTE_LOG(ERR, PMD, "Invalid interface name: %s\n", dev->ifname);
> + return -1;
> + }
> +
> + state = vring_states[internal->port_id];
> + if (!state) {
> + RTE_LOG(ERR, PMD, "Unused port id: %d\n", internal->port_id);
> + return -1;
> + }
> +
> +#ifdef RTE_LIBRTE_VHOST_NUMA
> + ret  = get_mempolicy(, NULL, 0, dev,
> + MPOL_F_NODE | MPOL_F_ADDR);
> + if (ret < 0) {
> + RTE_LOG(ERR, PMD, "Unknow numa node\n");
> + return -1;
> + }
> +
> + rte_eth_devices[internal->port_id].data->numa_node = newnode;

If you prefer to keep the list of device instead of list of internal data, can 
escape accessing global device array

> +#endif
> + rte_spinlock_lock(>lock);
> + state->cur[vring] = enable;
> + state->max_vring = RTE_MAX(vring, state->max_vring);
> + rte_spinlock_unlock(>lock);
> +
> + RTE_LOG(INFO, PMD, "vring%u is %s\n",
> + vring, enable ? "enabled" : "disabled");
> +
> + _rte_eth_dev_callback_process(_eth_devices[internal->port_id],
> + RTE_ETH_EVENT_QUEUE_STATE_CHANGE);
> +
> + return 0;
> +}
> +