Re: [libvirt] [PATCH 2/4] event: introduce new event for cputune

On 08/28/2014 02:38 PM, Pavel Hrdina wrote: > Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; > --- > daemon/remote.c | 87 +++++++++++++++++++++++++++++++ > include/libvirt/libvirt.h.in | 62 ++++++++++++++++++++++ > src/conf/domain_event.c | 120 +++++++++++++++++++++++++++++++++++++++++++ > src/conf/domain_event.h | 7 +++ > src/libvirt_private.syms | 2 + > src/remote/remote_driver.c | 110 +++++++++++++++++++++++++++++++++++++++ > src/remote/remote_protocol.x | 39 +++++++++++++- > src/remote_protocol-structs | 32 ++++++++++++ > tools/virsh-domain.c | 49 ++++++++++++++++++ > 9 files changed, 507 insertions(+), 1 deletion(-) > Should it be noted in any documentation (docs/*.html.in) about the new event being triggered? My quick scan didn't find anything, but figured I'd ask to be sure.

> diff --git a/daemon/remote.c b/daemon/remote.c > index 89714ca..ae42c4d 100644 > --- a/daemon/remote.c > +++ b/daemon/remote.c > @@ -969,6 +969,92 @@ remoteRelayDomainEventBlockJob2(virConnectPtr conn, > } > > > +static int > +remoteRelayDomainEventCputune(virConnectPtr conn, > + virDomainPtr dom, > + virDomainCputuneInfoPtr cputune, > + void *opaque) > +{ > + daemonClientEventCallbackPtr callback = opaque; > + remote_domain_event_cputune_msg data; > + size_t i; > + > + if (callback->callbackID < 0 || > + !remoteRelayDomainEventCheckACL(callback->client, conn, dom)) > + return -1; > + > + VIR_DEBUG("Relaying domain cputune event %s %d, callback %d", > + dom->name, dom->id, callback->callbackID); > + > + /* build return data */ > + memset(&data, 0, sizeof(data)); > + make_nonnull_domain(&data.dom, dom); > + > + data.shares = cputune->shares; > + data.sharesSpecified = cputune->sharesSpecified; > + data.period = cputune->period; > + data.quota = cputune->quota; > + data.emulatorPeriod = cputune->emulatorPeriod; > + data.emulatorQuota = cputune->emulatorQuota; > + data.nvcpupin = cputune->nvcpupin; > + > + if (cputune->emulatorpin.map) { > + if (VIR_ALLOC_N(data.emulatorpin.map.map_val, > + cputune->emulatorpin.mapLen) < 0) > + goto error; > + memcpy(data.emulatorpin.map.map_val, cputune->emulatorpin.map, > + cputune->emulatorpin.mapLen); > + data.emulatorpin.map.map_len = cputune->emulatorpin.mapLen; > + data.emulatorpin.mapLen = cputune->emulatorpin.mapLen; > + } > + if (cputune->vcpupin) { > + if (VIR_ALLOC_N(data.vcpupin.vcpupin_val, data.nvcpupin) < 0) > + goto error; > + data.vcpupin.vcpupin_len = data.nvcpupin; > + > + for (i = 0; i < data.nvcpupin; i++) { > + data.vcpupin.vcpupin_val[i].vcpuid = cputune->vcpupin[i].vcpuid; > + if (VIR_ALLOC_N(data.vcpupin.vcpupin_val[i].cpumask.map.map_val, > + cputune->vcpupin[i].cpumask.mapLen) < 0) > + goto error; > + memcpy(data.vcpupin.vcpupin_val[i].cpumask.map.map_val, > + cputune->vcpupin[i].cpumask.map, > + cputune->vcpupin[i].cpumask.mapLen); Was this more or less what the unused VIR_COPY_CPUMAP was supposed to be? Nothing you created, but something I found during my recent look into the code.

> + data.vcpupin.vcpupin_val[i].cpumask.map.map_len = > + cputune->vcpupin[i].cpumask.mapLen; > + data.vcpupin.vcpupin_val[i].cpumask.mapLen = > + cputune->vcpupin[i].cpumask.mapLen; > + } > + } > + > + if (callback->legacy) { > + remoteDispatchObjectEventSend(callback->client, remoteProgram, > + REMOTE_PROC_DOMAIN_EVENT_CPUTUNE, > + (xdrproc_t)xdr_remote_domain_event_cputune_msg, > + &data); > + } else { > + remote_domain_event_callback_cputune_msg msg = { callback->callbackID, > + data }; > + > + remoteDispatchObjectEventSend(callback->client, remoteProgram, > + REMOTE_PROC_DOMAIN_EVENT_CALLBACK_CPUTUNE, > + (xdrproc_t)xdr_remote_domain_event_callback_cputune_msg, > + &msg); > + } > + > + return 0; > + > + error: > + VIR_FREE(data.emulatorpin.map.map_val); > + if (data.vcpupin.vcpupin_val) { > + for (i = 0; i < data.nvcpupin; i++) > + VIR_FREE(data.vcpupin.vcpupin_val[i].cpumask.map.map_val); > + VIR_FREE(data.vcpupin.vcpupin_val); > + } > + return -1; > +} > + > + > static virConnectDomainEventGenericCallback domainEventCallbacks[] = { > VIR_DOMAIN_EVENT_CALLBACK(remoteRelayDomainEventLifecycle), > VIR_DOMAIN_EVENT_CALLBACK(remoteRelayDomainEventReboot), > @@ -987,6 +1073,7 @@ static virConnectDomainEventGenericCallback domainEventCallbacks[] = { > VIR_DOMAIN_EVENT_CALLBACK(remoteRelayDomainEventPMSuspendDisk), > VIR_DOMAIN_EVENT_CALLBACK(remoteRelayDomainEventDeviceRemoved), > VIR_DOMAIN_EVENT_CALLBACK(remoteRelayDomainEventBlockJob2), > + VIR_DOMAIN_EVENT_CALLBACK(remoteRelayDomainEventCputune), > }; > > verify(ARRAY_CARDINALITY(domainEventCallbacks) == VIR_DOMAIN_EVENT_ID_LAST); > diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in > index 9358314..636b89b 100644 > --- a/include/libvirt/libvirt.h.in > +++ b/include/libvirt/libvirt.h.in > @@ -5127,7 +5127,68 @@ typedef void (*virConnectDomainEventDeviceRemovedCallback)(virConnectPtr conn, > virDomainPtr dom, > const char *devAlias, > void *opaque); > +/** > + * _virDomainCpumaksInfo s/maks/mask/ > + * > + * This structure stores a mask with pinning information for emulator > + * or vcpus. And of course I'm muddying things up by adding a iothreadpin > + */ > +struct _virDomainCpumaskInfo { > + int mapLen; > + unsigned char *map; > +}; > +typedef struct _virDomainCpumaskInfo virDomainCpumaskInfo; > +typedef virDomainCpumaskInfo *virDomainCpumaskInfoPtr; > + > +/** > + * _virDomainVcpupinInfo > + * > + * This structure stores cpumask with pinning information > + * for each vcpu where the pinning has been set. > + */ > +struct _virDomainVcpupinInfo { > + int vcpuid; Looking at this makes me think I should change the 'int vcpuid' to just 'int id' for my series (in _virDomainVcpuPinDef) - makes reusing this so much easier when it comes to the difference between vcpuid and iothreadid. Course that probably also means a structure name change from Vcpu to something more generic like Array. > + virDomainCpumaskInfo cpumask; > +}; > +typedef struct _virDomainVcpupinInfo virDomainVcpupinInfo; > +typedef virDomainVcpupinInfo *virDomainVcpupinInfoPtr; > + > +/** > + * _virDomainCputuneInfo > + * > + * Structure containing all infromation about cputune for > + * specific domain. s/infromation/information s/for specific domain/for a specific domain/ Your call - add something about this is for events or event mgmt API's. Basically this is the user view right?

> + */ > +struct _virDomainCputuneInfo { > + unsigned long long shares; > + int sharesSpecified; > + unsigned long long period; > + long long quota; > + unsigned long long emulatorPeriod; > + long long emulatorQuota; > + size_t nvcpupin; > + virDomainVcpupinInfoPtr vcpupin; > + virDomainCpumaskInfo emulatorpin; > +}; > +typedef struct _virDomainCputuneInfo virDomainCputuneInfo; > +typedef virDomainCputuneInfo *virDomainCputuneInfoPtr; > > +/** > + * virConnectDomainEventCputuneCallback: > + * @conn: connection object > + * @dom: domain on which the event occurred > + * @cputune: cputune informations s/informations/information/ > + * @opaque: application specified data > + * > + * This callback occurs when cpu tune is updated. s/cpu tune/guest cputune data/ (or something similar) The remainder of this seems OK. Although I'm curious to know if there's been any thought as to whether we bump up against any limits if there are a lot of remote_domain_vcpupin's (e.g. vcpupin_val) within the remote_domain_event_cputune_msg? Perhaps 1 or 2 for testing work, but what if someone has whatever the maximum is?

My other question or concern would be if/when the iothreadpin series is accepted - obviously another patch would be required to add iothreadpin data in. Would this need to be done within the same release version? And if it wasn't would there have to be a v2 type event? IOW, can event data be extended? If not, then I just have to be sure to set aside the time within the release scope.

Hopefully someone with a bit more knowledge of events can double check that you've touched everything you're supposed to touch John