| Message ID | 55A5FC6E.1030405@cn.fujitsu.com |
|---|---|
| State | New |
| Headers | show |
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 <wency@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? > + > + 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? > + } > + > + 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);
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 <wency@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 > >> + } >> + >> + 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); > > . >
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 <wency@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); >> . >> >
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 <wency@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? 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); >>> . >>> >> > > . >
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); + + for (i = new_num_queues - 1; i < old_num_queues - 1; i++) { + virtio_del_queue(vdev, i); + } + + 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);
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 <wency@cn.fujitsu.com> --- hw/net/virtio-net.c | 69 +++++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 54 insertions(+), 15 deletions(-)