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@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 */ } (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@xxxxxxxxxx > https://www.redhat.com/mailman/listinfo/libvir-list
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list