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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 10/13/2015 12:29 PM, Peter Krempa wrote:
> 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. 
> 

Yeah - I agree, but since we didn't set a limit... I think to a degree
we are stuck.  Sometimes I wonder about the reality of tests. Then
again, I seem to recall a recent dialog over the number of nics (and
thus hwaddrs) that were deemed reasonable when the algorithm was first
written...

>>
>> 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@xxxxxxxxxx>
>> ---
>>  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 */
>     }
> 
This would work if we required thread_id values to be <= def->iothreads,
but we don't, yet...  I thought of using a bitmap, but it would no cover
the following case :

   <iothreads>4</iothreads>
   <iothreadids>
     <iothread id='100'/>
   <iothreadids>


This would/should create thread ids 100, 1, 2, 3

We never placed a limit on the thread_id value other than it cannot be
zero.  Although we do indicate the default algorithm is 1 to the number
of iothreads.

Would a 'real world' user do something like this - perhaps not.  Much
less make "<iothreads>1000000</iothreads>" (that'd be one long command
line - say nothing of the resources required in order to really start it.

I'd be all for restricting iothread id values to be <= def->iothreads -
that'd solve the unnecessarily complex algorithm.

My other thought was to restrict the number of iothreads to be the
number of disk devices or even 2 * ndisks. It's possible to assign
multiple disks to 1 iothread, but impossible to assign 1 disk to
multiple threads.


> 
> (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.
> 

Well 80 cols max is the desired number. Although in thinking about it, I
don't think we could ever hit the i == def->iothreads - if we did then
there was another error.  Removing it moves us back down to < 80 all on
one for line.

>> +            /* 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.
> 

Suffice to say those VIR_APPEND_* are not necessarily self documenting.
I assume though you are indicating using VIR_RESIZE_N. However, that may
not work in 'all cases'. This algorithm handles the case where there's 5
iothreads and let's say id "2" and "5" are defined.  There's no way to
"know" what the id's are without looking and I gave up sorting them.
Again, assuming iothread_id can be > def->iothreads.

John

>> +                goto error;
>>          }
>> -        iothread_id++;
>>      }
>>      retval = 0;
>>  
>> -- 
>> 2.1.0
>>
>> --
>> libvir-list mailing list
>> libvir-list@xxxxxxxxxx
>> https://www.redhat.com/mailman/listinfo/libvir-list

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]