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 <mprivozn@xxxxxxxxxx>
> ---
> 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(&priv->job.cond, &obj->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(&now));
>
> - 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,
> + qemuDomainJobTypeToString(priv->job.active),
> + qemuDomainAsyncJobTypeToString(priv->job.asyncJob));
> + priv->job.agentActive = agentJob;
> + priv->job.agentOwner = virThreadSelfID();
> + priv->job.agentOwnerAPI = virThreadJobGet();
> priv->job.started = now;
This should be priv->job.agentStarted = 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 (qemuDomainTrackJob(job))
> @@ -6554,12 +6591,46 @@ int qemuDomainObjBeginJob(virQEMUDriverPtr driver,
> qemuDomainJob job)
> {
> if (qemuDomainObjBeginJobInternal(driver, obj, job,
> + QEMU_AGENT_JOB_NONE,
> QEMU_ASYNC_JOB_NONE, false) < 0)
> return -1;
> else
> return 0;
> }
>
> +/**
> + * qemuDomainObjBeginAgentJob:
> + *
> + * Grabs agent type of job. Use if caller talks to guest agent only.
> + *
> + * To end job call qemuDomainObjEndAgentJob.
> + */
> +int qemuDomainObjBeginAgentJob(virQEMUDriverPtr driver,
> + virDomainObjPtr obj,
> + qemuDomainAgentJob agentJob)
I know this file is pretty inconsistent and you're not making it a lot
worse, but s/int /int
/ and reindent the rest.
> +{
> + return qemuDomainObjBeginJobInternal(driver, obj, QEMU_JOB_NONE,
> + agentJob,
> + QEMU_ASYNC_JOB_NONE, false);
> +}
> +
> +/**
> + * qemuDomainObjBeginJobWithAgent:
> + *
> + * Grabs both monitor and agent types of job. Use if caller talks to both
> + * monitor and guest agent.
> + *
> + * To end job call qemuDomainObjEndJobWithAgent.
> + */
> +int qemuDomainObjBeginJobWithAgent(virQEMUDriverPtr driver,
> + virDomainObjPtr obj,
> + qemuDomainJob job,
> + qemuDomainAgentJob agentJob)
s/int /int
/ again
> +{
> + return qemuDomainObjBeginJobInternal(driver, obj, job,
> + agentJob, QEMU_ASYNC_JOB_NONE, false);
Strange indentation. And since you're going to touch the code here, the
following formatting would look a bit better:
return qemuDomainObjBeginJobInternal(driver, obj, job, agentJob,
QEMU_ASYNC_JOB_NONE, false);
> +}
> +
> int qemuDomainObjBeginAsyncJob(virQEMUDriverPtr driver,
> virDomainObjPtr obj,
> qemuDomainAsyncJob asyncJob,
> @@ -6569,6 +6640,7 @@ int qemuDomainObjBeginAsyncJob(virQEMUDriverPtr driver,
> qemuDomainObjPrivatePtr priv;
>
> if (qemuDomainObjBeginJobInternal(driver, obj, QEMU_JOB_ASYNC,
> + QEMU_AGENT_JOB_NONE,
> asyncJob, false) < 0)
> return -1;
>
> @@ -6598,6 +6670,7 @@ qemuDomainObjBeginNestedJob(virQEMUDriverPtr driver,
>
> return qemuDomainObjBeginJobInternal(driver, obj,
> QEMU_JOB_ASYNC_NESTED,
> + QEMU_AGENT_JOB_NONE,
> QEMU_ASYNC_JOB_NONE,
> false);
> }
> @@ -6621,6 +6694,7 @@ qemuDomainObjBeginJobNowait(virQEMUDriverPtr driver,
> qemuDomainJob job)
> {
> return qemuDomainObjBeginJobInternal(driver, obj, job,
> + QEMU_AGENT_JOB_NONE,
> QEMU_ASYNC_JOB_NONE, true);
> }
>
> @@ -6646,7 +6720,47 @@ qemuDomainObjEndJob(virQEMUDriverPtr driver, virDomainObjPtr obj)
> qemuDomainObjResetJob(priv);
> if (qemuDomainTrackJob(job))
> qemuDomainObjSaveJob(driver, obj);
> - virCondSignal(&priv->job.cond);
> + virCondBroadcast(&priv->job.cond);
> +}
> +
> +void
> +qemuDomainObjEndAgentJob(virDomainObjPtr obj)
> +{
> + qemuDomainObjPrivatePtr priv = obj->privateData;
> + qemuDomainAgentJob agentJob = priv->job.agentActive;
> +
> + priv->jobs_queued--;
> +
> + VIR_DEBUG("Stopping agent job: %s (async=%s vm=%p name=%s)",
> + qemuDomainAgentJobTypeToString(agentJob),
> + qemuDomainAsyncJobTypeToString(priv->job.asyncJob),
> + obj, obj->def->name);
> +
> + qemuDomainObjResetAgentJob(priv);
> + virCondBroadcast(&priv->job.cond);
> +}
> +
> +void
> +qemuDomainObjEndJobWithAgent(virQEMUDriverPtr driver,
> + virDomainObjPtr obj)
> +{
> + qemuDomainObjPrivatePtr priv = obj->privateData;
> + qemuDomainJob job = priv->job.active;
> + qemuDomainAgentJob agentJob = priv->job.agentActive;
> +
> + priv->jobs_queued--;
> +
> + VIR_DEBUG("Stopping both jobs: %s %s (async=%s vm=%p name=%s)",
> + qemuDomainJobTypeToString(job),
> + qemuDomainAgentJobTypeToString(agentJob),
> + qemuDomainAsyncJobTypeToString(priv->job.asyncJob),
> + obj, obj->def->name);
> +
> + qemuDomainObjResetJob(priv);
> + qemuDomainObjResetAgentJob(priv);
> + if (qemuDomainTrackJob(job))
> + qemuDomainObjSaveJob(driver, obj);
> + virCondBroadcast(&priv->job.cond);
> }
>
> void
> @@ -6800,8 +6914,9 @@ qemuDomainObjEnterMonitorAsync(virQEMUDriverPtr driver,
> * obj must be locked before calling
> *
> * To be called immediately before any QEMU agent API call.
> - * Must have already called qemuDomainObjBeginJob() and checked
> - * that the VM is still active.
> + * Must have already called qemuDomainObjBeginAgentJob() or
> + * qemuDomainObjBeginJobWithAgent() and checked that the VM is
> + * still active.
The documentation of qemuDomainObjEnterMonitorInternal would benefit
from similar treatment.
> *
> * To be followed with qemuDomainObjExitAgent() once complete
> */
...
With the small issues fixed
Reviewed-by: Jiri Denemark <jdenemar@xxxxxxxxxx>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
