Re: [libvirt] [v2 RESEND 2/3] qemu: Introduce APIs for manipulating qemuDomainAgentJob
On Tue, Jun 19, 2018 at 08:38:01 +0200, Michal Privoznik wrote:
> The point is to break QEMU_JOB_* into smaller pieces which
> enables us to achieve higher throughput. For instance, if there
> are two threads, one is trying to query something on qemu
> monitor while the other is trying to query something on agent
> monitor these two threads would serialize. There is not much
> reason for that.
>
> Signed-off-by: Michal Privoznik
> ---
> src/qemu/THREADS.txt | 112 ++---
> src/qemu/qemu_domain.c | 187
> +++--
> src/qemu/qemu_domain.h | 12
> 3 files changed, 264 insertions(+), 47 deletions(-)
...
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 91f3c6d236..30a06a1575 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
...
> @@ -6356,6 +6369,15 @@ qemuDomainJobAllowed(qemuDomainObjPrivatePtr priv,
> qemuDomainJob job)
> return !priv->job.active && qemuDomainNestedJobAllowed(priv, job);
> }
>
> +static bool
> +qemuDomainObjCanSetJob(qemuDomainObjPrivatePtr priv,
> + qemuDomainJob job,
> + qemuDomainAgentJob agentJob)
> +{
> +return (job == QEMU_JOB_NONE || !priv->job.active) &&
> + (agentJob == QEMU_AGENT_JOB_NONE || !priv->job.agentActive);
This is pretty strange, everything you compare here is an enum, either
qemuDomainJob or qemuDomainAgentJob and mixing ! with an explicit
comparison with *_NONE is confusing. It's not incorrect, but I think
(job == QEMU_JOB_NONE || priv->job.active == QEMU_JOB_NONE) &&
(agentJob == QEMU_AGENT_JOB_NONE || priv->job.agentActive ==
QEMU_AGENT_JOB_NONE)
would be easier to read. Or alternatively you could use ! everywhere.
> +}
> +
> /* Give up waiting for mutex after 30 seconds */
> #define QEMU_JOB_WAIT_TIME (1000ull * 30)
>
...
> @@ -6434,10 +6456,9 @@ qemuDomainObjBeginJobInternal(virQEMUDriverPtr driver,
> goto error;
> }
>
> -while (priv->job.active) {
> +while (!qemuDomainObjCanSetJob(priv, job, agentJob)) {
You're now checking more variables for a single condition which means
waking up a single thread with virCondSignal could easily wake the one
which cannot currently run. We need to use virCondBroadcast instead and
I see you did the change, which is nice. However, it might be useful to
add a comment to the code explaining why we use virCondBroadcast to
avoid any future confusion or even blind attempts to replace it with
virCondSignal.
> if (nowait)
> goto cleanup;
> -
Drop this line removal, it's most likely a rebase artifact anyway.
> VIR_DEBUG("Waiting for job (vm=%p name=%s)", obj, obj->def->name);
> if (virCondWaitUntil(>job.cond, >parent.lock, then) < 0)
> goto error;
> @@ -6448,32 +6469,48 @@ qemuDomainObjBeginJobInternal(virQEMUDriverPtr driver,
> if (!nested && !qemuDomainNestedJobAllowed(priv, job))
> goto retry;
>
> -qemuDomainObjResetJob(priv);
> -
> ignore_value(virTimeMillisNow());
>
> -if (job != QEMU_JOB_ASYNC) {
> -VIR_DEBUG("Started job: %s (async=%s vm=%p name=%s)",
> - qemuDomainJobTypeToString(job),
> - qemuDomainAsyncJobTypeToString(priv->job.asyncJob),
> - obj, obj->def->name);
> -priv->job.active = job;
> -priv->job.owner = virThreadSelfID();
> -priv->job.ownerAPI = virThreadJobGet();
> +if (job) {
> +qemuDomainObjResetJob(priv);
> +
> +if (job != QEMU_JOB_ASYNC) {
> +VIR_DEBUG("Started job: %s (async=%s vm=%p name=%s)",
> + qemuDomainJobTypeToString(job),
> + qemuDomainAsyncJobTypeToString(priv->job.asyncJob),
> + obj, obj->def->name);
> +priv->job.active = job;
> +priv->job.owner = virThreadSelfID();
> +priv->job.ownerAPI = virThreadJobGet();
> +priv->job.started = now;
> +} else {
> +VIR_DEBUG("Started async job: %s (vm=%p name=%s)",
> + qemuDomainAsyncJobTypeToString(asyncJob),
> + obj, obj->def->name);
> +qemuDomainObjResetAsyncJob(priv);
> +if (VIR_ALLOC(priv->job.current) < 0)
> +goto cleanup;
> +priv->job.current->status = QEMU_DOMAIN_JOB_STATUS_ACTIVE;
> +priv->job.asyncJob = asyncJob;
> +priv->job.asyncOwner = virThreadSelfID();
> +priv->job.asyncOwnerAPI = virThreadJobGet();
> +priv->job.asyncStarted = now;
> +priv->job.current->started = now;
> +}
> +}
> +
> +if (agentJob) {
> +qemuDomainObjResetAgentJob(priv);
> +
> +VIR_DEBUG("Started agent job: %s (vm=%p name=%s job=%s async=%s)",
> + qemuDomainAgentJobTypeToString(agentJob),
> + obj, obj->def->name,
> +
[libvirt] [v2 RESEND 2/3] qemu: Introduce APIs for manipulating qemuDomainAgentJob
The point is to break QEMU_JOB_* into smaller pieces which enables us to achieve higher throughput. For instance, if there are two threads, one is trying to query something on qemu monitor while the other is trying to query something on agent monitor these two threads would serialize. There is not much reason for that. Signed-off-by: Michal Privoznik --- src/qemu/THREADS.txt | 112 ++--- src/qemu/qemu_domain.c | 187 +++-- src/qemu/qemu_domain.h | 12 3 files changed, 264 insertions(+), 47 deletions(-) diff --git a/src/qemu/THREADS.txt b/src/qemu/THREADS.txt index 7243161fe3..d17f3f4e0c 100644 --- a/src/qemu/THREADS.txt +++ b/src/qemu/THREADS.txt @@ -51,8 +51,8 @@ There are a number of locks on various objects Since virDomainObjPtr lock must not be held during sleeps, the job conditions provide additional protection for code making updates. -QEMU driver uses two kinds of job conditions: asynchronous and -normal. +QEMU driver uses three kinds of job conditions: asynchronous, agent +and normal. Asynchronous job condition is used for long running jobs (such as migration) that consist of several monitor commands and it is @@ -69,16 +69,23 @@ There are a number of locks on various objects it needs to wait until the asynchronous job ends and try to acquire the job again. +Agent job condition is then used when thread wishes to talk to qemu +agent monitor. It is possible to acquire just agent job +(qemuDomainObjBeginAgentJob), or only normal job +(qemuDomainObjBeginJob) or both at the same time +(qemuDomainObjBeginJobWithAgent). Which type of job to grab depends +whether caller wishes to communicate only with agent socket, or only +with qemu monitor socket or both, respectively. + Immediately after acquiring the virDomainObjPtr lock, any method -which intends to update state must acquire either asynchronous or -normal job condition. The virDomainObjPtr lock is released while -blocking on these condition variables. Once the job condition is -acquired, a method can safely release the virDomainObjPtr lock -whenever it hits a piece of code which may sleep/wait, and -re-acquire it after the sleep/wait. Whenever an asynchronous job -wants to talk to the monitor, it needs to acquire nested job (a -special kind of normal job) to obtain exclusive access to the -monitor. +which intends to update state must acquire asynchronous, normal or +agent job . The virDomainObjPtr lock is released while blocking on +these condition variables. Once the job condition is acquired, a +method can safely release the virDomainObjPtr lock whenever it hits +a piece of code which may sleep/wait, and re-acquire it after the +sleep/wait. Whenever an asynchronous job wants to talk to the +monitor, it needs to acquire nested job (a special kind of normal +job) to obtain exclusive access to the monitor. Since the virDomainObjPtr lock was dropped while waiting for the job condition, it is possible that the domain is no longer active @@ -132,6 +139,30 @@ To acquire the normal job condition +To acquire the agent job condition + + qemuDomainObjBeginAgentJob() +- Waits until there is no other agent job set +- Sets job.agentActive tp the job type + + qemuDomainObjEndAgentJob() +- Sets job.agentActive to 0 +- Signals on job.cond condition + + + +To acquire both normal and agent job condition + + qemuDomainObjBeginJobWithAgent() +- Waits until there is no normal and no agent job set +- Sets both job.active and job.agentActive with required job types + + qemuDomainObjEndJobWithAgent() +- Sets both job.active and job.agentActive to 0 +- Signals on job.cond condition + + + To acquire the asynchronous job condition qemuDomainObjBeginAsyncJob() @@ -247,6 +278,65 @@ Design patterns virDomainObjEndAPI(); + * Invoking an agent command on a virDomainObjPtr + + virDomainObjPtr obj; + qemuAgentPtr agent; + + obj = qemuDomObjFromDomain(dom); + + qemuDomainObjBeginAgentJob(obj, QEMU_AGENT_JOB_TYPE); + + ...do prep work... + + if (!qemuDomainAgentAvailable(obj, true)) + goto cleanup; + + agent = qemuDomainObjEnterAgent(obj); + qemuAgent(agent, ..); + qemuDomainObjExitAgent(obj, agent); + + ...do final work... + + qemuDomainObjEndAgentJob(obj); + virDomainObjEndAPI(); + + + * Invoking both monitor and agent commands on a virDomainObjPtr + + virDomainObjPtr obj; + qemuAgentPtr agent; + + obj = qemuDomObjFromDomain(dom); + + qemuDomainObjBeginJobWithAgent(obj, QEMU_JOB_TYPE, QEMU_AGENT_JOB_TYPE); + + if (!virDomainObjIsActive(dom)) + goto cleanup; + + ...do prep work... + + if (!qemuDomainAgentAvailable(obj, true)) + goto cleanup; + + agent =
