Re: [libvirt] [PATCH v3 1/9] Implement public API for virDomainGetIOThreadsInfo
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(a)redhat.com>
> ---
> 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