Re: [PATCH] memory: batch allocate ioeventfds[] in address_space_update_ioeventfds()

[Stefan Hajnoczi]

On Tue, Feb 18, 2020 at 9:50 PM Peter Xu <address@hidden> wrote:
>
> On Tue, Feb 18, 2020 at 06:22:26PM +0000, Stefan Hajnoczi wrote:
> > Reallocing the ioeventfds[] array each time an element is added is very
> > expensive as the number of ioeventfds increases.  Batch allocate instead
> > to amortize the cost of realloc.
> >
> > This patch reduces Linux guest boot times from 362s to 140s when there
> > are 2 virtio-blk devices with 1 virtqueue and 99 virtio-blk devices with
> > 32 virtqueues.
> >
> > Signed-off-by: Stefan Hajnoczi <address@hidden>
> > ---
> >  memory.c | 17 ++++++++++++++---
> >  1 file changed, 14 insertions(+), 3 deletions(-)
> >
> > diff --git a/memory.c b/memory.c
> > index aeaa8dcc9e..2d6f931f8c 100644
> > --- a/memory.c
> > +++ b/memory.c
> > @@ -794,10 +794,18 @@ static void 
> > address_space_update_ioeventfds(AddressSpace *as)
> >      FlatView *view;
> >      FlatRange *fr;
> >      unsigned ioeventfd_nb = 0;
> > -    MemoryRegionIoeventfd *ioeventfds = NULL;
> > +    unsigned ioeventfd_max;
> > +    MemoryRegionIoeventfd *ioeventfds;
> >      AddrRange tmp;
> >      unsigned i;
> >
> > +    /*
> > +     * It is likely that the number of ioeventfds hasn't changed much, so 
> > use
> > +     * the previous size as the starting value.
> > +     */
> > +    ioeventfd_max = as->ioeventfd_nb;
> > +    ioeventfds = g_new(MemoryRegionIoeventfd, ioeventfd_max);
>
> Would the ioeventfd_max being cached and never goes down but it can
> only keep or increase?

No, it will decrease but only the next time
address_space_update_ioeventfds() is called.  That's when we'll use
the next ioeventfds_nb as the starting point.

> I'm not sure if that's a big problem, but
> considering the commit message mentioned 99 virtio-blk with 32 queues
> each, I'm not sure... :)
>
> I'm thinking maybe start with a relative big number but always under
> control (e.g., 64), then...

I also considered doing a final g_realloc() at the end of the function
to get rid of the excess allocation but I'm not sure it's worth it...

> > +
> >      view = address_space_get_flatview(as);
> >      FOR_EACH_FLAT_RANGE(fr, view) {
> >          for (i = 0; i < fr->mr->ioeventfd_nb; ++i) {
> > @@ -806,8 +814,11 @@ static void 
> > address_space_update_ioeventfds(AddressSpace *as)
> >                                               
> > int128_make64(fr->offset_in_region)));
> >              if (addrrange_intersects(fr->addr, tmp)) {
> >                  ++ioeventfd_nb;
> > -                ioeventfds = g_realloc(ioeventfds,
> > -                                          ioeventfd_nb * 
> > sizeof(*ioeventfds));
> > +                if (ioeventfd_nb > ioeventfd_max) {
> > +                    ioeventfd_max += 64;
>
> ... do exponential increase here (max*=2) instead so still easy to
> converge?

I'm happy to tweak the policy.  Let's see what Paolo thinks.

Stefan