[libvirt] [PATCH 4/4] conf: Optimize the iothreadid initialization

[Peter Krempa]

On Tue, Oct 13, 2015 at 11:47:10 -0400, John Ferlan wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1264008
> 
> The existing algorithm assumed that someone was making small, incremental
> changes; however, it is possible to change iothreads from 0 (or relatively
> small number) to some really large number and the algorithm would possibly
> spin its wheels doing unnecessary searches.

While the existing algorithm was "suboptimal" the use case is strange
too. Starting a million iothreads certainly won't help in the total
performance. 

> 
> So, optimize the algorithm in order to first detect whether there are any
> iothreadid's defined in the XML. If not, then rather than add one at a time
> searching for the next valid id, just allocate the whole array and populate
> it as designed starting at iothread_id = 1 up to the number of iothreads
> defined in the XML
> 
> Otherwise, we have a situation where "some number" of iothreadid's were
> defined in the XML and we're filling in the holes of iothread_id's. Thus,
> instead of determining if the iothread_id was used (ThreadIDFind) for
> every ID entry that needs to be filled in, let's only call the find while
> we still have holes and rather than additionally calling ThreadIDAdd
> (which also calls the ThreadIDFind), let's just directly add the entry.
> 
> These algorithm changes only "penalize" those with a large iothreads
> value that also have a large number of iothread id's which aren't
> completely defined.
> 
> Signed-off-by: John Ferlan <jferlan at redhat.com>
> ---
>  src/conf/domain_conf.c | 46 +++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 41 insertions(+), 5 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 217179d..6c90653 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -2334,6 +2334,8 @@ virDomainIOThreadIDDefArrayInit(virDomainDefPtr def)
>  {
>      unsigned int iothread_id = 1;
>      int retval = -1;
> +    size_t i;
> +    virDomainIOThreadIDDefPtr iothrid = NULL;
>  
>      /* Same value (either 0 or some number), then we have none to fill in or
>       * the iothreadid array was filled from the XML
> @@ -2341,15 +2343,49 @@ virDomainIOThreadIDDefArrayInit(virDomainDefPtr def)
>      if (def->iothreads == def->niothreadids)
>          return 0;

The code below seems too complex. I'd suggest something along the
following pseudo-code:

    assert(def->iothreads >= def->niothreads);

    virBitmapPtr *threads = virBitmapNew(def->iothreads);
    ssize_t nxt = 0;

    virBitmapSetAll(threads);

    /* mark which are already provided by the user */
    for (i = 0; i < def->niothreads; i++)
        virBitmapClearBit(threads, def->iothreadids[i]->id);

    /* resize array */
    VIR_REALLOC_N(def->iothreadids....);

    while ((nxt = virBitmapNextSetBit(bitmap, nxt)) >= 0) {
        /* add the stuff */
    }


(may contain off-by-ones)

>  
> -    while (def->niothreadids != def->iothreads) {
> -        if (!virDomainIOThreadIDFind(def, iothread_id)) {
> -            virDomainIOThreadIDDefPtr iothrid;
> +    /* Optimize - there are no <iothread id='#'> in the XML */
> +    if (def->niothreadids == 0) {
> +        if (VIR_ALLOC_N(def->iothreadids, def->iothreads) < 0)
> +            goto error;
> +        def->niothreadids = def->iothreads;
> +        for (i = 1; i <= def->iothreads; i++) {
> +            if (VIR_ALLOC(iothrid) < 0)
> +                goto error;
> +            def->iothreadids[i - 1] = iothrid;
> +            iothrid->iothread_id = i;
> +            iothrid->autofill = true;
> +        }
> +    } else {
> +        int found = 0;
> +        int orig_nids = def->niothreadids;
> +
> +        /* <iothread id='#'> entries were found, then let's fill in the
> +         * holes one at a time, e.g. the relatively hard way. Rather than
> +         * using ThreadIDFind and call ThreadIDAdd which also calls
> +         * ThreadIDFind again which could cause lots of needless spinning
> +         * let's just add the entries directly
> +         */
> +        for (i = 0;
> +             i < def->iothreads && def->niothreadids != def->iothreads; i++) {

I'll happily live with a longer line rather than a broken if statement.

> +            /* While we still have defined <thread id='#'>'s compare our
> +             * current thread_id value against the array.
> +             */
> +            if (found < orig_nids &&
> +                virDomainIOThreadIDFind(def, iothread_id)) {
> +                iothread_id++;
> +                found++;
> +                continue;
> +            }
>  
> -            if (!(iothrid = virDomainIOThreadIDAdd(def, iothread_id)))
> +            /* Add a new entry using the current iothread_id */
> +            if (VIR_ALLOC(iothrid) < 0)
>                  goto error;
> +            iothrid->iothread_id = iothread_id++;
>              iothrid->autofill = true;
> +            if (VIR_APPEND_ELEMENT_COPY(def->iothreadids, def->niothreadids,
> +                                        iothrid) < 0)

Rather than extending the array all the time you can extend it once and
then fill it.

> +                goto error;
>          }
> -        iothread_id++;
>      }
>      retval = 0;
>  
> -- 
> 2.1.0
> 
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20151013/ccc94906/attachment-0001.sig>