| Message ID | 20190617154105.32323-5-anthony.perard@citrix.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix build of Xen support + cleanup | expand |
> -----Original Message----- > From: Anthony PERARD [mailto:anthony.perard@citrix.com] > Sent: 17 June 2019 16:41 > To: qemu-devel@nongnu.org > Cc: xen-devel@lists.xenproject.org; Anthony Perard <anthony.perard@citrix.com>; Stefano Stabellini > <sstabellini@kernel.org>; Paul Durrant <Paul.Durrant@citrix.com> > Subject: [PATCH 4/4] xen: Avoid VLA > > Avoid using a variable length array. > > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> > --- > > Notes: > Was suggested by Peter here: > <CAFEAcA88+A2oCkQnxKDEdpmfCZSmPzWMBg01wDDV68bMZoY5Jg@mail.gmail.com> > "should we try to stop using variable length arrays?" > > hw/i386/xen/xen-hvm.c | 11 ++++++++--- > 1 file changed, 8 insertions(+), 3 deletions(-) > > diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c > index 725f9c2278..10d73b55b4 100644 > --- a/hw/i386/xen/xen-hvm.c > +++ b/hw/i386/xen/xen-hvm.c > @@ -615,7 +615,8 @@ static void xen_sync_dirty_bitmap(XenIOState *state, > { > hwaddr npages = size >> TARGET_PAGE_BITS; > const int width = sizeof(unsigned long) * 8; > - unsigned long bitmap[DIV_ROUND_UP(npages, width)]; > + unsigned long *bitmap = NULL; > + size_t bitmap_size = DIV_ROUND_UP(npages, width); > int rc, i, j; > const XenPhysmap *physmap = NULL; > > @@ -632,6 +633,8 @@ static void xen_sync_dirty_bitmap(XenIOState *state, > return; > } > > + bitmap = g_new0(unsigned long, bitmap_size); > + How hot is this function? It looks (unsurprisingly) like the section size determines the map size so I wonder whether it can instead be allocated once when the section is added? Paul > rc = xen_track_dirty_vram(xen_domid, start_addr >> TARGET_PAGE_BITS, > npages, bitmap); > if (rc < 0) { > @@ -644,10 +647,10 @@ static void xen_sync_dirty_bitmap(XenIOState *state, > ", 0x" TARGET_FMT_plx "): %s\n", > start_addr, start_addr + size, strerror(errno)); > } > - return; > + goto out; > } > > - for (i = 0; i < ARRAY_SIZE(bitmap); i++) { > + for (i = 0; i < bitmap_size; i++) { > unsigned long map = bitmap[i]; > while (map != 0) { > j = ctzl(map); > @@ -657,6 +660,8 @@ static void xen_sync_dirty_bitmap(XenIOState *state, > TARGET_PAGE_SIZE); > }; > } > +out: > + g_free(bitmap); > } > > static void xen_log_start(MemoryListener *listener, > -- > Anthony PERARD
On Mon, Jun 17, 2019 at 05:39:09PM +0100, Paul Durrant wrote: > > @@ -632,6 +633,8 @@ static void xen_sync_dirty_bitmap(XenIOState *state, > > return; > > } > > > > + bitmap = g_new0(unsigned long, bitmap_size); > > + > > How hot is this function? It looks (unsurprisingly) like the section > size determines the map size so I wonder whether it can instead be > allocated once when the section is added? I think we can store the bitmap buffer into the `state' where `log_for_dirtybit' is already present, and free the bitmap when `log_for_dirtybit' is cleared. Thanks,
> -----Original Message----- > From: Anthony PERARD [mailto:anthony.perard@citrix.com] > Sent: 17 June 2019 18:37 > To: Paul Durrant <Paul.Durrant@citrix.com> > Cc: qemu-devel@nongnu.org; xen-devel@lists.xenproject.org; Stefano Stabellini <sstabellini@kernel.org> > Subject: Re: [PATCH 4/4] xen: Avoid VLA > > On Mon, Jun 17, 2019 at 05:39:09PM +0100, Paul Durrant wrote: > > > @@ -632,6 +633,8 @@ static void xen_sync_dirty_bitmap(XenIOState *state, > > > return; > > > } > > > > > > + bitmap = g_new0(unsigned long, bitmap_size); > > > + > > > > How hot is this function? It looks (unsurprisingly) like the section > > size determines the map size so I wonder whether it can instead be > > allocated once when the section is added? > > I think we can store the bitmap buffer into the `state' where > `log_for_dirtybit' is already present, and free the bitmap when > `log_for_dirtybit' is cleared. That sounds better :-) Cheers, Paul > > Thanks, > > -- > Anthony PERARD
diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c index 725f9c2278..10d73b55b4 100644 --- a/hw/i386/xen/xen-hvm.c +++ b/hw/i386/xen/xen-hvm.c @@ -615,7 +615,8 @@ static void xen_sync_dirty_bitmap(XenIOState *state, { hwaddr npages = size >> TARGET_PAGE_BITS; const int width = sizeof(unsigned long) * 8; - unsigned long bitmap[DIV_ROUND_UP(npages, width)]; + unsigned long *bitmap = NULL; + size_t bitmap_size = DIV_ROUND_UP(npages, width); int rc, i, j; const XenPhysmap *physmap = NULL; @@ -632,6 +633,8 @@ static void xen_sync_dirty_bitmap(XenIOState *state, return; } + bitmap = g_new0(unsigned long, bitmap_size); + rc = xen_track_dirty_vram(xen_domid, start_addr >> TARGET_PAGE_BITS, npages, bitmap); if (rc < 0) { @@ -644,10 +647,10 @@ static void xen_sync_dirty_bitmap(XenIOState *state, ", 0x" TARGET_FMT_plx "): %s\n", start_addr, start_addr + size, strerror(errno)); } - return; + goto out; } - for (i = 0; i < ARRAY_SIZE(bitmap); i++) { + for (i = 0; i < bitmap_size; i++) { unsigned long map = bitmap[i]; while (map != 0) { j = ctzl(map); @@ -657,6 +660,8 @@ static void xen_sync_dirty_bitmap(XenIOState *state, TARGET_PAGE_SIZE); }; } +out: + g_free(bitmap); } static void xen_log_start(MemoryListener *listener,
Avoid using a variable length array. Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> --- Notes: Was suggested by Peter here: <CAFEAcA88+A2oCkQnxKDEdpmfCZSmPzWMBg01wDDV68bMZoY5Jg@mail.gmail.com> "should we try to stop using variable length arrays?" hw/i386/xen/xen-hvm.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)