Re: [Qemu-devel] [Question] why need to start all queues in vhost_net_start

[Jason Wang]

On 2017年11月17日 12:32, Michael S. Tsirkin wrote:

On Thu, Nov 16, 2017 at 08:04:34PM +0800, Jason Wang wrote:


On 2017年11月16日 17:32, Longpeng (Mike) wrote:

Hi Jason,

On 2017/11/16 17:13, Jason Wang wrote:


On 2017年11月16日 17:01, Gonglei (Arei) wrote:

No, Windows guest + vhost-user/DPDK.

BTW pls see virtio spec in :

"If VIRTIO_NET_F_MQ is negotiated, each of receiveq1. . .receiveqN that will
be used SHOULD be populated
with receive buffers."

It is not mandatory that all queues must be initialized.

I think not, since it said we should fill receive buffers for each queue which
means we should initialize all queues. May Michael can clarify on this.


I think this doesn't matter, but QEMU should consider this scenario...

For example, if one queues isn't initialized (Windows guest), the vring.avail=0,
so vq->desc_phys=0, then vq->desc='a avail HVA'(which is the start addr of 
pc.ram).

  vq->desc_size = s = l = virtio_queue_get_desc_size(vdev, idx);
  vq->desc_phys = a = virtio_queue_get_desc_addr(vdev, idx);
  vq->desc = vhost_memory_map(dev, a, , 0);
  if (!vq->desc || l != s) {
  r = -ENOMEM;
  goto fail_alloc_desc;
  }
  .
  r = vhost_virtqueue_set_addr(dev, vq, vhost_vq_index, dev->log_enabled);
  if (r < 0) {
  r = -errno;
  goto fail_alloc;
  }

Then the HVA is send to the vhost-user.

I think this is wrong, because the '0' here means guest driver doesn't init this
queues, it should not be used to calculate the HVA for this vq.

Yes, workaround is not hard if windows driver won't use the left 3 queues
any more. But we should have a complete solution. The main problem is when
vhost need to be started. For legacy device, there's no easy way to detect
whether or not a specific virtqueue is ready to be used. For modern device,
we can probably do this through queue_enable (but this is not implemented in
current code).

Thanks

What isn't implemented?


I mean queue_enable. Virtio spec said:

queue_enable
   The driver uses this to selectively prevent the device from
   executing requests from this virtqueue. 1 - enabled; 0 - disabled. 



But we have:

    case VIRTIO_PCI_COMMON_Q_ENABLE:
    virtio_queue_set_num(vdev, vdev->queue_sel,
 proxy->vqs[vdev->queue_sel].num);
    virtio_queue_set_rings(vdev, vdev->queue_sel,
((uint64_t)proxy->vqs[vdev->queue_sel].desc[1]) << 32 |
   proxy->vqs[vdev->queue_sel].desc[0],
((uint64_t)proxy->vqs[vdev->queue_sel].avail[1]) << 32 |
   proxy->vqs[vdev->queue_sel].avail[0],
((uint64_t)proxy->vqs[vdev->queue_sel].used[1]) << 32 |
   proxy->vqs[vdev->queue_sel].used[0]);
    proxy->vqs[vdev->queue_sel].enabled = 1;
    break;

So it looks to me that we need:

- Not assume the value is 1
- Start or stop vhost virtqueue depends on value

Thanks



Spec is quite explicit:


Client must only process each ring when it is started.

Client must only pass data between the ring and the
backend, when the ring is enabled.

and later:


Client must start ring upon receiving a kick (that is, detecting that file
descriptor is readable) on the descriptor specified by
VHOST_USER_SET_VRING_KICK, and stop ring upon receiving
VHOST_USER_GET_VRING_BASE.


Does someone kick unused rings? What entity does this?





Thanks


Thanks,
-Gonglei




.

Re: [Qemu-devel] [Question] why need to start all queues in vhost_net_start

[Longpeng (Mike)]

On 2017/11/17 12:32, Michael S. Tsirkin wrote:

> On Thu, Nov 16, 2017 at 08:04:34PM +0800, Jason Wang wrote:
>>
>>
>> On 2017年11月16日 17:32, Longpeng (Mike) wrote:
>>> Hi Jason,
>>>
>>> On 2017/11/16 17:13, Jason Wang wrote:
>>>

 On 2017年11月16日 17:01, Gonglei (Arei) wrote:
> No, Windows guest + vhost-user/DPDK.
>
> BTW pls see virtio spec in :
>
> "If VIRTIO_NET_F_MQ is negotiated, each of receiveq1. . .receiveqN that 
> will
> be used SHOULD be populated
> with receive buffers."
>
> It is not mandatory that all queues must be initialized.
 I think not, since it said we should fill receive buffers for each queue 
 which
 means we should initialize all queues. May Michael can clarify on this.

>>>
>>> I think this doesn't matter, but QEMU should consider this scenario...
>>>
>>> For example, if one queues isn't initialized (Windows guest), the 
>>> vring.avail=0,
>>> so vq->desc_phys=0, then vq->desc='a avail HVA'(which is the start addr of 
>>> pc.ram).
>>>
>>>  vq->desc_size = s = l = virtio_queue_get_desc_size(vdev, idx);
>>>  vq->desc_phys = a = virtio_queue_get_desc_addr(vdev, idx);
>>>  vq->desc = vhost_memory_map(dev, a, , 0);
>>>  if (!vq->desc || l != s) {
>>>  r = -ENOMEM;
>>>  goto fail_alloc_desc;
>>>  }
>>>  .
>>>  r = vhost_virtqueue_set_addr(dev, vq, vhost_vq_index, 
>>> dev->log_enabled);
>>>  if (r < 0) {
>>>  r = -errno;
>>>  goto fail_alloc;
>>>  }
>>>
>>> Then the HVA is send to the vhost-user.
>>>
>>> I think this is wrong, because the '0' here means guest driver doesn't init 
>>> this
>>> queues, it should not be used to calculate the HVA for this vq.
>>
>> Yes, workaround is not hard if windows driver won't use the left 3 queues
>> any more. But we should have a complete solution. The main problem is when
>> vhost need to be started. For legacy device, there's no easy way to detect
>> whether or not a specific virtqueue is ready to be used. For modern device,
>> we can probably do this through queue_enable (but this is not implemented in
>> current code).
>>
>> Thanks
> 
> What isn't implemented?
> 
> Spec is quite explicit:
> 
> 
> Client must only process each ring when it is started.
> 
> Client must only pass data between the ring and the
> backend, when the ring is enabled.
> 
> and later:
> 
> 
> Client must start ring upon receiving a kick (that is, detecting that file
> descriptor is readable) on the descriptor specified by
> VHOST_USER_SET_VRING_KICK, and stop ring upon receiving
> VHOST_USER_GET_VRING_BASE.
> 
> 
> Does someone kick unused rings? What entity does this?
> 


