[libvirt] [PATCH 1/4] qemu: Introduce qemuDomainAgentJob

[John Ferlan]

On 06/08/2018 09:45 AM, Michal Privoznik wrote:
> This enum will list all possible jobs for guest agent. The idea
> is when a thread needs to talk to guest agent only it will take
> the QEMU_AGENT_JOB instead of QEMU_JOB helping better
> concurrency.

Consider:

Introduce guest agent specific job categories to allow threads to run
agent monitor specific jobs while normal monitor jobs can also be running.

Alter _qemuDomainJobObj in order to duplicate certain fields that will
be used for guest agent specific tasks to increase concurrency and
throughput and reduce serialization.

> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  src/qemu/qemu_domain.c |  4 ++++
>  src/qemu/qemu_domain.h | 15 +++++++++++++++
>  2 files changed, 19 insertions(+)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 11c261db1a..b8e34c1c2c 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -93,6 +93,10 @@ VIR_ENUM_IMPL(qemuDomainJob, QEMU_JOB_LAST,
>                "async nested",
>  );
>  
> +VIR_ENUM_IMPL(qemuDomainAgentJob, QEMU_AGENT_JOB_LAST,
> +              "none"
> +);
> +

I think merging patch3 in here is perfectly reasonable, unless of course
you're looking to get your patch counts up to stay ahead of Peter ;-)

>  VIR_ENUM_IMPL(qemuDomainAsyncJob, QEMU_ASYNC_JOB_LAST,
>                "none",
>                "migration out",
> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
> index f17157b951..709b42e6fd 100644
> --- a/src/qemu/qemu_domain.h
> +++ b/src/qemu/qemu_domain.h
> @@ -82,6 +82,13 @@ typedef enum {
>  } qemuDomainJob;
>  VIR_ENUM_DECL(qemuDomainJob)
>  
> +typedef enum {
> +    QEMU_AGENT_JOB_NONE = 0,    /* No agent job. */
> +
> +    QEMU_AGENT_JOB_LAST
> +} qemuDomainAgentJob;
> +VIR_ENUM_DECL(qemuDomainAgentJob)
> +
>  /* Async job consists of a series of jobs that may change state. Independent
>   * jobs that do not change state (and possibly others if explicitly allowed by
>   * current async job) are allowed to be run even if async job is active.
> @@ -158,11 +165,19 @@ typedef struct _qemuDomainJobObj qemuDomainJobObj;
>  typedef qemuDomainJobObj *qemuDomainJobObjPtr;
>  struct _qemuDomainJobObj {
>      virCond cond;                       /* Use to coordinate jobs */
> +
> +    /* The following members are for QEMU_JOB_* */
>      qemuDomainJob active;               /* Currently running job */
>      unsigned long long owner;           /* Thread id which set current job */
>      const char *ownerAPI;               /* The API which owns the job */
>      unsigned long long started;         /* When the current job started */
>  
> +    /* The following members are for QEMU_AGENT_JOB_* */
> +    qemuDomainAgentJob agentActive;     /* Currently running agent job */

Hmm... I seem to recall a recent patch that spoke against using an enum
type for struct fields:

https://www.redhat.com/archives/libvir-list/2018-June/msg00486.html

While I know you're just copying the existing qemuDomainJob - this is
the kind of stuff that just gets copied around and used elsewhere.

> +    unsigned long long agentOwner;      /* Thread id which set current agent job */
> +    const char *agentOwnerAPI;          /* The API which owns the agent job */
> +    unsigned long long agentStarted;    /* When the current agent job started */
> +

FWIW: From the description in the next patch, I would think there would
need to be an agentCond variable too.  But reading patch 4 I know that's
not quite the case. I have concerns over the combined @cond usage, but
those are only towards some "future change" that misses some subtlety.

And the following few fields are for async jobs - may as well add a
comment here too...

John

>      virCond asyncCond;                  /* Use to coordinate with async jobs */
>      qemuDomainAsyncJob asyncJob;        /* Currently active async job */
>      unsigned long long asyncOwner;      /* Thread which set current async job */
>