[libvirt] [PATCH 4/4] conf: Optimize the iothreadid initialization

[Peter Krempa]

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20151014/7cc3d0f6/attachment-0001.sig>