Hi Michael,

Maybe you don't get our point, so let me be clear. :)

Suppose there is a VM with 4 vcpus and 7 queues (vhost-user backend), the
Windows guest will only initialize the first 4 queues, so in QEMU the addresses
of the desc/avail/used tables of last 3 queues are all ZERO (initial value).

However QEMU will start all 7 queues, please look at the codes below:
'''
vhost_net_start
  for (i = 0; i < total_queues; i++) // start all 7 queues
vhost_net_start_one
  vhost_dev_start
vhost_virtqueue_start
'''

In vhost_virtqueue_start(), it will use the ZERO to calculate a corresponding
HVA for the last 3 queues and then send to the backend ( by
vhost_virtqueue_set_addr() ).

So our issue here is: the ZERO here means guest doesn't initialize the last 3
queues, so it needn't to calculate the HVA for them and send to then backend.

But, there is no issue on Linux guest, because Linux driver will initialize the
desc/avail/used tables for all 7 queues (even if it only uses the first 4 
queues).

Jason has already get our point, now we need to obtain more information about
Windows virtio-net driver from Yan.

> 
> 
> 
>>>
 Thanks

> Thanks,
> -Gonglei
>



 .

>>>
> 
> .
> 


-- 
Regards,
Longpeng(Mike)

Re: [Qemu-devel] [Question] why need to start all queues in vhost_net_start

[Michael S. Tsirkin]

On Thu, Nov 16, 2017 at 08:04:34PM +0800, Jason Wang wrote:
> 
> 
> On 2017年11月16日 17:32, Longpeng (Mike) wrote:
> > Hi Jason,
> > 
> > On 2017/11/16 17:13, Jason Wang wrote:
> > 
> > > 
> > > On 2017年11月16日 17:01, Gonglei (Arei) wrote:
> > > > No, Windows guest + vhost-user/DPDK.
> > > > 
> > > > BTW pls see virtio spec in :
> > > > 
> > > > "If VIRTIO_NET_F_MQ is negotiated, each of receiveq1. . .receiveqN that 
> > > > will
> > > > be used SHOULD be populated
> > > > with receive buffers."
> > > > 
> > > > It is not mandatory that all queues must be initialized.
> > > I think not, since it said we should fill receive buffers for each queue 
> > > which
> > > means we should initialize all queues. May Michael can clarify on this.
> > > 
> > 
> > I think this doesn't matter, but QEMU should consider this scenario...
> > 
> > For example, if one queues isn't initialized (Windows guest), the 
> > vring.avail=0,
> > so vq->desc_phys=0, then vq->desc='a avail HVA'(which is the start addr of 
> > pc.ram).
> > 
> >  vq->desc_size = s = l = virtio_queue_get_desc_size(vdev, idx);
> >  vq->desc_phys = a = virtio_queue_get_desc_addr(vdev, idx);
> >  vq->desc = vhost_memory_map(dev, a, , 0);
> >  if (!vq->desc || l != s) {
> >  r = -ENOMEM;
> >  goto fail_alloc_desc;
> >  }
> >  .
> >  r = vhost_virtqueue_set_addr(dev, vq, vhost_vq_index, 
> > dev->log_enabled);
> >  if (r < 0) {
> >  r = -errno;
> >  goto fail_alloc;
> >  }
> > 
> > Then the HVA is send to the vhost-user.
> > 
> > I think this is wrong, because the '0' here means guest driver doesn't init 
> > this
> > queues, it should not be used to calculate the HVA for this vq.
> 
> Yes, workaround is not hard if windows driver won't use the left 3 queues
> any more. But we should have a complete solution. The main problem is when
> vhost need to be started. For legacy device, there's no easy way to detect
> whether or not a specific virtqueue is ready to be used. For modern device,
> we can probably do this through queue_enable (but this is not implemented in
> current code).
> 
> Thanks

What isn't implemented?

Spec is quite explicit:


Client must only process each ring when it is started.

Client must only pass data between the ring and the
backend, when the ring is enabled.

and later:


Client must start ring upon receiving a kick (that is, detecting that file
descriptor is readable) on the descriptor specified by
VHOST_USER_SET_VRING_KICK, and stop ring upon receiving
VHOST_USER_GET_VRING_BASE.


Does someone kick unused rings? What entity does this?




> > 
> > > Thanks
> > > 
> > > > Thanks,
> > > > -Gonglei
> > > > 
> > > 
> > > 
> > > 
> > > .
> > > 
> > 

Re: [Qemu-devel] [Question] why need to start all queues in vhost_net_start

[Jason Wang]

On 2017年11月17日 10:01, Longpeng (Mike) wrote:

On 2017/11/16 20:04, Jason Wang wrote:


On 2017年11月16日 17:32, Longpeng (Mike) wrote:

Hi Jason,

On 2017/11/16 17:13, Jason Wang wrote:


On 2017年11月16日 17:01, Gonglei (Arei) wrote:

No, Windows guest + vhost-user/DPDK.

BTW pls see virtio spec in :

"If VIRTIO_NET_F_MQ is negotiated, each of receiveq1. . .receiveqN that will
be used SHOULD be populated
with receive buffers."

It is not mandatory that all queues must be initialized.

I think not, since it said we should fill receive buffers for each queue which
means we should initialize all queues. May Michael can clarify on this.


I think this doesn't matter, but QEMU should consider this scenario...

For example, if one queues isn't initialized (Windows guest), the vring.avail=0,
so vq->desc_phys=0, then vq->desc='a avail HVA'(which is the start addr of
pc.ram).

  vq->desc_size = s = l = virtio_queue_get_desc_size(vdev, idx);
  vq->desc_phys = a = virtio_queue_get_desc_addr(vdev, idx);
  vq->desc = vhost_memory_map(dev, a, , 0);
  if (!vq->desc || l != s) {
  r = -ENOMEM;
  goto fail_alloc_desc;
  }
  .
  r = vhost_virtqueue_set_addr(dev, vq, vhost_vq_index, dev->log_enabled);
  if (r < 0) {
  r = -errno;
  goto fail_alloc;
  }

