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