Re: [PATCH v3 1/9] Implement public API for virDomainGetIOThreadsInfo

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 03/04/2015 12:24 PM, Ján Tomko wrote:
> On Tue, Feb 17, 2015 at 04:03:50PM -0500, John Ferlan wrote:
>> Add virDomainGetIOThreadsInfo in order to return a list of
>> virDomainIOThreadsInfoPtr structures which list the IOThread ID,
>> the CPU Affinity map, and associated resources for each IOThread
>> for the domain. For an active domain, the live data will be
>> returned, while for an inactive domain, the config data will be
>> returned. The resources data is expected to be the src path for
>> the disks that have an assigned iothread.
>>
>> The API supports either the --live or --config flag, but not both.
>>
>> Also added virDomainIOThreadsInfoFree in order to free the cpumap,
>> list of resources, and the array entry structure.
>>
>> Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx>
>> ---
>>  include/libvirt/libvirt-domain.h | 23 +++++++++++-
>>  src/driver-hypervisor.h          |  8 +++-
>>  src/libvirt-domain.c             | 80 +++++++++++++++++++++++++++++++++++++++-
>>  src/libvirt_public.syms          |  6 +++
>>  4 files changed, 114 insertions(+), 3 deletions(-)
>>
>>  
>>  /**
>> + * virIOThreadsInfo:
> 
> s/Threads/Thread/ as it only contains info about one thread.
> 

Since IOThreads was the "techology name" - I just followed suit and used
it, but will change it.

>> + *
>> + * The data structure for information about IOThreads in a domain
>> + */
>> +typedef struct _virDomainIOThreadsInfo virDomainIOThreadsInfo;
>> +typedef virDomainIOThreadsInfo *virDomainIOThreadsInfoPtr;
>> +struct _virDomainIOThreadsInfo {
>> +    unsigned int iothread_id;          /* IOThread ID */
> 
> Since iothread ids are sequential, starting from 1, this value will
> always be the index in the array + 1.
> 

Well - today they are, but once the "Add" and "Remove" functionality is
added we could have gaps since you'll be able to define/start with 3
threads and conceivably delete thread #2 (or the middle one).

>> +    unsigned char *cpumap;             /* CPU map for thread */
>> +    int cpumaplen;                     /* cpumap size */
> 
>> +    size_t nresources;                 /* count of resources using IOThread */
>> +    char **resources;                  /* array of resources using IOThread */
> 
> "resources" is too vague.

Suggestion?  Is "devices" better?

Today it's the disk source path, but I remember reading something where
an IOThread could be potentially used for something else (perhaps
network, but I cannot find the reference quickly).

> Moreover, storing the source path here does not seem that much useful to
> me, considering that the corresponding *Set API cannot change the
> resources used by the thread (also, some disks don't even have a path).

Adding a disk to a domain and assigning an iothread to it is
accomplished via the 'attach-disk'. There's a "finite" set of support
for now - a virtio-scsi local disk, which I thought had to have a path
defined as opposed to some remote disk where the "path" is generated.

> 
> Considering that the XML format is copying <vcpupin>, maybe this API
> could have a similar signature? I.e.:
> 
> int
> virDomainGetIOThreadPin(virDomainPtr domain,
>                         unsigned int ncpumaps,
>                         unsigned char *cpumaps,
>                         unsigned int maplen,
>                         unsigned int flags)
> 
> Or without the 'ncpumaps' parameter, so we don't need an API to get the
> number of iothreads.
> 
> Another possibility is getting rid of the 'maplen' as well and return
> the bitmap as formatted by virBitmapFormat (but the client app still
> needs the node CPU count to interpret it correctly).

This is where I disagree.  The "GetIOThreadsInfo" API is more like the
vcpuinfo API insomuch as it gets multiple aspects of IOThread objects.

As a way of understanding my thought process - I went with the one API
rather than an API to get IOThread data and a separate API to get
IOThread pin data because I felt a one round trip operation was 'better'
than 1..n round trips. If a separate API is requested or required, I'll
add it, but I'd prefer to not call it during the IOThreadsInfo fetch
operation.

Thanks for the review -

John

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list





[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]