Then the HVA is send to the vhost-user.

I think this is wrong, because the '0' here means guest driver doesn't init this
queues, it should not be used to calculate the HVA for this vq.

Yes, workaround is not hard if windows driver won't use the left 3 queues any
more. But we should have a complete solution. The main problem is when vhost
need to be started. For legacy device, there's no easy way to detect whether or
not a specific virtqueue is ready to be used. For modern device, we can probably
do this through queue_enable (but this is not implemented in current code).


Can we initialize the vring.desc/vring.avail/vring.used to -1, then we can
detect whether or not a specific vq is ready to be used by check the addr of its
tables is -1 or not ?

Initialize to 0 is not suitable, because we don't know whether the guest set GPA
0 or the guest not initialize this vq.



Unfortunately, it's too late to do this in spec for legacy device. (If 
you look at the spec, legacy MMIO transport treat 0x0 as an illegal 
value.) For modern device, we could detect this through queue_enable.


Let's wait for Yan to reply whether or not windows driver can use the 
left three queues. If not, I will try to post a fix(workaround) for this 
and let's consider a complete solution.


Thanks

Re: [Qemu-devel] [Question] why need to start all queues in vhost_net_start

[Longpeng (Mike)]

On 2017/11/16 20:04, Jason Wang wrote:

> 
> 
> On 2017年11月16日 17:32, Longpeng (Mike) wrote:
>> Hi Jason,
>>
>> On 2017/11/16 17:13, Jason Wang wrote:
>>
>>>
>>> On 2017年11月16日 17:01, Gonglei (Arei) wrote:
 No, Windows guest + vhost-user/DPDK.

 BTW pls see virtio spec in :

 "If VIRTIO_NET_F_MQ is negotiated, each of receiveq1. . .receiveqN that 
 will
 be used SHOULD be populated
 with receive buffers."

 It is not mandatory that all queues must be initialized.
>>> I think not, since it said we should fill receive buffers for each queue 
>>> which
>>> means we should initialize all queues. May Michael can clarify on this.
>>>
>>
>> I think this doesn't matter, but QEMU should consider this scenario...
>>
>> For example, if one queues isn't initialized (Windows guest), the 
>> vring.avail=0,
>> so vq->desc_phys=0, then vq->desc='a avail HVA'(which is the start addr of
>> pc.ram).
>>
>>  vq->desc_size = s = l = virtio_queue_get_desc_size(vdev, idx);
>>  vq->desc_phys = a = virtio_queue_get_desc_addr(vdev, idx);
>>  vq->desc = vhost_memory_map(dev, a, , 0);
>>  if (!vq->desc || l != s) {
>>  r = -ENOMEM;
>>  goto fail_alloc_desc;
>>  }
>>  .
>>  r = vhost_virtqueue_set_addr(dev, vq, vhost_vq_index, dev->log_enabled);
>>  if (r < 0) {
>>  r = -errno;
>>  goto fail_alloc;
>>  }
>>
>> Then the HVA is send to the vhost-user.
>>
>> I think this is wrong, because the '0' here means guest driver doesn't init 
>> this
>> queues, it should not be used to calculate the HVA for this vq.
> 
> Yes, workaround is not hard if windows driver won't use the left 3 queues any
> more. But we should have a complete solution. The main problem is when vhost
> need to be started. For legacy device, there's no easy way to detect whether 
> or
> not a specific virtqueue is ready to be used. For modern device, we can 
> probably
> do this through queue_enable (but this is not implemented in current code).
> 


Can we initialize the vring.desc/vring.avail/vring.used to -1, then we can
detect whether or not a specific vq is ready to be used by check the addr of its
tables is -1 or not ?

Initialize to 0 is not suitable, because we don't know whether the guest set GPA
0 or the guest not initialize this vq.

> Thanks
> 
>>
>>> Thanks
>>>
 Thanks,
 -Gonglei

>>>
>>>
>>>
>>> .
>>>
>>
> 
> 
> .
> 


-- 
Regards,
Longpeng(Mike)

Re: [Qemu-devel] [Question] why need to start all queues in vhost_net_start

[Jason Wang]

On 2017年11月16日 17:32, Longpeng (Mike) wrote:

Hi Jason,

On 2017/11/16 17:13, Jason Wang wrote:



On 2017年11月16日 17:01, Gonglei (Arei) wrote:

No, Windows guest + vhost-user/DPDK.

BTW pls see virtio spec in :

"If VIRTIO_NET_F_MQ is negotiated, each of receiveq1. . .receiveqN that will
be used SHOULD be populated
with receive buffers."

It is not mandatory that all queues must be initialized.

I think not, since it said we should fill receive buffers for each queue which
means we should initialize all queues. May Michael can clarify on this.



I think this doesn't matter, but QEMU should consider this scenario...

For example, if one queues isn't initialized (Windows guest), the vring.avail=0,
so vq->desc_phys=0, then vq->desc='a avail HVA'(which is the start addr of 
pc.ram).

 vq->desc_size = s = l = virtio_queue_get_desc_size(vdev, idx);
 vq->desc_phys = a = virtio_queue_get_desc_addr(vdev, idx);
 vq->desc = vhost_memory_map(dev, a, , 0);
 if (!vq->desc || l != s) {
 r = -ENOMEM;
 goto fail_alloc_desc;
 }
 .
 r = vhost_virtqueue_set_addr(dev, vq, vhost_vq_index, dev->log_enabled);
 if (r < 0) {
 r = -errno;
 goto fail_alloc;
 }

Then the HVA is send to the vhost-user.

I think this is wrong, because the '0' here means guest driver doesn't init this
queues, it should not be used to calculate the HVA for this vq.


Yes, workaround is not hard if windows driver won't use the left 3 
queues any more. But we should have a complete solution. The main 
problem is when vhost need to be started. For legacy device, there's no 
easy way to detect whether or not a specific virtqueue is ready to be 
used. For modern device, we can probably do this through queue_enable 
(but this is not implemented in current code).


Thanks




Thanks


Thanks,
-Gonglei





.

Re: [Qemu-devel] [Question] why need to start all queues in vhost_net_start

