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 <we...@cn.fujitsu.com> >> 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? 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); >> . >> >