Re: [Qemu-devel] [PATCH] virtio-net: remove virtio queues if the guest doesn't support multiqueue

[Wen Congyang]

On 07/15/2015 03:41 PM, Jason Wang wrote:
> 
> 
> On 07/15/2015 03:13 PM, Wen Congyang wrote:
>> On 07/15/2015 02:59 PM, Jason Wang wrote:
>>>
>>> On 07/15/2015 02:23 PM, Wen Congyang wrote:
>>>> commit da51a335 adds all queues in .realize(). But if the
>>>> guest doesn't support multiqueue, we forget to remove them. And
>>>> we cannot handle the ctrl vq corretly. The guest will hang.
>>>>
>>>> Signed-off-by: Wen Congyang <address@hidden>
>>> Thanks for the patch, some questions, see below.
>>>
>>>> ---
>>>>  hw/net/virtio-net.c | 69 
>>>> +++++++++++++++++++++++++++++++++++++++++------------
>>>>  1 file changed, 54 insertions(+), 15 deletions(-)
>>>>
>>>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>>>> index e3c2db3..658806a 100644
>>>> --- a/hw/net/virtio-net.c
>>>> +++ b/hw/net/virtio-net.c
>>>> @@ -1306,9 +1306,62 @@ static void virtio_net_tx_bh(void *opaque)
>>>>      }
>>>>  }
>>>>  
>>>> +static void virtio_net_add_queue(VirtIONet *n, int index)
>>>> +{
>>>> +    VirtIODevice *vdev = VIRTIO_DEVICE(n);
>>>> +
>>>> +    n->vqs[index].rx_vq = virtio_add_queue(vdev, 256, 
>>>> virtio_net_handle_rx);
>>>> +    if (n->net_conf.tx && !strcmp(n->net_conf.tx, "timer")) {
>>>> +        n->vqs[index].tx_vq =
>>>> +            virtio_add_queue(vdev, 256, virtio_net_handle_tx_timer);
>>>> +        n->vqs[index].tx_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
>>>> +                                              virtio_net_tx_timer,
>>>> +                                              &n->vqs[index]);
>>>> +    } else {
>>>> +        n->vqs[index].tx_vq =
>>>> +            virtio_add_queue(vdev, 256, virtio_net_handle_tx_bh);
>>>> +        n->vqs[index].tx_bh = qemu_bh_new(virtio_net_tx_bh, 
>>>> &n->vqs[index]);
>>>> +    }
>>>> +
>>>> +    n->vqs[index].tx_waiting = 0;
>>>> +    n->vqs[index].n = n;
>>>> +}
>>>> +
>>>> +static void virtio_net_change_num_queues(VirtIONet *n, int new_max_queues)
>>>> +{
>>>> +    VirtIODevice *vdev = VIRTIO_DEVICE(n);
>>>> +    int old_num_queues = virtio_get_num_queues(vdev);
>>>> +    int new_num_queues = new_max_queues * 2 + 1;
>>>> +    int i;
>>>> +
>>>> +    assert(old_num_queues >= 3);
>>>> +    assert(old_num_queues % 2 == 1);
>>>> +
>>>> +    if (old_num_queues == new_num_queues) {
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    /* Remove ctrl_vq first */
>>>> +    virtio_del_queue(vdev, old_num_queues - 1);
>>> Why ctrl vq must be first to be deleted?
>> It just make the codes clean. We always need to remove and add ctrl vq if
>> old_num_queues != new_num_queues. We only enter one of the two loops.
>>
>>>> +
>>>> +    for (i = new_num_queues - 1; i < old_num_queues - 1; i++) {
>>>> +        virtio_del_queue(vdev, i);
>>> Do we need to delete bh and timer also here?
>> I think we should delete it too. Before commit da51a335, we only call
>> virtio_del_queue(), so I forgot to do it.
>>
>> Thanks
>> Wen Congyang
> 
> I believe we need this patch for 2.4. So could you please do this in V2?

OK, I am updating it now. Just wait for a moment.

Thanks
Wen Congyang

> 
> Thanks
>>
>>>> +    }
>>>> +
>>>> +    for (i = old_num_queues - 1; i < new_num_queues - 1; i += 2) {
>>>> +        virtio_net_add_queue(n, i / 2);
>>>> +    }
>>>> +
>>>> +    /* add ctrl_vq last */
>>>> +    n->ctrl_vq = virtio_add_queue(vdev, 64, virtio_net_handle_ctrl);
>>>> +}
>>>> +
>>>>  static void virtio_net_set_multiqueue(VirtIONet *n, int multiqueue)
>>>>  {
>>>> +    int max = multiqueue ? n->max_queues : 1;
>>>> +
>>>>      n->multiqueue = multiqueue;
>>>> +    virtio_net_change_num_queues(n, max);
>>>>  
>>>>      virtio_net_set_queues(n);
>>>>  }
>>>> @@ -1583,21 +1636,7 @@ static void virtio_net_device_realize(DeviceState 
>>>> *dev, Error **errp)
>>>>      }
>>>>  
>>>>      for (i = 0; i < n->max_queues; i++) {
>>>> -        n->vqs[i].rx_vq = virtio_add_queue(vdev, 256, 
>>>> virtio_net_handle_rx);
>>>> -        if (n->net_conf.tx && !strcmp(n->net_conf.tx, "timer")) {
>>>> -            n->vqs[i].tx_vq =
>>>> -                virtio_add_queue(vdev, 256, virtio_net_handle_tx_timer);
>>>> -            n->vqs[i].tx_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
>>>> -                                              virtio_net_tx_timer,
>>>> -                                              &n->vqs[i]);
>>>> -        } else {
>>>> -            n->vqs[i].tx_vq =
>>>> -                virtio_add_queue(vdev, 256, virtio_net_handle_tx_bh);
>>>> -            n->vqs[i].tx_bh = qemu_bh_new(virtio_net_tx_bh, &n->vqs[i]);
>>>> -        }
>>>> -
>>>> -        n->vqs[i].tx_waiting = 0;
>>>> -        n->vqs[i].n = n;
>>>> +        virtio_net_add_queue(n, i);
>>>>      }
>>>>  
>>>>      n->ctrl_vq = virtio_add_queue(vdev, 64, virtio_net_handle_ctrl);
>>> .
>>>
>>
> 
> .
>