[Jason Wang]

On 2017年11月16日 16:11, Yan Vugenfirer wrote:

Hi Jason,

Windows driver will initialise only the amount of queue based on the amount of 
available vCPUs. So if there will be more queues in the device than we have 
vCPUs on the guest, the driver will not initialise “excessive” queues. This is 
tied to the way RSS on Windows should be implemented.
Exactly as in described scenario (7 queues, but only 4 vCPUs).


I see but is there any chance that driver can use the left 3 queues (e 
.g cpu hotplug)?


Thanks



Best regards,
Yan.


On 16 Nov 2017, at 07:53, Longpeng (Mike)  wrote:

Hi Jason & Michael,

Do you have any idea about this problem ?

--
Regards,
Longpeng(Mike)

On 2017/11/15 23:54, Longpeng(Mike) wrote:


2017-11-15 23:05 GMT+08:00 Jason Wang :


On 2017年11月15日 22:55, Longpeng(Mike) wrote:

Hi guys,

We got a BUG report from our testers yesterday, the testing scenario was
migrating a VM (Windows guest, *4 vcpus*, 4GB, vhost-user net: *7
queues*).

We found the cause reason, and we'll report the BUG or send a fix patch
to upstream if necessary( we haven't test the upstream yet, sorry... ).


Could you explain this a little bit more?


We want to know why the vhost_net_start() must start *total queues* ( in
our
VM there're 7 queues ) but not *the queues that current used* ( in our VM,
guest
only uses the first 4 queues because it's limited by the number of vcpus)
?

Looking forward to your help, thx :)


Since the codes have been there for years and works well for kernel
datapath. You should really explain what's wrong.


OK. :)

In our scenario,  the Windows's virtio-net driver only use the first 4
queues and it
*only set desc/avail/used table for the first 4 queues*, so in QEMU
the desc/avail/
used of the last 3 queues are ZERO,  but unfortunately...
'''
vhost_net_start
  for (i = 0; i < total_queues; i++)
vhost_net_start_one
  vhost_dev_start
vhost_virtqueue_start
'''
In vhost_virtqueue_start(), it will calculate the HVA of
desc/avail/used table, so for last
3 queues, it will use ZERO as the GPA to calculate the HVA, and then
send the results
to the user-mode backend ( we use *vhost-user* ) by vhost_virtqueue_set_addr().

When the EVS get these address, it will update a *idx* which will be
treated as  vq's
last_avail_idx when virtio-net stop ( pls see vhost_virtqueue_stop() ).

So we get the following result after virtio-net stop:
  the desc/avail/used of the last 3 queues's vqs are all ZERO, but these vqs's
  last_avail_idx is NOT ZERO.

At last, virtio_load() reports an error:
'''
  if (!vdev->vq[i].vring.desc && vdev->vq[i].last_avail_idx) { // <--
will be TRUE
  error_report("VQ %d address 0x0 "
 "inconsistent with Host index 0x%x",
 i, vdev->vq[i].last_avail_idx);
return -1;
   }
'''

BTW, the problem won't appear if use Linux guest, because the Linux virtio-net
driver will set all 7 queues's desc/avail/used tables. And the problem
won't appear
if the VM use vhost-net, because vhost-net won't update *idx* in SET_ADDR ioctl.

Sorry for my pool English, Maybe I could describe the problem in Chinese for you
in private if necessary.



Thanks




--
Regards,
Longpeng(Mike)

Re: [Qemu-devel] [Question] why need to start all queues in vhost_net_start

[Longpeng (Mike)]

Hi Jason,

On 2017/11/16 17:13, Jason Wang wrote:

> 
> 
> On 2017年11月16日 17:01, Gonglei (Arei) wrote:
>> No, Windows guest + vhost-user/DPDK.
>>
>> BTW pls see virtio spec in :
>>
>> "If VIRTIO_NET_F_MQ is negotiated, each of receiveq1. . .receiveqN that will
>> be used SHOULD be populated
>> with receive buffers."
>>
>> It is not mandatory that all queues must be initialized.
> 
> I think not, since it said we should fill receive buffers for each queue which
> means we should initialize all queues. May Michael can clarify on this.
> 


I think this doesn't matter, but QEMU should consider this scenario...

For example, if one queues isn't initialized (Windows guest), the vring.avail=0,
so vq->desc_phys=0, then vq->desc='a avail HVA'(which is the start addr of 
pc.ram).

vq->desc_size = s = l = virtio_queue_get_desc_size(vdev, idx);
vq->desc_phys = a = virtio_queue_get_desc_addr(vdev, idx);
vq->desc = vhost_memory_map(dev, a, , 0);
if (!vq->desc || l != s) {
r = -ENOMEM;
goto fail_alloc_desc;
}
.
r = vhost_virtqueue_set_addr(dev, vq, vhost_vq_index, dev->log_enabled);
if (r < 0) {
r = -errno;
goto fail_alloc;
}

Then the HVA is send to the vhost-user.

I think this is wrong, because the '0' here means guest driver doesn't init this
queues, it should not be used to calculate the HVA for this vq.

> Thanks
> 
>>
>> Thanks,
>> -Gonglei
>>
> 
> 
> 
> 
> .
> 


-- 
Regards,
Longpeng(Mike)

Re: [Qemu-devel] [Question] why need to start all queues in vhost_net_start

[Jason Wang]

On 2017年11月16日 17:01, Gonglei (Arei) wrote:

No, Windows guest + vhost-user/DPDK.

BTW pls see virtio spec in :

"If VIRTIO_NET_F_MQ is negotiated, each of receiveq1. . .receiveqN that will be 
used SHOULD be populated
with receive buffers."

It is not mandatory that all queues must be initialized.


I think not, since it said we should fill receive buffers for each queue 
which means we should initialize all queues. May Michael can clarify on 
this.


Thanks



Thanks,
-Gonglei

Re: [Qemu-devel] [Question] why need to start all queues in vhost_net_start

[Longpeng (Mike)]

On 2017/11/16 16:54, Jason Wang wrote:

> 
> 
> On 2017年11月16日 13:53, Longpeng (Mike) wrote:
>> On 2017/11/15 23:54, Longpeng(Mike) wrote:
>>> 2017-11-15 23:05 GMT+08:00 Jason Wang:
 On 2017年11月15日 22:55, Longpeng(Mike) wrote:
