[dpdk-dev] [PATCH v8 3/8] vhost: vring queue setup for multiple queue support

2015-10-27 Thread Yuanhan Liu 
On Tue, Oct 27, 2015 at 11:42:24AM +0200, Michael S. Tsirkin wrote:
...
> > > Looking at that, at least when MQ is enabled, please don't key
> > > stopping queues off GET_VRING_BASE.
> > 
> > Yes, that's only a workaround. I guess it has been there for quite a
> > while, maybe at the time qemu doesn't send RESET_OWNER message.
> 
> RESET_OWNER was a bad idea since it basically closes
> everything.
> 
> > > There are ENABLE/DISABLE messages for that.
> > 
> > That's something new,
> 
> That's part of multiqueue support. If you ignore them,
> nothing works properly.

I will handle them shortly. (well, it may still need weeks :(

> > though I have plan to use them instead, we still
> > need to make sure our code work with old qemu, without ENABLE/DISABLE
> > messages.
> 
> OK but don't rely on this for new code.

Yes.

> 
> > And I will think more while enabling live migration: I should have
> > more time to address issues like this at that time.
> > 
> > > Generally guys, don't take whatever QEMU happens to do for
> > > granted! Look at the protocol spec under doc/specs directory,
> > > if you are making more assumptions you must document them!
> > 
> > Indeed. And we will try to address them bit by bit in future.
> > 
> > --yliu
> 
> But don't pile up these workarounds meanwhile.  I'm very worried.  The
> way you are carrying on, each new QEMU is likely to break your
> assumptions.

Good point. I'll have more discussion with Huawei, to see if we can
fix them sooner.

--yliu

[dpdk-dev] [PATCH v8 3/8] vhost: vring queue setup for multiple queue support

2015-10-27 Thread Yuanhan Liu 
On Tue, Oct 27, 2015 at 11:17:16AM +0200, Michael S. Tsirkin wrote:
> On Tue, Oct 27, 2015 at 03:20:40PM +0900, Tetsuya Mukawa wrote:
> > On 2015/10/26 14:42, Yuanhan Liu wrote:
> > > On Mon, Oct 26, 2015 at 02:24:08PM +0900, Tetsuya Mukawa wrote:
> > >> On 2015/10/22 21:35, Yuanhan Liu wrote:
> > > ...
> > >>> @@ -292,13 +300,13 @@ user_get_vring_base(struct vhost_device_ctx ctx,
> > >>>  * sent and only sent in vhost_vring_stop.
> > >>>  * TODO: cleanup the vring, it isn't usable since here.
> > >>>  */
> > >>> -   if ((dev->virtqueue[VIRTIO_RXQ]->kickfd) >= 0) {
> > >>> -   close(dev->virtqueue[VIRTIO_RXQ]->kickfd);
> > >>> -   dev->virtqueue[VIRTIO_RXQ]->kickfd = -1;
> > >>> +   if ((dev->virtqueue[state->index]->kickfd + VIRTIO_RXQ) >= 0) {
> > >>> +   close(dev->virtqueue[state->index + 
> > >>> VIRTIO_RXQ]->kickfd);
> > >>> +   dev->virtqueue[state->index + VIRTIO_RXQ]->kickfd = -1;
> > >>> }
> > >> Hi Yuanhan,
> > >>
> > >> Please let me make sure whether below is correct.
> > >> if ((dev->virtqueue[state->index]->kickfd + VIRTIO_RXQ) >= 0) {
> > >>
> > >>> -   if ((dev->virtqueue[VIRTIO_TXQ]->kickfd) >= 0) {
> > >>> -   close(dev->virtqueue[VIRTIO_TXQ]->kickfd);
> > >>> -   dev->virtqueue[VIRTIO_TXQ]->kickfd = -1;
> > >>> +   if ((dev->virtqueue[state->index]->kickfd + VIRTIO_TXQ) >= 0) {
> > >>> +   close(dev->virtqueue[state->index + 
> > >>> VIRTIO_TXQ]->kickfd);
> > >>> +   dev->virtqueue[state->index + VIRTIO_TXQ]->kickfd = -1;
> > >> Also, same question here.
> > > Oops, silly typos... Thanks for catching it out!
> > >
> > > Here is an update patch (Thomas, please let me know if you prefer me
> > > to send the whole patchset for you to apply):
> > 
> > Hi Yuanhan,
> > 
> > I may miss one more issue here.
> > Could you please see below patch I've submitted today?
> > (I may find a similar issue, so I've fixed it also in below patch.)
> >  
> > - http://dpdk.org/dev/patchwork/patch/8038/
> >  
> > Thanks,
> > Tetsuya
> 
> Looking at that, at least when MQ is enabled, please don't key
> stopping queues off GET_VRING_BASE.

Yes, that's only a workaround. I guess it has been there for quite a
while, maybe at the time qemu doesn't send RESET_OWNER message.

> There are ENABLE/DISABLE messages for that.

That's something new, though I have plan to use them instead, we still
need to make sure our code work with old qemu, without ENABLE/DISABLE
messages.

And I will think more while enabling live migration: I should have
more time to address issues like this at that time.

> Generally guys, don't take whatever QEMU happens to do for
> granted! Look at the protocol spec under doc/specs directory,
> if you are making more assumptions you must document them!

Indeed. And we will try to address them bit by bit in future.

--yliu

[dpdk-dev] [PATCH v8 3/8] vhost: vring queue setup for multiple queue support

2015-10-27 Thread Tetsuya Mukawa 
On 2015/10/26 14:42, Yuanhan Liu wrote:
> On Mon, Oct 26, 2015 at 02:24:08PM +0900, Tetsuya Mukawa wrote:
>> On 2015/10/22 21:35, Yuanhan Liu wrote:
> ...
>>> @@ -292,13 +300,13 @@ user_get_vring_base(struct vhost_device_ctx ctx,
>>>  * sent and only sent in vhost_vring_stop.
>>>  * TODO: cleanup the vring, it isn't usable since here.
>>>  */
>>> -   if ((dev->virtqueue[VIRTIO_RXQ]->kickfd) >= 0) {
>>> -   close(dev->virtqueue[VIRTIO_RXQ]->kickfd);
>>> -   dev->virtqueue[VIRTIO_RXQ]->kickfd = -1;
>>> +   if ((dev->virtqueue[state->index]->kickfd + VIRTIO_RXQ) >= 0) {
>>> +   close(dev->virtqueue[state->index + VIRTIO_RXQ]->kickfd);
>>> +   dev->virtqueue[state->index + VIRTIO_RXQ]->kickfd = -1;
>>> }
>> Hi Yuanhan,
>>
>> Please let me make sure whether below is correct.
>> if ((dev->virtqueue[state->index]->kickfd + VIRTIO_RXQ) >= 0) {
>>
>>> -   if ((dev->virtqueue[VIRTIO_TXQ]->kickfd) >= 0) {
>>> -   close(dev->virtqueue[VIRTIO_TXQ]->kickfd);
>>> -   dev->virtqueue[VIRTIO_TXQ]->kickfd = -1;
>>> +   if ((dev->virtqueue[state->index]->kickfd + VIRTIO_TXQ) >= 0) {
>>> +   close(dev->virtqueue[state->index + VIRTIO_TXQ]->kickfd);
>>> +   dev->virtqueue[state->index + VIRTIO_TXQ]->kickfd = -1;
>> Also, same question here.
> Oops, silly typos... Thanks for catching it out!
>
> Here is an update patch (Thomas, please let me know if you prefer me
> to send the whole patchset for you to apply):

Hi Yuanhan,

I may miss one more issue here.
Could you please see below patch I've submitted today?
(I may find a similar issue, so I've fixed it also in below patch.)

- http://dpdk.org/dev/patchwork/patch/8038/

Thanks,
Tetsuya

[dpdk-dev] [PATCH v8 3/8] vhost: vring queue setup for multiple queue support

2015-10-27 Thread Michael S. Tsirkin 
On Tue, Oct 27, 2015 at 10:51:14AM +0100, Thomas Monjalon wrote:
> 2015-10-27 11:42, Michael S. Tsirkin:
> > On Tue, Oct 27, 2015 at 05:30:41PM +0800, Yuanhan Liu wrote:
> > > On Tue, Oct 27, 2015 at 11:17:16AM +0200, Michael S. Tsirkin wrote:
> > > > Looking at that, at least when MQ is enabled, please don't key
> > > > stopping queues off GET_VRING_BASE.
> > > 
> > > Yes, that's only a workaround. I guess it has been there for quite a
> > > while, maybe at the time qemu doesn't send RESET_OWNER message.
> > 
> > RESET_OWNER was a bad idea since it basically closes
> > everything.
> > 
> > > > There are ENABLE/DISABLE messages for that.
> > > 
> > > That's something new,
> > 
> > That's part of multiqueue support. If you ignore them,
> > nothing works properly.
> > 
> > > though I have plan to use them instead, we still
> > > need to make sure our code work with old qemu, without ENABLE/DISABLE
> > > messages.
> > 
> > OK but don't rely on this for new code.
> > 
> > > And I will think more while enabling live migration: I should have
> > > more time to address issues like this at that time.
> > > 
> > > > Generally guys, don't take whatever QEMU happens to do for
> > > > granted! Look at the protocol spec under doc/specs directory,
> > > > if you are making more assumptions you must document them!
> > > 
> > > Indeed. And we will try to address them bit by bit in future.
> > > 
> > >   --yliu
> > 
> > But don't pile up these workarounds meanwhile.  I'm very worried.  The
> > way you are carrying on, each new QEMU is likely to break your
> > assumptions.
> 
> I think it may be saner to increase the minimum QEMU version supported in
> each DPDK release, dropping old stuff progressively.
> Michael, you are welcome to suggest how to move precisely.
> Thanks

This doesn't work for downstreams which need to backport fixes and
features.

Just go by the spec, and if you find issues, fix them at the
source instead of working around them - the code is open.

For new features, we have protocol feature bits.

-- 
MST

[dpdk-dev] [PATCH v8 3/8] vhost: vring queue setup for multiple queue support

2015-10-27 Thread Michael S. Tsirkin 
On Tue, Oct 27, 2015 at 05:30:41PM +0800, Yuanhan Liu wrote:
> On Tue, Oct 27, 2015 at 11:17:16AM +0200, Michael S. Tsirkin wrote:
> > On Tue, Oct 27, 2015 at 03:20:40PM +0900, Tetsuya Mukawa wrote:
> > > On 2015/10/26 14:42, Yuanhan Liu wrote:
> > > > On Mon, Oct 26, 2015 at 02:24:08PM +0900, Tetsuya Mukawa wrote:
> > > >> On 2015/10/22 21:35, Yuanhan Liu wrote:
> > > > ...
> > > >>> @@ -292,13 +300,13 @@ user_get_vring_base(struct vhost_device_ctx ctx,
> > > >>>* sent and only sent in vhost_vring_stop.
> > > >>>* TODO: cleanup the vring, it isn't usable since here.
> > > >>>*/
> > > >>> - if ((dev->virtqueue[VIRTIO_RXQ]->kickfd) >= 0) {
> > > >>> - close(dev->virtqueue[VIRTIO_RXQ]->kickfd);
> > > >>> - dev->virtqueue[VIRTIO_RXQ]->kickfd = -1;
> > > >>> + if ((dev->virtqueue[state->index]->kickfd + VIRTIO_RXQ) >= 0) {
> > > >>> + close(dev->virtqueue[state->index + 
> > > >>> VIRTIO_RXQ]->kickfd);
> > > >>> + dev->virtqueue[state->index + VIRTIO_RXQ]->kickfd = -1;
> > > >>>   }
> > > >> Hi Yuanhan,
> > > >>
> > > >> Please let me make sure whether below is correct.
> > > >> if ((dev->virtqueue[state->index]->kickfd + VIRTIO_RXQ) >= 0) {
> > > >>
> > > >>> - if ((dev->virtqueue[VIRTIO_TXQ]->kickfd) >= 0) {
> > > >>> - close(dev->virtqueue[VIRTIO_TXQ]->kickfd);
> > > >>> - dev->virtqueue[VIRTIO_TXQ]->kickfd = -1;
> > > >>> + if ((dev->virtqueue[state->index]->kickfd + VIRTIO_TXQ) >= 0) {
> > > >>> + close(dev->virtqueue[state->index + 
> > > >>> VIRTIO_TXQ]->kickfd);
> > > >>> + dev->virtqueue[state->index + VIRTIO_TXQ]->kickfd = -1;
> > > >> Also, same question here.
> > > > Oops, silly typos... Thanks for catching it out!
> > > >
> > > > Here is an update patch (Thomas, please let me know if you prefer me
> > > > to send the whole patchset for you to apply):
> > > 
> > > Hi Yuanhan,
> > > 
> > > I may miss one more issue here.
> > > Could you please see below patch I've submitted today?
> > > (I may find a similar issue, so I've fixed it also in below patch.)
> > >  
> > > - http://dpdk.org/dev/patchwork/patch/8038/
> > >  
> > > Thanks,
> > > Tetsuya
> > 
> > Looking at that, at least when MQ is enabled, please don't key
> > stopping queues off GET_VRING_BASE.
> 
> Yes, that's only a workaround. I guess it has been there for quite a
> while, maybe at the time qemu doesn't send RESET_OWNER message.

RESET_OWNER was a bad idea since it basically closes
everything.

> > There are ENABLE/DISABLE messages for that.
> 
> That's something new,

That's part of multiqueue support. If you ignore them,
nothing works properly.

> though I have plan to use them instead, we still
> need to make sure our code work with old qemu, without ENABLE/DISABLE
> messages.

OK but don't rely on this for new code.

> And I will think more while enabling live migration: I should have
> more time to address issues like this at that time.
> 
> > Generally guys, don't take whatever QEMU happens to do for
> > granted! Look at the protocol spec under doc/specs directory,
> > if you are making more assumptions you must document them!
> 
> Indeed. And we will try to address them bit by bit in future.
> 
>   --yliu

But don't pile up these workarounds meanwhile.  I'm very worried.  The
way you are carrying on, each new QEMU is likely to break your
assumptions.

-- 
MST

[dpdk-dev] [PATCH v8 3/8] vhost: vring queue setup for multiple queue support

2015-10-27 Thread Michael S. Tsirkin 
On Tue, Oct 27, 2015 at 03:20:40PM +0900, Tetsuya Mukawa wrote:
> On 2015/10/26 14:42, Yuanhan Liu wrote:
> > On Mon, Oct 26, 2015 at 02:24:08PM +0900, Tetsuya Mukawa wrote:
> >> On 2015/10/22 21:35, Yuanhan Liu wrote:
> > ...
> >>> @@ -292,13 +300,13 @@ user_get_vring_base(struct vhost_device_ctx ctx,
> >>>* sent and only sent in vhost_vring_stop.
> >>>* TODO: cleanup the vring, it isn't usable since here.
> >>>*/
> >>> - if ((dev->virtqueue[VIRTIO_RXQ]->kickfd) >= 0) {
> >>> - close(dev->virtqueue[VIRTIO_RXQ]->kickfd);
> >>> - dev->virtqueue[VIRTIO_RXQ]->kickfd = -1;
> >>> + if ((dev->virtqueue[state->index]->kickfd + VIRTIO_RXQ) >= 0) {
> >>> + close(dev->virtqueue[state->index + VIRTIO_RXQ]->kickfd);
> >>> + dev->virtqueue[state->index + VIRTIO_RXQ]->kickfd = -1;
> >>>   }
> >> Hi Yuanhan,
> >>
> >> Please let me make sure whether below is correct.
> >> if ((dev->virtqueue[state->index]->kickfd + VIRTIO_RXQ) >= 0) {
> >>
> >>> - if ((dev->virtqueue[VIRTIO_TXQ]->kickfd) >= 0) {
> >>> - close(dev->virtqueue[VIRTIO_TXQ]->kickfd);
> >>> - dev->virtqueue[VIRTIO_TXQ]->kickfd = -1;
> >>> + if ((dev->virtqueue[state->index]->kickfd + VIRTIO_TXQ) >= 0) {
> >>> + close(dev->virtqueue[state->index + VIRTIO_TXQ]->kickfd);
> >>> + dev->virtqueue[state->index + VIRTIO_TXQ]->kickfd = -1;
> >> Also, same question here.
> > Oops, silly typos... Thanks for catching it out!
> >
> > Here is an update patch (Thomas, please let me know if you prefer me
> > to send the whole patchset for you to apply):
> 
> Hi Yuanhan,
> 
> I may miss one more issue here.
> Could you please see below patch I've submitted today?
> (I may find a similar issue, so I've fixed it also in below patch.)
>  
> - http://dpdk.org/dev/patchwork/patch/8038/
>  
> Thanks,
> Tetsuya

Looking at that, at least when MQ is enabled, please don't key
stopping queues off GET_VRING_BASE.

There are ENABLE/DISABLE messages for that.

Generally guys, don't take whatever QEMU happens to do for
granted! Look at the protocol spec under doc/specs directory,
if you are making more assumptions you must document them!

-- 
MST

[dpdk-dev] [PATCH v8 3/8] vhost: vring queue setup for multiple queue support

2015-10-27 Thread Thomas Monjalon 
2015-10-27 11:42, Michael S. Tsirkin:
> On Tue, Oct 27, 2015 at 05:30:41PM +0800, Yuanhan Liu wrote:
> > On Tue, Oct 27, 2015 at 11:17:16AM +0200, Michael S. Tsirkin wrote:
> > > Looking at that, at least when MQ is enabled, please don't key
> > > stopping queues off GET_VRING_BASE.
> > 
> > Yes, that's only a workaround. I guess it has been there for quite a
> > while, maybe at the time qemu doesn't send RESET_OWNER message.
> 
> RESET_OWNER was a bad idea since it basically closes
> everything.
> 
> > > There are ENABLE/DISABLE messages for that.
> > 
> > That's something new,
> 
> That's part of multiqueue support. If you ignore them,
> nothing works properly.
> 
> > though I have plan to use them instead, we still
> > need to make sure our code work with old qemu, without ENABLE/DISABLE
> > messages.
> 
> OK but don't rely on this for new code.
> 
> > And I will think more while enabling live migration: I should have
> > more time to address issues like this at that time.
> > 
> > > Generally guys, don't take whatever QEMU happens to do for
> > > granted! Look at the protocol spec under doc/specs directory,
> > > if you are making more assumptions you must document them!
> > 
> > Indeed. And we will try to address them bit by bit in future.
> > 
> > --yliu
> 
> But don't pile up these workarounds meanwhile.  I'm very worried.  The
> way you are carrying on, each new QEMU is likely to break your
> assumptions.

I think it may be saner to increase the minimum QEMU version supported in
each DPDK release, dropping old stuff progressively.
Michael, you are welcome to suggest how to move precisely.
Thanks

[dpdk-dev] [PATCH v8 3/8] vhost: vring queue setup for multiple queue support

2015-10-27 Thread Xie, Huawei 
On 10/27/2015 5:56 PM, Michael S. Tsirkin wrote:
> On Tue, Oct 27, 2015 at 10:51:14AM +0100, Thomas Monjalon wrote:
>> 2015-10-27 11:42, Michael S. Tsirkin:
>>> On Tue, Oct 27, 2015 at 05:30:41PM +0800, Yuanhan Liu wrote:
 On Tue, Oct 27, 2015 at 11:17:16AM +0200, Michael S. Tsirkin wrote:
> Looking at that, at least when MQ is enabled, please don't key
> stopping queues off GET_VRING_BASE.
 Yes, that's only a workaround. I guess it has been there for quite a
 while, maybe at the time qemu doesn't send RESET_OWNER message.
>>> RESET_OWNER was a bad idea since it basically closes
>>> everything.
>>>
> There are ENABLE/DISABLE messages for that.
 That's something new,
>>> That's part of multiqueue support. If you ignore them,
>>> nothing works properly.
>>>
 though I have plan to use them instead, we still
 need to make sure our code work with old qemu, without ENABLE/DISABLE
 messages.
>>> OK but don't rely on this for new code.
>>>
 And I will think more while enabling live migration: I should have
 more time to address issues like this at that time.

> Generally guys, don't take whatever QEMU happens to do for
> granted! Look at the protocol spec under doc/specs directory,
> if you are making more assumptions you must document them!
 Indeed. And we will try to address them bit by bit in future.

--yliu
>>> But don't pile up these workarounds meanwhile.  I'm very worried.  The
>>> way you are carrying on, each new QEMU is likely to break your
>>> assumptions.
>> I think it may be saner to increase the minimum QEMU version supported in
>> each DPDK release, dropping old stuff progressively.
>> Michael, you are welcome to suggest how to move precisely.
>> Thanks
> This doesn't work for downstreams which need to backport fixes and
> features.
>
> Just go by the spec, and if you find issues, fix them at the
> source instead of working around them - the code is open.
>
> For new features, we have protocol feature bits.
To me, one requirement is we need clear message(or spec) to know when
virtio device (or better queue granularity) could be processed or should
be stopped from processing. We need have a clear state machine in mind.
For another requirement, we hope QEMU could send vhost an ID to let
vhost-user have the ability to identify the connection. Let us discuss
this in other thread.

[dpdk-dev] [PATCH v8 3/8] vhost: vring queue setup for multiple queue support

2015-10-26 Thread Tetsuya Mukawa 
On 2015/10/22 21:35, Yuanhan Liu wrote:
> All queue pairs, including the default (the first) queue pair,
> are allocated dynamically, when a vring_call message is received
> first time for a specific queue pair.
>
> This is a refactor work for enabling vhost-user multiple queue;
> it should not break anything as it does no functional changes:
> we don't support mq set, so there is only one mq at max.
>
> This patch is based on Changchun's patch.
>
> Signed-off-by: Ouyang Changchun 
> Signed-off-by: Yuanhan Liu 
> Acked-by: Flavio Leitner 
>
> ---
>
> v8: - move virtuque field to the end of `virtio_net' struct.
>
> - Add a FIXME at set_vring_call() for doing vring queue pair
>   allocation.
> ---
>  lib/librte_vhost/rte_virtio_net.h |   3 +-
>  lib/librte_vhost/vhost_user/virtio-net-user.c |  46 
>  lib/librte_vhost/virtio-net.c | 156 
> --
>  3 files changed, 123 insertions(+), 82 deletions(-)
>
> diff --git a/lib/librte_vhost/rte_virtio_net.h 
> b/lib/librte_vhost/rte_virtio_net.h
> index e3a21e5..9a32a95 100644
> --- a/lib/librte_vhost/rte_virtio_net.h
> +++ b/lib/librte_vhost/rte_virtio_net.h
> @@ -96,7 +96,6 @@ struct vhost_virtqueue {
>   * Device structure contains all configuration information relating to the 
> device.
>   */
>  struct virtio_net {
> - struct vhost_virtqueue  *virtqueue[VIRTIO_QNUM];/**< Contains 
> all virtqueue information. */
>   struct virtio_memory*mem;   /**< QEMU memory and memory 
> region information. */
>   uint64_tfeatures;   /**< Negotiated feature set. */
>   uint64_tprotocol_features;  /**< Negotiated 
> protocol feature set. */
> @@ -104,7 +103,9 @@ struct virtio_net {
>   uint32_tflags;  /**< Device flags. Only used to 
> check if device is running on data core. */
>  #define IF_NAME_SZ (PATH_MAX > IFNAMSIZ ? PATH_MAX : IFNAMSIZ)
>   charifname[IF_NAME_SZ]; /**< Name of the tap 
> device or socket path. */
> + uint32_tvirt_qp_nb; /**< number of queue pair we 
> have allocated */
>   void*priv;  /**< private context */
> + struct vhost_virtqueue  *virtqueue[VIRTIO_NET_CTRL_MQ_VQ_PAIRS_MAX];
> /**< Contains all virtqueue information. */
>  } __rte_cache_aligned;
>  
>  /**
> diff --git a/lib/librte_vhost/vhost_user/virtio-net-user.c 
> b/lib/librte_vhost/vhost_user/virtio-net-user.c
> index 6da729d..d62f3d7 100644
> --- a/lib/librte_vhost/vhost_user/virtio-net-user.c
> +++ b/lib/librte_vhost/vhost_user/virtio-net-user.c
> @@ -206,25 +206,33 @@ err_mmap:
>  }
>  
>  static int
> +vq_is_ready(struct vhost_virtqueue *vq)
> +{
> + return vq && vq->desc   &&
> +vq->kickfd != -1 &&
> +vq->callfd != -1;
> +}
> +
> +static int
>  virtio_is_ready(struct virtio_net *dev)
>  {
>   struct vhost_virtqueue *rvq, *tvq;
> + uint32_t i;
>  
> - /* mq support in future.*/
> - rvq = dev->virtqueue[VIRTIO_RXQ];
> - tvq = dev->virtqueue[VIRTIO_TXQ];
> - if (rvq && tvq && rvq->desc && tvq->desc &&
> - (rvq->kickfd != -1) &&
> - (rvq->callfd != -1) &&
> - (tvq->kickfd != -1) &&
> - (tvq->callfd != -1)) {
> - RTE_LOG(INFO, VHOST_CONFIG,
> - "virtio is now ready for processing.\n");
> - return 1;
> + for (i = 0; i < dev->virt_qp_nb; i++) {
> + rvq = dev->virtqueue[i * VIRTIO_QNUM + VIRTIO_RXQ];
> + tvq = dev->virtqueue[i * VIRTIO_QNUM + VIRTIO_TXQ];
> +
> + if (!vq_is_ready(rvq) || !vq_is_ready(tvq)) {
> + RTE_LOG(INFO, VHOST_CONFIG,
> + "virtio is not ready for processing.\n");
> + return 0;
> + }
>   }
> +
>   RTE_LOG(INFO, VHOST_CONFIG,
> - "virtio isn't ready for processing.\n");
> - return 0;
> + "virtio is now ready for processing.\n");
> + return 1;
>  }
>  
>  void
> @@ -292,13 +300,13 @@ user_get_vring_base(struct vhost_device_ctx ctx,
>* sent and only sent in vhost_vring_stop.
>* TODO: cleanup the vring, it isn't usable since here.
>*/
> - if ((dev->virtqueue[VIRTIO_RXQ]->kickfd) >= 0) {
> - close(dev->virtqueue[VIRTIO_RXQ]->kickfd);
> - dev->virtqueue[VIRTIO_RXQ]->kickfd = -1;
> + if ((dev->virtqueue[state->index]->kickfd + VIRTIO_RXQ) >= 0) {
> + close(dev->virtqueue[state->index + VIRTIO_RXQ]->kickfd);
> + dev->virtqueue[state->index + VIRTIO_RXQ]->kickfd = -1;
>   }

Hi Yuanhan,

Please let me make sure whether below is correct.
if ((dev->virtqueue[state->index]->kickfd + VIRTIO_RXQ) >= 0) {

> - if ((dev->virtqueue[VIRTIO_TXQ]->kickfd) >= 0) {
> - close(dev->virtqueue[VIRTIO_TXQ]->kickfd);
> -

[dpdk-dev] [PATCH v8 3/8] vhost: vring queue setup for multiple queue support

2015-10-26 Thread Yuanhan Liu 
On Mon, Oct 26, 2015 at 02:24:08PM +0900, Tetsuya Mukawa wrote:
> On 2015/10/22 21:35, Yuanhan Liu wrote:
...
> > @@ -292,13 +300,13 @@ user_get_vring_base(struct vhost_device_ctx ctx,
> >  * sent and only sent in vhost_vring_stop.
> >  * TODO: cleanup the vring, it isn't usable since here.
> >  */
> > -   if ((dev->virtqueue[VIRTIO_RXQ]->kickfd) >= 0) {
> > -   close(dev->virtqueue[VIRTIO_RXQ]->kickfd);
> > -   dev->virtqueue[VIRTIO_RXQ]->kickfd = -1;
> > +   if ((dev->virtqueue[state->index]->kickfd + VIRTIO_RXQ) >= 0) {
> > +   close(dev->virtqueue[state->index + VIRTIO_RXQ]->kickfd);
> > +   dev->virtqueue[state->index + VIRTIO_RXQ]->kickfd = -1;
> > }
> 
> Hi Yuanhan,
> 
> Please let me make sure whether below is correct.
> if ((dev->virtqueue[state->index]->kickfd + VIRTIO_RXQ) >= 0) {
> 
> > -   if ((dev->virtqueue[VIRTIO_TXQ]->kickfd) >= 0) {
> > -   close(dev->virtqueue[VIRTIO_TXQ]->kickfd);
> > -   dev->virtqueue[VIRTIO_TXQ]->kickfd = -1;
> > +   if ((dev->virtqueue[state->index]->kickfd + VIRTIO_TXQ) >= 0) {
> > +   close(dev->virtqueue[state->index + VIRTIO_TXQ]->kickfd);
> > +   dev->virtqueue[state->index + VIRTIO_TXQ]->kickfd = -1;
> 
> Also, same question here.

Oops, silly typos... Thanks for catching it out!

Here is an update patch (Thomas, please let me know if you prefer me
to send the whole patchset for you to apply):

-- >8 --
>From 2b7d8155b6c9f37bffcbb220e87f7634f329acee Mon Sep 17 00:00:00 2001
From: Yuanhan Liu 
Date: Fri, 18 Sep 2015 16:01:10 +0800
Subject: [PATCH] vhost: vring queue setup for multiple queue support

All queue pairs, including the default (the first) queue pair,
are allocated dynamically, when a vring_call message is received
first time for a specific queue pair.

This is a refactor work for enabling vhost-user multiple queue;
it should not break anything as it does no functional changes:
we don't support mq set, so there is only one mq at max.

This patch is based on Changchun's patch.

Signed-off-by: Ouyang Changchun 
Signed-off-by: Yuanhan Liu 
Acked-by: Flavio Leitner 

---

v9: - fix silly error "dev->virtqueue[state->index]->kickfd + VIRTIO_RXQ"

v8: - move virtuque field to the end of `virtio_net' struct.

- Add a FIXME at set_vring_call() for doing vring queue pair
  allocation.
---
 lib/librte_vhost/rte_virtio_net.h |   3 +-
 lib/librte_vhost/vhost_user/virtio-net-user.c |  46 
 lib/librte_vhost/virtio-net.c | 156 --
 3 files changed, 123 insertions(+), 82 deletions(-)

diff --git a/lib/librte_vhost/rte_virtio_net.h 
b/lib/librte_vhost/rte_virtio_net.h
index e3a21e5..9a32a95 100644
--- a/lib/librte_vhost/rte_virtio_net.h
+++ b/lib/librte_vhost/rte_virtio_net.h
@@ -96,7 +96,6 @@ struct vhost_virtqueue {
  * Device structure contains all configuration information relating to the 
device.
  */
 struct virtio_net {
-   struct vhost_virtqueue  *virtqueue[VIRTIO_QNUM];/**< Contains 
all virtqueue information. */
struct virtio_memory*mem;   /**< QEMU memory and memory 
region information. */
uint64_tfeatures;   /**< Negotiated feature set. */
uint64_tprotocol_features;  /**< Negotiated 
protocol feature set. */
@@ -104,7 +103,9 @@ struct virtio_net {
uint32_tflags;  /**< Device flags. Only used to 
check if device is running on data core. */
 #define IF_NAME_SZ (PATH_MAX > IFNAMSIZ ? PATH_MAX : IFNAMSIZ)
charifname[IF_NAME_SZ]; /**< Name of the tap 
device or socket path. */
+   uint32_tvirt_qp_nb; /**< number of queue pair we 
have allocated */
void*priv;  /**< private context */
+   struct vhost_virtqueue  *virtqueue[VIRTIO_NET_CTRL_MQ_VQ_PAIRS_MAX];
/**< Contains all virtqueue information. */
 } __rte_cache_aligned;

 /**
diff --git a/lib/librte_vhost/vhost_user/virtio-net-user.c 
b/lib/librte_vhost/vhost_user/virtio-net-user.c
index 6da729d..7fc3805 100644
--- a/lib/librte_vhost/vhost_user/virtio-net-user.c
+++ b/lib/librte_vhost/vhost_user/virtio-net-user.c
@@ -206,25 +206,33 @@ err_mmap:
 }

 static int
+vq_is_ready(struct vhost_virtqueue *vq)
+{
+   return vq && vq->desc   &&
+  vq->kickfd != -1 &&
+  vq->callfd != -1;
+}
+
+static int
 virtio_is_ready(struct virtio_net *dev)
 {
struct vhost_virtqueue *rvq, *tvq;
+   uint32_t i;

-   /* mq support in future.*/
-   rvq = dev->virtqueue[VIRTIO_RXQ];
-   tvq = dev->virtqueue[VIRTIO_TXQ];
-   if (rvq && tvq && rvq->desc && tvq->desc &&
-   (rvq->kickfd != -1) &&
-   (rvq->callfd != -1) &&
-   (tvq->kickfd != -1) &&
-   (tvq->callfd != -1)) {
-   RTE_LOG(INFO, VHOST_CONFIG,
-

[dpdk-dev] [PATCH v8 3/8] vhost: vring queue setup for multiple queue support

2015-10-22 Thread Yuanhan Liu 
All queue pairs, including the default (the first) queue pair,
are allocated dynamically, when a vring_call message is received
first time for a specific queue pair.

This is a refactor work for enabling vhost-user multiple queue;
it should not break anything as it does no functional changes:
we don't support mq set, so there is only one mq at max.

This patch is based on Changchun's patch.

Signed-off-by: Ouyang Changchun 
Signed-off-by: Yuanhan Liu 
Acked-by: Flavio Leitner 

---

v8: - move virtuque field to the end of `virtio_net' struct.

- Add a FIXME at set_vring_call() for doing vring queue pair
  allocation.
---
 lib/librte_vhost/rte_virtio_net.h |   3 +-
 lib/librte_vhost/vhost_user/virtio-net-user.c |  46 
 lib/librte_vhost/virtio-net.c | 156 --
 3 files changed, 123 insertions(+), 82 deletions(-)

diff --git a/lib/librte_vhost/rte_virtio_net.h 
b/lib/librte_vhost/rte_virtio_net.h
index e3a21e5..9a32a95 100644
--- a/lib/librte_vhost/rte_virtio_net.h
+++ b/lib/librte_vhost/rte_virtio_net.h
@@ -96,7 +96,6 @@ struct vhost_virtqueue {
  * Device structure contains all configuration information relating to the 
device.
  */
 struct virtio_net {
-   struct vhost_virtqueue  *virtqueue[VIRTIO_QNUM];/**< Contains 
all virtqueue information. */
struct virtio_memory*mem;   /**< QEMU memory and memory 
region information. */
uint64_tfeatures;   /**< Negotiated feature set. */
uint64_tprotocol_features;  /**< Negotiated 
protocol feature set. */
@@ -104,7 +103,9 @@ struct virtio_net {
uint32_tflags;  /**< Device flags. Only used to 
check if device is running on data core. */
 #define IF_NAME_SZ (PATH_MAX > IFNAMSIZ ? PATH_MAX : IFNAMSIZ)
charifname[IF_NAME_SZ]; /**< Name of the tap 
device or socket path. */
+   uint32_tvirt_qp_nb; /**< number of queue pair we 
have allocated */
void*priv;  /**< private context */
+   struct vhost_virtqueue  *virtqueue[VIRTIO_NET_CTRL_MQ_VQ_PAIRS_MAX];
/**< Contains all virtqueue information. */
 } __rte_cache_aligned;

 /**
diff --git a/lib/librte_vhost/vhost_user/virtio-net-user.c 
b/lib/librte_vhost/vhost_user/virtio-net-user.c
index 6da729d..d62f3d7 100644
--- a/lib/librte_vhost/vhost_user/virtio-net-user.c
+++ b/lib/librte_vhost/vhost_user/virtio-net-user.c
@@ -206,25 +206,33 @@ err_mmap:
 }

 static int
+vq_is_ready(struct vhost_virtqueue *vq)
+{
+   return vq && vq->desc   &&
+  vq->kickfd != -1 &&
+  vq->callfd != -1;
+}
+
+static int
 virtio_is_ready(struct virtio_net *dev)
 {
struct vhost_virtqueue *rvq, *tvq;
+   uint32_t i;

-   /* mq support in future.*/
-   rvq = dev->virtqueue[VIRTIO_RXQ];
-   tvq = dev->virtqueue[VIRTIO_TXQ];
-   if (rvq && tvq && rvq->desc && tvq->desc &&
-   (rvq->kickfd != -1) &&
-   (rvq->callfd != -1) &&
-   (tvq->kickfd != -1) &&
-   (tvq->callfd != -1)) {
-   RTE_LOG(INFO, VHOST_CONFIG,
-   "virtio is now ready for processing.\n");
-   return 1;
+   for (i = 0; i < dev->virt_qp_nb; i++) {
+   rvq = dev->virtqueue[i * VIRTIO_QNUM + VIRTIO_RXQ];
+   tvq = dev->virtqueue[i * VIRTIO_QNUM + VIRTIO_TXQ];
+
+   if (!vq_is_ready(rvq) || !vq_is_ready(tvq)) {
+   RTE_LOG(INFO, VHOST_CONFIG,
+   "virtio is not ready for processing.\n");
+   return 0;
+   }
}
+
RTE_LOG(INFO, VHOST_CONFIG,
-   "virtio isn't ready for processing.\n");
-   return 0;
+   "virtio is now ready for processing.\n");
+   return 1;
 }

 void
@@ -292,13 +300,13 @@ user_get_vring_base(struct vhost_device_ctx ctx,
 * sent and only sent in vhost_vring_stop.
 * TODO: cleanup the vring, it isn't usable since here.
 */
-   if ((dev->virtqueue[VIRTIO_RXQ]->kickfd) >= 0) {
-   close(dev->virtqueue[VIRTIO_RXQ]->kickfd);
-   dev->virtqueue[VIRTIO_RXQ]->kickfd = -1;
+   if ((dev->virtqueue[state->index]->kickfd + VIRTIO_RXQ) >= 0) {
+   close(dev->virtqueue[state->index + VIRTIO_RXQ]->kickfd);
+   dev->virtqueue[state->index + VIRTIO_RXQ]->kickfd = -1;
}
-   if ((dev->virtqueue[VIRTIO_TXQ]->kickfd) >= 0) {
-   close(dev->virtqueue[VIRTIO_TXQ]->kickfd);
-   dev->virtqueue[VIRTIO_TXQ]->kickfd = -1;
+   if ((dev->virtqueue[state->index]->kickfd + VIRTIO_TXQ) >= 0) {
+   close(dev->virtqueue[state->index + VIRTIO_TXQ]->kickfd);
+   dev->virtqueue[state->index + VIRTIO_TXQ]->kickfd = -1;
}

return 0;
diff --git