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