On Tue, Oct 13, 2015 at 13:54:54 -0400, John Ferlan wrote:
On 10/13/2015 12:29 PM, Peter Krempa wrote:
On Tue, Oct 13, 2015 at 11:47:10 -0400, John Ferlan wrote:
...
@@ -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) {
[1]
/* 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 :
Actually not necessarily, you only need to map the IDs in the range of <0,def->iothreads> since the worst case scenario is that you'd be adding all the iothreads in that range (if def->niothreadids is 0). Any iothread with id greater than def->iothreads will be ignored in the mapping process, but left in the array so the suggested loop [1] needs to be slightly modified: while ((nxt = virBitmapNextSetBit(bitmap, nxt)) >= 0 && def->niothreadis < def->iothreads) {
<iothreads>4</iothreads> <iothreadids> <iothread id='100'/> <iothreadids>
This would/should create thread ids 100, 1, 2, 3
The code above with the slight tweak will produce exactly the results above.
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.
IMO, none of the above is necessary. Peter