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@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 */ }
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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list