| Message ID | 1361439957-16507-1-git-send-email-jasowang@redhat.com |
|---|---|
| State | New |
| Headers | show |
Jason Wang <jasowang@redhat.com> writes: > Edivaldo reports a problem that the array of NetClientState in NICState is too > large - MAX_QUEUE_NUM(1024) which will wastes memory even if multiqueue is not > used. > > Instead of static arrays, solving this issue by allocating the queues on demand > for both the NetClientState array in NICState and VirtIONetQueue array in > VirtIONet. > > Tested by myself, with single virtio-net-pci device. The memory allocation is > almost the same as when multiqueue is not merged. > > Cc: Edivaldo de Araujo Pereira <edivaldoapereira@yahoo.com.br> > Cc: qemu-stable@nongnu.org > Signed-off-by: Jason Wang <jasowang@redhat.com> > --- > hw/virtio-net.c | 6 ++++-- > include/net/net.h | 2 +- > net/net.c | 19 +++++++++---------- > 3 files changed, 14 insertions(+), 13 deletions(-) > > diff --git a/hw/virtio-net.c b/hw/virtio-net.c > index 573c669..70ab641 100644 > --- a/hw/virtio-net.c > +++ b/hw/virtio-net.c > @@ -44,7 +44,7 @@ typedef struct VirtIONet > VirtIODevice vdev; > uint8_t mac[ETH_ALEN]; > uint16_t status; > - VirtIONetQueue vqs[MAX_QUEUE_NUM]; > + VirtIONetQueue *vqs; > VirtQueue *ctrl_vq; > NICState *nic; > uint32_t tx_timeout; > @@ -1326,8 +1326,9 @@ VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf, > n->vdev.set_status = virtio_net_set_status; > n->vdev.guest_notifier_mask = virtio_net_guest_notifier_mask; > n->vdev.guest_notifier_pending = virtio_net_guest_notifier_pending; > + n->max_queues = MAX(conf->queues, 1); I think you mean MIN here. > + n->vqs = g_malloc0(sizeof(VirtIONetQueue) * n->max_queues); > n->vqs[0].rx_vq = virtio_add_queue(&n->vdev, 256, virtio_net_handle_rx); > - n->max_queues = conf->queues; > n->curr_queues = 1; > n->vqs[0].n = n; > n->tx_timeout = net->txtimer; > @@ -1397,6 +1398,7 @@ void virtio_net_exit(VirtIODevice *vdev) > > g_free(n->mac_table.macs); > g_free(n->vlans); > + g_free(n->vqs); > > for (i = 0; i < n->max_queues; i++) { > VirtIONetQueue *q = &n->vqs[i]; > diff --git a/include/net/net.h b/include/net/net.h > index 43a045e..cb049a1 100644 > --- a/include/net/net.h > +++ b/include/net/net.h > @@ -72,7 +72,7 @@ struct NetClientState { > }; > > typedef struct NICState { > - NetClientState ncs[MAX_QUEUE_NUM]; > + NetClientState *ncs; > NICConf *conf; > void *opaque; > bool peer_deleted; > diff --git a/net/net.c b/net/net.c > index be03a8d..6262ed0 100644 > --- a/net/net.c > +++ b/net/net.c > @@ -235,23 +235,20 @@ NICState *qemu_new_nic(NetClientInfo *info, > const char *name, > void *opaque) > { > - NetClientState *nc; > NetClientState **peers = conf->peers.ncs; > NICState *nic; > - int i; > + int i, queues = MAX(1, conf->queues); And here. This patch is what had created problems for me. Regards, Anthony Liguori > > assert(info->type == NET_CLIENT_OPTIONS_KIND_NIC); > assert(info->size >= sizeof(NICState)); > > - nc = qemu_new_net_client(info, peers[0], model, name); > - nc->queue_index = 0; > - > - nic = qemu_get_nic(nc); > + nic = g_malloc0(info->size + sizeof(NetClientState) * queues); > + nic->ncs = (void *)nic + info->size; > nic->conf = conf; > nic->opaque = opaque; > > - for (i = 1; i < conf->queues; i++) { > - qemu_net_client_setup(&nic->ncs[i], info, peers[i], model, nc->name, > + for (i = 0; i < queues; i++) { > + qemu_net_client_setup(&nic->ncs[i], info, peers[i], model, name, > NULL); > nic->ncs[i].queue_index = i; > } > @@ -261,7 +258,7 @@ NICState *qemu_new_nic(NetClientInfo *info, > > NetClientState *qemu_get_subqueue(NICState *nic, int queue_index) > { > - return &nic->ncs[queue_index]; > + return nic->ncs + queue_index; > } > > NetClientState *qemu_get_queue(NICState *nic) > @@ -273,7 +270,7 @@ NICState *qemu_get_nic(NetClientState *nc) > { > NetClientState *nc0 = nc - nc->queue_index; > > - return DO_UPCAST(NICState, ncs[0], nc0); > + return (NICState *)((void *)nc0 - nc->info->size); > } > > void *qemu_get_nic_opaque(NetClientState *nc) > @@ -368,6 +365,8 @@ void qemu_del_nic(NICState *nic) > qemu_cleanup_net_client(nc); > qemu_free_net_client(nc); > } > + > + g_free(nic); > } > > void qemu_foreach_nic(qemu_nic_foreach func, void *opaque) > -- > 1.7.1
On 02/22/2013 06:32 AM, Anthony Liguori wrote: > Jason Wang <jasowang@redhat.com> writes: > >> Edivaldo reports a problem that the array of NetClientState in NICState is too >> large - MAX_QUEUE_NUM(1024) which will wastes memory even if multiqueue is not >> used. >> >> Instead of static arrays, solving this issue by allocating the queues on demand >> for both the NetClientState array in NICState and VirtIONetQueue array in >> VirtIONet. >> >> Tested by myself, with single virtio-net-pci device. The memory allocation is >> almost the same as when multiqueue is not merged. >> >> Cc: Edivaldo de Araujo Pereira <edivaldoapereira@yahoo.com.br> >> Cc: qemu-stable@nongnu.org >> Signed-off-by: Jason Wang <jasowang@redhat.com> >> --- >> hw/virtio-net.c | 6 ++++-- >> include/net/net.h | 2 +- >> net/net.c | 19 +++++++++---------- >> 3 files changed, 14 insertions(+), 13 deletions(-) >> >> diff --git a/hw/virtio-net.c b/hw/virtio-net.c >> index 573c669..70ab641 100644 >> --- a/hw/virtio-net.c >> +++ b/hw/virtio-net.c >> @@ -44,7 +44,7 @@ typedef struct VirtIONet >> VirtIODevice vdev; >> uint8_t mac[ETH_ALEN]; >> uint16_t status; >> - VirtIONetQueue vqs[MAX_QUEUE_NUM]; >> + VirtIONetQueue *vqs; >> VirtQueue *ctrl_vq; >> NICState *nic; >> uint32_t tx_timeout; >> @@ -1326,8 +1326,9 @@ VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf, >> n->vdev.set_status = virtio_net_set_status; >> n->vdev.guest_notifier_mask = virtio_net_guest_notifier_mask; >> n->vdev.guest_notifier_pending = virtio_net_guest_notifier_pending; >> + n->max_queues = MAX(conf->queues, 1); > I think you mean MIN here. Then the at most 1 queue will be created. MAX is used to create at least one queue when conf->queues is zero which means the nic were not created with netdev. >> + n->vqs = g_malloc0(sizeof(VirtIONetQueue) * n->max_queues); >> n->vqs[0].rx_vq = virtio_add_queue(&n->vdev, 256, virtio_net_handle_rx); >> - n->max_queues = conf->queues; >> n->curr_queues = 1; >> n->vqs[0].n = n; >> n->tx_timeout = net->txtimer; >> @@ -1397,6 +1398,7 @@ void virtio_net_exit(VirtIODevice *vdev) >> >> g_free(n->mac_table.macs); >> g_free(n->vlans); >> + g_free(n->vqs); >> >> for (i = 0; i < n->max_queues; i++) { >> VirtIONetQueue *q = &n->vqs[i]; >> diff --git a/include/net/net.h b/include/net/net.h >> index 43a045e..cb049a1 100644 >> --- a/include/net/net.h >> +++ b/include/net/net.h >> @@ -72,7 +72,7 @@ struct NetClientState { >> }; >> >> typedef struct NICState { >> - NetClientState ncs[MAX_QUEUE_NUM]; >> + NetClientState *ncs; >> NICConf *conf; >> void *opaque; >> bool peer_deleted; >> diff --git a/net/net.c b/net/net.c >> index be03a8d..6262ed0 100644 >> --- a/net/net.c >> +++ b/net/net.c >> @@ -235,23 +235,20 @@ NICState *qemu_new_nic(NetClientInfo *info, >> const char *name, >> void *opaque) >> { >> - NetClientState *nc; >> NetClientState **peers = conf->peers.ncs; >> NICState *nic; >> - int i; >> + int i, queues = MAX(1, conf->queues); > And here. This patch is what had created problems for me. Same as above, so I think the code is ok or anything I miss? > > Regards, > > Anthony Liguori > >> >> assert(info->type == NET_CLIENT_OPTIONS_KIND_NIC); >> assert(info->size >= sizeof(NICState)); >> >> - nc = qemu_new_net_client(info, peers[0], model, name); >> - nc->queue_index = 0; >> - >> - nic = qemu_get_nic(nc); >> + nic = g_malloc0(info->size + sizeof(NetClientState) * queues); >> + nic->ncs = (void *)nic + info->size; >> nic->conf = conf; >> nic->opaque = opaque; >> >> - for (i = 1; i < conf->queues; i++) { >> - qemu_net_client_setup(&nic->ncs[i], info, peers[i], model, nc->name, >> + for (i = 0; i < queues; i++) { >> + qemu_net_client_setup(&nic->ncs[i], info, peers[i], model, name, >> NULL); >> nic->ncs[i].queue_index = i; >> } >> @@ -261,7 +258,7 @@ NICState *qemu_new_nic(NetClientInfo *info, >> >> NetClientState *qemu_get_subqueue(NICState *nic, int queue_index) >> { >> - return &nic->ncs[queue_index]; >> + return nic->ncs + queue_index; >> } >> >> NetClientState *qemu_get_queue(NICState *nic) >> @@ -273,7 +270,7 @@ NICState *qemu_get_nic(NetClientState *nc) >> { >> NetClientState *nc0 = nc - nc->queue_index; >> >> - return DO_UPCAST(NICState, ncs[0], nc0); >> + return (NICState *)((void *)nc0 - nc->info->size); >> } >> >> void *qemu_get_nic_opaque(NetClientState *nc) >> @@ -368,6 +365,8 @@ void qemu_del_nic(NICState *nic) >> qemu_cleanup_net_client(nc); >> qemu_free_net_client(nc); >> } >> + >> + g_free(nic); >> } >> >> void qemu_foreach_nic(qemu_nic_foreach func, void *opaque) >> -- >> 1.7.1 >
On Thu, Feb 21, 2013 at 05:45:57PM +0800, Jason Wang wrote: > @@ -1397,6 +1398,7 @@ void virtio_net_exit(VirtIODevice *vdev) > > g_free(n->mac_table.macs); > g_free(n->vlans); > + g_free(n->vqs); > > for (i = 0; i < n->max_queues; i++) { > VirtIONetQueue *q = &n->vqs[i]; Use after free!
Stefan Hajnoczi <stefanha@redhat.com> writes: > On Thu, Feb 21, 2013 at 05:45:57PM +0800, Jason Wang wrote: >> @@ -1397,6 +1398,7 @@ void virtio_net_exit(VirtIODevice *vdev) >> >> g_free(n->mac_table.macs); >> g_free(n->vlans); >> + g_free(n->vqs); >> >> for (i = 0; i < n->max_queues; i++) { >> VirtIONetQueue *q = &n->vqs[i]; > > Use after free! Ah, this was it :-) Regards, Anthony Liguori
On 02/22/2013 09:18 PM, Anthony Liguori wrote: > Stefan Hajnoczi <stefanha@redhat.com> writes: > >> On Thu, Feb 21, 2013 at 05:45:57PM +0800, Jason Wang wrote: >>> @@ -1397,6 +1398,7 @@ void virtio_net_exit(VirtIODevice *vdev) >>> >>> g_free(n->mac_table.macs); >>> g_free(n->vlans); >>> + g_free(n->vqs); >>> >>> for (i = 0; i < n->max_queues; i++) { >>> VirtIONetQueue *q = &n->vqs[i]; >> Use after free! > Ah, this was it :-) > > Regards, > > Anthony Liguori > Will post V2. Thanks
diff --git a/hw/virtio-net.c b/hw/virtio-net.c index 573c669..70ab641 100644 --- a/hw/virtio-net.c +++ b/hw/virtio-net.c @@ -44,7 +44,7 @@ typedef struct VirtIONet VirtIODevice vdev; uint8_t mac[ETH_ALEN]; uint16_t status; - VirtIONetQueue vqs[MAX_QUEUE_NUM]; + VirtIONetQueue *vqs; VirtQueue *ctrl_vq; NICState *nic; uint32_t tx_timeout; @@ -1326,8 +1326,9 @@ VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf, n->vdev.set_status = virtio_net_set_status; n->vdev.guest_notifier_mask = virtio_net_guest_notifier_mask; n->vdev.guest_notifier_pending = virtio_net_guest_notifier_pending; + n->max_queues = MAX(conf->queues, 1); + n->vqs = g_malloc0(sizeof(VirtIONetQueue) * n->max_queues); n->vqs[0].rx_vq = virtio_add_queue(&n->vdev, 256, virtio_net_handle_rx); - n->max_queues = conf->queues; n->curr_queues = 1; n->vqs[0].n = n; n->tx_timeout = net->txtimer; @@ -1397,6 +1398,7 @@ void virtio_net_exit(VirtIODevice *vdev) g_free(n->mac_table.macs); g_free(n->vlans); + g_free(n->vqs); for (i = 0; i < n->max_queues; i++) { VirtIONetQueue *q = &n->vqs[i]; diff --git a/include/net/net.h b/include/net/net.h index 43a045e..cb049a1 100644 --- a/include/net/net.h +++ b/include/net/net.h @@ -72,7 +72,7 @@ struct NetClientState { }; typedef struct NICState { - NetClientState ncs[MAX_QUEUE_NUM]; + NetClientState *ncs; NICConf *conf; void *opaque; bool peer_deleted; diff --git a/net/net.c b/net/net.c index be03a8d..6262ed0 100644 --- a/net/net.c +++ b/net/net.c @@ -235,23 +235,20 @@ NICState *qemu_new_nic(NetClientInfo *info, const char *name, void *opaque) { - NetClientState *nc; NetClientState **peers = conf->peers.ncs; NICState *nic; - int i; + int i, queues = MAX(1, conf->queues); assert(info->type == NET_CLIENT_OPTIONS_KIND_NIC); assert(info->size >= sizeof(NICState)); - nc = qemu_new_net_client(info, peers[0], model, name); - nc->queue_index = 0; - - nic = qemu_get_nic(nc); + nic = g_malloc0(info->size + sizeof(NetClientState) * queues); + nic->ncs = (void *)nic + info->size; nic->conf = conf; nic->opaque = opaque; - for (i = 1; i < conf->queues; i++) { - qemu_net_client_setup(&nic->ncs[i], info, peers[i], model, nc->name, + for (i = 0; i < queues; i++) { + qemu_net_client_setup(&nic->ncs[i], info, peers[i], model, name, NULL); nic->ncs[i].queue_index = i; } @@ -261,7 +258,7 @@ NICState *qemu_new_nic(NetClientInfo *info, NetClientState *qemu_get_subqueue(NICState *nic, int queue_index) { - return &nic->ncs[queue_index]; + return nic->ncs + queue_index; } NetClientState *qemu_get_queue(NICState *nic) @@ -273,7 +270,7 @@ NICState *qemu_get_nic(NetClientState *nc) { NetClientState *nc0 = nc - nc->queue_index; - return DO_UPCAST(NICState, ncs[0], nc0); + return (NICState *)((void *)nc0 - nc->info->size); } void *qemu_get_nic_opaque(NetClientState *nc) @@ -368,6 +365,8 @@ void qemu_del_nic(NICState *nic) qemu_cleanup_net_client(nc); qemu_free_net_client(nc); } + + g_free(nic); } void qemu_foreach_nic(qemu_nic_foreach func, void *opaque)
Edivaldo reports a problem that the array of NetClientState in NICState is too large - MAX_QUEUE_NUM(1024) which will wastes memory even if multiqueue is not used. Instead of static arrays, solving this issue by allocating the queues on demand for both the NetClientState array in NICState and VirtIONetQueue array in VirtIONet. Tested by myself, with single virtio-net-pci device. The memory allocation is almost the same as when multiqueue is not merged. Cc: Edivaldo de Araujo Pereira <edivaldoapereira@yahoo.com.br> Cc: qemu-stable@nongnu.org Signed-off-by: Jason Wang <jasowang@redhat.com> --- hw/virtio-net.c | 6 ++++-- include/net/net.h | 2 +- net/net.c | 19 +++++++++---------- 3 files changed, 14 insertions(+), 13 deletions(-)