> Hi guys,
>
> We got a BUG report from our testers yesterday, the testing scenario was
> migrating a VM (Windows guest, *4 vcpus*, 4GB, vhost-user net: *7
> queues*).
>
> We found the cause reason, and we'll report the BUG or send a fix patch
> to upstream if necessary( we haven't test the upstream yet, sorry... ).
 Could you explain this a little bit more?

> We want to know why the vhost_net_start() must start*total queues*  ( in
> our
> VM there're 7 queues ) but not*the queues that current used*  ( in our VM,
> guest
> only uses the first 4 queues because it's limited by the number of vcpus)
> ?
>
> Looking forward to your help, thx:)
 Since the codes have been there for years and works well for kernel
 datapath. You should really explain what's wrong.

>>> OK.:)
>>>
>>> In our scenario,  the Windows's virtio-net driver only use the first 4
>>> queues and it
>>> *only set desc/avail/used table for the first 4 queues*, so in QEMU
>>> the desc/avail/
>>> used of the last 3 queues are ZERO,  but unfortunately...
>>> '''
>>> vhost_net_start
>>>for (i = 0; i < total_queues; i++)
>>>  vhost_net_start_one
>>>vhost_dev_start
>>>  vhost_virtqueue_start
>>> '''
>>> In vhost_virtqueue_start(), it will calculate the HVA of
>>> desc/avail/used table, so for last
>>> 3 queues, it will use ZERO as the GPA to calculate the HVA, and then
>>> send the results
>>> to the user-mode backend ( we use*vhost-user*  ) by 
>>> vhost_virtqueue_set_addr().
>>>
>>> When the EVS get these address, it will update a*idx*  which will be
>>> treated as  vq's
>>> last_avail_idx when virtio-net stop ( pls see vhost_virtqueue_stop() ).
>>>
>>> So we get the following result after virtio-net stop:
>>>the desc/avail/used of the last 3 queues's vqs are all ZERO, but these 
>>> vqs's
>>>last_avail_idx is NOT ZERO.
>>>
>>> At last, virtio_load() reports an error:
>>> '''
>>>if (!vdev->vq[i].vring.desc && vdev->vq[i].last_avail_idx) { // <--
>>> will be TRUE
>>>error_report("VQ %d address 0x0 "
>>>   "inconsistent with Host index 0x%x",
>>>   i, vdev->vq[i].last_avail_idx);
>>>  return -1;
>>> }
>>> '''
>>>
>>> BTW, the problem won't appear if use Linux guest, because the Linux 
>>> virtio-net
>>> driver will set all 7 queues's desc/avail/used tables. And the problem
>>> won't appear
>>> if the VM use vhost-net, because vhost-net won't update*idx*  in SET_ADDR 
>>> ioctl.
> 
> Just to make sure I understand here, I thought Windows guest + vhost_net hit
> this issue?
> 


Windows guest + vhost-user hit.
Windows guest + vhost-net is fine.

'''
In vhost_virtqueue_start(), it will calculate the HVA of
desc/avail/used tables, so for last
3 queues, it will use ZERO as the GPA to calculate the HVA, and then
send the results
to the user-mode backend ( we use *vhost-user* ) by vhost_virtqueue_set_addr().
'''
I think this is the root cause, it is strange, right ?

> Thanks
> 
>>>
>>> Sorry for my pool English, Maybe I could describe the problem in Chinese 
>>> for you
>>> in private if necessary.
>>>
>>>
 Thanks
>> -- Regards, Longpeng(Mike)
> 
> 
> .
> 


-- 
Regards,
Longpeng(Mike)

Re: [Qemu-devel] [Question] why need to start all queues in vhost_net_start

[Jason Wang]

On 2017年11月16日 17:01, Gonglei (Arei) wrote:



-Original Message-
From: Jason Wang [mailto:jasow...@redhat.com]
Sent: Thursday, November 16, 2017 4:55 PM
To: longpeng; m...@redhat.com
Cc: Longpeng(Mike); qemu-devel@nongnu.org; Gonglei (Arei); Wangjing (King,
Euler); Huangweidong (C); stefa...@redhat.com
Subject: Re: [Question] why need to start all queues in vhost_net_start



On 2017年11月16日 13:53, Longpeng (Mike) wrote:

On 2017/11/15 23:54, Longpeng(Mike) wrote:

2017-11-15 23:05 GMT+08:00 Jason Wang:

On 2017年11月15日 22:55, Longpeng(Mike) wrote:

Hi guys,

We got a BUG report from our testers yesterday, the testing scenario was
migrating a VM (Windows guest, *4 vcpus*, 4GB, vhost-user net: *7
queues*).

We found the cause reason, and we'll report the BUG or send a fix patch
to upstream if necessary( we haven't test the upstream yet, sorry... ).

Could you explain this a little bit more?


We want to know why the vhost_net_start() must start*total queues*

( in

our
VM there're 7 queues ) but not*the queues that current used*  ( in our

VM,

guest
only uses the first 4 queues because it's limited by the number of vcpus)
?

Looking forward to your help, thx:)

Since the codes have been there for years and works well for kernel
datapath. You should really explain what's wrong.


OK.:)

In our scenario,  the Windows's virtio-net driver only use the first 4
queues and it
*only set desc/avail/used table for the first 4 queues*, so in QEMU
the desc/avail/
used of the last 3 queues are ZERO,  but unfortunately...
'''
vhost_net_start
for (i = 0; i < total_queues; i++)
  vhost_net_start_one
vhost_dev_start
  vhost_virtqueue_start
'''
In vhost_virtqueue_start(), it will calculate the HVA of
desc/avail/used table, so for last
3 queues, it will use ZERO as the GPA to calculate the HVA, and then
send the results
to the user-mode backend ( we use*vhost-user*  ) by

vhost_virtqueue_set_addr().

When the EVS get these address, it will update a*idx*  which will be
treated as  vq's
last_avail_idx when virtio-net stop ( pls see vhost_virtqueue_stop() ).

So we get the following result after virtio-net stop:
the desc/avail/used of the last 3 queues's vqs are all ZERO, but these

vqs's

last_avail_idx is NOT ZERO.

At last, virtio_load() reports an error:
'''
if (!vdev->vq[i].vring.desc && vdev->vq[i].last_avail_idx) { // <--
will be TRUE
error_report("VQ %d address 0x0 "
   "inconsistent with Host index 0x%x",
   i, vdev->vq[i].last_avail_idx);
  return -1;
 }
'''

