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