[libvirt] [PATCH v2 3/9] conf: Add new domain XML element 'iothreadids'

[John Ferlan]

On 04/13/2015 08:13 AM, Peter Krempa wrote:
> On Fri, Apr 10, 2015 at 17:36:21 -0400, John Ferlan wrote:

...

>> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
>> index 95cbb9c..03a0ecd 100644
>> --- a/src/conf/domain_conf.h
>> +++ b/src/conf/domain_conf.h
>> @@ -2041,6 +2041,19 @@ struct _virDomainHugePage {
>>      unsigned long long size;    /* hugepage size in KiB */
>>  };
>>  
>> +typedef struct _virDomainIOThreadIDDef virDomainIOThreadIDDef;
>> +typedef virDomainIOThreadIDDef *virDomainIOThreadIDDefPtr;
>> +
>> +struct _virDomainIOThreadIDDef {
>> +    bool defined;
>> +    unsigned int iothread_id;
>> +    char *name;
> 
> This structure along with the one you add in the next patch and the
> pinning structures that already exist make now three places where we
> store data regarding IO threads. I don't think we should do that.
> Keeping track of which data introduces messy code.
> 
> I think it will be desirable that you move the data regarding pinning of
> iothreads into this structure along with thread ids from the monitor so
> that we can keep everything in one place.
> 
> 

While I don't disagree that having multiple places and data structures
is suboptimal - it is no different than the existing vcpu code which has
a [n]vcpupids list for the monitor/driver stored and separate cputune
vcpupin list. What it doesn't have is a mechanism to have different vcpu
id numbers - it forces sequential.  There's also a lot to be said for
keeping the cputune stuff together as much as there is for keeping the
iothreadsid stuff together.

The whole point behind not printing out <iothreadsid> <iothread ...> was
if it doesn't exist, we can continue with the existing algorithm and no
one is the wiser.  As soon as someone goes to 'customize' by adding a
non sequential id, then things are exposed.  I could care less either
way though. If the general feeling is print it regardless of whether it
was found on input, then that's fine by me - makes it easier.

Another "option" for the <name> (if it was to be kept) is that it
becomes the alias for the thread. The current algorithm just generates
the "iothread#" as a/the mechanism for the alias. An IOThread when
"added" can use any name as long as it's unique.

All that said - I'm fine with removing <name> - it was added mainly
because I felt "id" would be lonely all by itself ;-)... Then moving the
cputune.iothreadpin data into the internal workings for iothreadid's is
fine - just have to account for existing configurations in some manner.
 Finally having the "live" information in the same data structure is
fine - I separated it mainly because existing code has it separated. I
didn't particularly like the existing code, but since no one has changed
it for all the years it's been there, well I didn't want that added onto
the "to do" list.

Finally as for some of the extraneous comments - you may in some cases
find them stating obvious facts, I've also seen that what some consider
to be self documenting code isn't in fact self documenting unless you're
the author or have become very familiar with the code due to working on
patches in the space.

I'll rework and repost tomorrow.  Good to know this is moving in the
right direction

John


FWIW: In patch 8 where I used // - yes I knew that (the .0 mentioned
it), but I was trying to draw attention to it.  The vcpu code doesn't do
much in the way of error recovery and while I could just do the same, I
figured I'd point it out rather than just ignore it.  I knew the //
would cause someone to balk.

>> +};
>> +
>> +void virDomainIOThreadIDDefFree(virDomainIOThreadIDDefPtr def);
>> +void virDomainIOThreadIDDefArrayFree(virDomainIOThreadIDDefPtr *def,
>> +                                     int nids);
>> +
>>  typedef struct _virDomainCputune virDomainCputune;
>>  typedef virDomainCputune *virDomainCputunePtr;
>>  
> 
> I think the direction this series is taking is okay.
> 
> Peter
>