BTW, the problem won't appear if use Linux guest, because the Linux

virtio-net

driver will set all 7 queues's desc/avail/used tables. And the problem
won't appear
if the VM use vhost-net, because vhost-net won't update*idx*  in

SET_ADDR ioctl.

Just to make sure I understand here, I thought Windows guest + vhost_net
hit this issue?


No, Windows guest + vhost-user/DPDK.

BTW pls see virtio spec in :

"If VIRTIO_NET_F_MQ is negotiated, each of receiveq1. . .receiveqN that will be 
used SHOULD be populated
with receive buffers."

It is not mandatory that all queues must be initialized.

Thanks,
-Gonglei



Interesting, vhost_net will set last_avail_idx to vq.num during 
SET_VRING_BASE. So I thought it should hit this.


Btw, maybe we should relax the check to:

if (!vdev->vq[i].vring.desc && (vdev->vq[i].last_avail_idx != 
vdev->vq[i].vring.num)) {


Thanks

Re: [Qemu-devel] [Question] why need to start all queues in vhost_net_start

[Gonglei (Arei)]

> -Original Message-
> From: Jason Wang [mailto:jasow...@redhat.com]
> Sent: Thursday, November 16, 2017 4:55 PM
> To: longpeng; m...@redhat.com
> Cc: Longpeng(Mike); qemu-devel@nongnu.org; Gonglei (Arei); Wangjing (King,
> Euler); Huangweidong (C); stefa...@redhat.com
> Subject: Re: [Question] why need to start all queues in vhost_net_start
> 
> 
> 
> On 2017年11月16日 13:53, Longpeng (Mike) wrote:
> > On 2017/11/15 23:54, Longpeng(Mike) wrote:
> >> 2017-11-15 23:05 GMT+08:00 Jason Wang:
> >>> On 2017年11月15日 22:55, Longpeng(Mike) wrote:
>  Hi guys,
> 
>  We got a BUG report from our testers yesterday, the testing scenario was
>  migrating a VM (Windows guest, *4 vcpus*, 4GB, vhost-user net: *7
>  queues*).
> 
>  We found the cause reason, and we'll report the BUG or send a fix patch
>  to upstream if necessary( we haven't test the upstream yet, sorry... ).
> >>> Could you explain this a little bit more?
> >>>
>  We want to know why the vhost_net_start() must start*total queues*
> ( in
>  our
>  VM there're 7 queues ) but not*the queues that current used*  ( in our
> VM,
>  guest
>  only uses the first 4 queues because it's limited by the number of vcpus)
>  ?
> 
>  Looking forward to your help, thx:)
> >>> Since the codes have been there for years and works well for kernel
> >>> datapath. You should really explain what's wrong.
> >>>
> >> OK.:)
> >>
> >> In our scenario,  the Windows's virtio-net driver only use the first 4
> >> queues and it
> >> *only set desc/avail/used table for the first 4 queues*, so in QEMU
> >> the desc/avail/
> >> used of the last 3 queues are ZERO,  but unfortunately...
> >> '''
> >> vhost_net_start
> >>for (i = 0; i < total_queues; i++)
> >>  vhost_net_start_one
> >>vhost_dev_start
> >>  vhost_virtqueue_start
> >> '''
> >> In vhost_virtqueue_start(), it will calculate the HVA of
> >> desc/avail/used table, so for last
> >> 3 queues, it will use ZERO as the GPA to calculate the HVA, and then
> >> send the results
> >> to the user-mode backend ( we use*vhost-user*  ) by
> vhost_virtqueue_set_addr().
> >>
> >> When the EVS get these address, it will update a*idx*  which will be
> >> treated as  vq's
> >> last_avail_idx when virtio-net stop ( pls see vhost_virtqueue_stop() ).
> >>
> >> So we get the following result after virtio-net stop:
> >>the desc/avail/used of the last 3 queues's vqs are all ZERO, but these
> vqs's
> >>last_avail_idx is NOT ZERO.
> >>
> >> At last, virtio_load() reports an error:
> >> '''
> >>if (!vdev->vq[i].vring.desc && vdev->vq[i].last_avail_idx) { // <--
> >> will be TRUE
> >>error_report("VQ %d address 0x0 "
> >>   "inconsistent with Host index 0x%x",
> >>   i, vdev->vq[i].last_avail_idx);
> >>  return -1;
> >> }
> >> '''
> >>
> >> BTW, the problem won't appear if use Linux guest, because the Linux
> virtio-net
> >> driver will set all 7 queues's desc/avail/used tables. And the problem
> >> won't appear
> >> if the VM use vhost-net, because vhost-net won't update*idx*  in
> SET_ADDR ioctl.
> 
> Just to make sure I understand here, I thought Windows guest + vhost_net
> hit this issue?
> 
No, Windows guest + vhost-user/DPDK.

BTW pls see virtio spec in :

"If VIRTIO_NET_F_MQ is negotiated, each of receiveq1. . .receiveqN that will be 
used SHOULD be populated
with receive buffers."

It is not mandatory that all queues must be initialized.

Thanks,
-Gonglei

Re: [Qemu-devel] [Question] why need to start all queues in vhost_net_start

[Jason Wang]

On 2017年11月16日 13:53, Longpeng (Mike) wrote:

On 2017/11/15 23:54, Longpeng(Mike) wrote:

2017-11-15 23:05 GMT+08:00 Jason Wang:

On 2017年11月15日 22:55, Longpeng(Mike) wrote:

Hi guys,

We got a BUG report from our testers yesterday, the testing scenario was
migrating a VM (Windows guest, *4 vcpus*, 4GB, vhost-user net: *7
queues*).

We found the cause reason, and we'll report the BUG or send a fix patch
to upstream if necessary( we haven't test the upstream yet, sorry... ).

Could you explain this a little bit more?


We want to know why the vhost_net_start() must start*total queues*  ( in
our
VM there're 7 queues ) but not*the queues that current used*  ( in our VM,
guest
only uses the first 4 queues because it's limited by the number of vcpus)
?

Looking forward to your help, thx:)

Since the codes have been there for years and works well for kernel
datapath. You should really explain what's wrong.


OK.:)

In our scenario,  the Windows's virtio-net driver only use the first 4
queues and it
*only set desc/avail/used table for the first 4 queues*, so in QEMU
the desc/avail/
used of the last 3 queues are ZERO,  but unfortunately...
'''
vhost_net_start
   for (i = 0; i < total_queues; i++)
 vhost_net_start_one
   vhost_dev_start
 vhost_virtqueue_start
'''
In vhost_virtqueue_start(), it will calculate the HVA of
desc/avail/used table, so for last
3 queues, it will use ZERO as the GPA to calculate the HVA, and then
send the results
to the user-mode backend ( we use*vhost-user*  ) by vhost_virtqueue_set_addr().

When the EVS get these address, it will update a*idx*  which will be
treated as  vq's
last_avail_idx when virtio-net stop ( pls see vhost_virtqueue_stop() ).

So we get the following result after virtio-net stop:
   the desc/avail/used of the last 3 queues's vqs are all ZERO, but these vqs's
   last_avail_idx is NOT ZERO.

At last, virtio_load() reports an error:
'''
   if (!vdev->vq[i].vring.desc && vdev->vq[i].last_avail_idx) { // <--
will be TRUE
   error_report("VQ %d address 0x0 "
  "inconsistent with Host index 0x%x",
  i, vdev->vq[i].last_avail_idx);
 return -1;
}
'''

BTW, the problem won't appear if use Linux guest, because the Linux virtio-net
driver will set all 7 queues's desc/avail/used tables. And the problem
won't appear
if the VM use vhost-net, because vhost-net won't update*idx*  in SET_ADDR ioctl.


Just to make sure I understand here, I thought Windows guest + vhost_net 
hit this issue?


Thanks



Sorry for my pool English, Maybe I could describe the problem in Chinese for you
in private if necessary.



Thanks

-- Regards, Longpeng(Mike)

Re: [Qemu-devel] [Question] why need to start all queues in vhost_net_start

[Yan Vugenfirer]

Hi Jason,

Windows driver will initialise only the amount of queue based on the amount of 
available vCPUs. So if there will be more queues in the device than we have 
vCPUs on the guest, the driver will not initialise “excessive” queues. This is 
tied to the way RSS on Windows should be implemented.
Exactly as in described scenario (7 queues, but only 4 vCPUs).

Best regards,
Yan.

> On 16 Nov 2017, at 07:53, Longpeng (Mike)  wrote:
> 
> Hi Jason & Michael,
> 
> Do you have any idea about this problem ?
> 
> -- 
> Regards,
> Longpeng(Mike)
> 
> On 2017/11/15 23:54, Longpeng(Mike) wrote:
> 
>> 2017-11-15 23:05 GMT+08:00 Jason Wang :
>>> 
>>> 
>>> On 2017年11月15日 22:55, Longpeng(Mike) wrote:
 
 Hi guys,
 
 We got a BUG report from our testers yesterday, the testing scenario was
 migrating a VM (Windows guest, *4 vcpus*, 4GB, vhost-user net: *7
 queues*).
 
 We found the cause reason, and we'll report the BUG or send a fix patch
 to upstream if necessary( we haven't test the upstream yet, sorry... ).
>>> 
>>> 
>>> Could you explain this a little bit more?
>>> 
 
 We want to know why the vhost_net_start() must start *total queues* ( in
 our
 VM there're 7 queues ) but not *the queues that current used* ( in our VM,
 guest
 only uses the first 4 queues because it's limited by the number of vcpus)
 ?
 
 Looking forward to your help, thx :)
>>> 
>>> 
>>> Since the codes have been there for years and works well for kernel
>>> datapath. You should really explain what's wrong.
>>> 
>> 
>> OK. :)
>> 
>> In our scenario,  the Windows's virtio-net driver only use the first 4
>> queues and it
>> *only set desc/avail/used table for the first 4 queues*, so in QEMU
>> the desc/avail/
>> used of the last 3 queues are ZERO,  but unfortunately...
>> '''
>> vhost_net_start
>>  for (i = 0; i < total_queues; i++)
>>vhost_net_start_one
>>  vhost_dev_start
>>vhost_virtqueue_start
>> '''
>> In vhost_virtqueue_start(), it will calculate the HVA of
>> desc/avail/used table, so for last
>> 3 queues, it will use ZERO as the GPA to calculate the HVA, and then
>> send the results
>> to the user-mode backend ( we use *vhost-user* ) by 
>> vhost_virtqueue_set_addr().
>> 
>> When the EVS get these address, it will update a *idx* which will be
>> treated as  vq's
>> last_avail_idx when virtio-net stop ( pls see vhost_virtqueue_stop() ).
>> 
>> So we get the following result after virtio-net stop:
>>  the desc/avail/used of the last 3 queues's vqs are all ZERO, but these vqs's
>>  last_avail_idx is NOT ZERO.
>> 
>> At last, virtio_load() reports an error:
>> '''
>>  if (!vdev->vq[i].vring.desc && vdev->vq[i].last_avail_idx) { // <--
>> will be TRUE
>>  error_report("VQ %d address 0x0 "
>> "inconsistent with Host index 0x%x",
>> i, vdev->vq[i].last_avail_idx);
>>return -1;
>>   }
>> '''
>> 
>> BTW, the problem won't appear if use Linux guest, because the Linux 
>> virtio-net
>> driver will set all 7 queues's desc/avail/used tables. And the problem
>> won't appear
>> if the VM use vhost-net, because vhost-net won't update *idx* in SET_ADDR 
>> ioctl.
>> 
>> Sorry for my pool English, Maybe I could describe the problem in Chinese for 
>> you
>> in private if necessary.
>> 
>> 
>>> Thanks
>> 
>> 
> 
> 
> -- 
> Regards,
> Longpeng(Mike)

Re: [Qemu-devel] [Question] why need to start all queues in vhost_net_start

[Longpeng (Mike)]

Hi Jason & Michael,

Do you have any idea about this problem ?

-- 
Regards,
Longpeng(Mike)

On 2017/11/15 23:54, Longpeng(Mike) wrote:

> 2017-11-15 23:05 GMT+08:00 Jason Wang :
>>
>>
>> On 2017年11月15日 22:55, Longpeng(Mike) wrote:
>>>
>>> Hi guys,
>>>
>>> We got a BUG report from our testers yesterday, the testing scenario was
>>> migrating a VM (Windows guest, *4 vcpus*, 4GB, vhost-user net: *7
>>> queues*).
>>>
>>> We found the cause reason, and we'll report the BUG or send a fix patch
>>> to upstream if necessary( we haven't test the upstream yet, sorry... ).
>>
>>
>> Could you explain this a little bit more?
>>
>>>
>>> We want to know why the vhost_net_start() must start *total queues* ( in
>>> our
>>> VM there're 7 queues ) but not *the queues that current used* ( in our VM,
>>> guest
>>> only uses the first 4 queues because it's limited by the number of vcpus)
>>> ?
>>>
>>> Looking forward to your help, thx :)
>>
>>
>> Since the codes have been there for years and works well for kernel
>> datapath. You should really explain what's wrong.
>>
> 
> OK. :)
> 
> In our scenario,  the Windows's virtio-net driver only use the first 4
> queues and it
> *only set desc/avail/used table for the first 4 queues*, so in QEMU
> the desc/avail/
> used of the last 3 queues are ZERO,  but unfortunately...
> '''
> vhost_net_start
>   for (i = 0; i < total_queues; i++)
> vhost_net_start_one
>   vhost_dev_start
> vhost_virtqueue_start
> '''
> In vhost_virtqueue_start(), it will calculate the HVA of
> desc/avail/used table, so for last
> 3 queues, it will use ZERO as the GPA to calculate the HVA, and then
> send the results
> to the user-mode backend ( we use *vhost-user* ) by 
> vhost_virtqueue_set_addr().
> 
> When the EVS get these address, it will update a *idx* which will be
> treated as  vq's
> last_avail_idx when virtio-net stop ( pls see vhost_virtqueue_stop() ).
> 
> So we get the following result after virtio-net stop:
>   the desc/avail/used of the last 3 queues's vqs are all ZERO, but these vqs's
>   last_avail_idx is NOT ZERO.
> 
> At last, virtio_load() reports an error:
> '''
>   if (!vdev->vq[i].vring.desc && vdev->vq[i].last_avail_idx) { // <--
> will be TRUE
>   error_report("VQ %d address 0x0 "
>  "inconsistent with Host index 0x%x",
>  i, vdev->vq[i].last_avail_idx);
> return -1;
>}
> '''
> 
> BTW, the problem won't appear if use Linux guest, because the Linux virtio-net
> driver will set all 7 queues's desc/avail/used tables. And the problem
> won't appear
> if the VM use vhost-net, because vhost-net won't update *idx* in SET_ADDR 
> ioctl.
> 
> Sorry for my pool English, Maybe I could describe the problem in Chinese for 
> you
> in private if necessary.
> 
> 
>> Thanks
> 
> 


-- 
Regards,
Longpeng(Mike)

Re: [Qemu-devel] [Question] why need to start all queues in vhost_net_start

[Longpeng(Mike)]

2017-11-15 23:05 GMT+08:00 Jason Wang :
>
>
> On 2017年11月15日 22:55, Longpeng(Mike) wrote:
>>
>> Hi guys,
>>
>> We got a BUG report from our testers yesterday, the testing scenario was
>> migrating a VM (Windows guest, *4 vcpus*, 4GB, vhost-user net: *7
>> queues*).
>>
>> We found the cause reason, and we'll report the BUG or send a fix patch
>> to upstream if necessary( we haven't test the upstream yet, sorry... ).
>
>
> Could you explain this a little bit more?
>
>>
>> We want to know why the vhost_net_start() must start *total queues* ( in
>> our
>> VM there're 7 queues ) but not *the queues that current used* ( in our VM,
>> guest
>> only uses the first 4 queues because it's limited by the number of vcpus)
>> ?
>>
>> Looking forward to your help, thx :)
>
>
> Since the codes have been there for years and works well for kernel
> datapath. You should really explain what's wrong.
>

OK. :)

In our scenario,  the Windows's virtio-net driver only use the first 4
queues and it
*only set desc/avail/used table for the first 4 queues*, so in QEMU
the desc/avail/
used of the last 3 queues are ZERO,  but unfortunately...
'''
vhost_net_start
  for (i = 0; i < total_queues; i++)
vhost_net_start_one
  vhost_dev_start
vhost_virtqueue_start
'''
In vhost_virtqueue_start(), it will calculate the HVA of
desc/avail/used table, so for last
3 queues, it will use ZERO as the GPA to calculate the HVA, and then
send the results
to the user-mode backend ( we use *vhost-user* ) by vhost_virtqueue_set_addr().

When the EVS get these address, it will update a *idx* which will be
treated as  vq's
last_avail_idx when virtio-net stop ( pls see vhost_virtqueue_stop() ).

So we get the following result after virtio-net stop:
  the desc/avail/used of the last 3 queues's vqs are all ZERO, but these vqs's
  last_avail_idx is NOT ZERO.

At last, virtio_load() reports an error:
'''
  if (!vdev->vq[i].vring.desc && vdev->vq[i].last_avail_idx) { // <--
will be TRUE
  error_report("VQ %d address 0x0 "
 "inconsistent with Host index 0x%x",
 i, vdev->vq[i].last_avail_idx);
return -1;
   }
'''

BTW, the problem won't appear if use Linux guest, because the Linux virtio-net
driver will set all 7 queues's desc/avail/used tables. And the problem
won't appear
if the VM use vhost-net, because vhost-net won't update *idx* in SET_ADDR ioctl.

Sorry for my pool English, Maybe I could describe the problem in Chinese for you
in private if necessary.


> Thanks


-- 
Regards,
Longpeng

Re: [Qemu-devel] [Question] why need to start all queues in vhost_net_start

[Jason Wang]

On 2017年11月15日 22:55, Longpeng(Mike) wrote:

Hi guys,

We got a BUG report from our testers yesterday, the testing scenario was
migrating a VM (Windows guest, *4 vcpus*, 4GB, vhost-user net: *7 queues*).

We found the cause reason, and we'll report the BUG or send a fix patch
to upstream if necessary( we haven't test the upstream yet, sorry... ).


Could you explain this a little bit more?



We want to know why the vhost_net_start() must start *total queues* ( in our
VM there're 7 queues ) but not *the queues that current used* ( in our VM, guest
only uses the first 4 queues because it's limited by the number of vcpus) ?

Looking forward to your help, thx :)


Since the codes have been there for years and works well for kernel 
datapath. You should really explain what's wrong.


Thanks