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

[Michal Privoznik]

On 06/14/2018 12:04 AM, John Ferlan wrote:
> 
> 
> 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.

Okay, sounds better than mine commit message.

> 
>>
>> 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 ;-)

Yeah, I was considering that. But for some reason did not do that. I
don't care, so I'll merge it.

> 
>>  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.

No this is something different. The problem with using enums in
virDomainDef (or any other struct that is immediate part of XML parsing)
is that some of us use the following construct:

  typedef enum {
    VIR_SOME_ENUM_ITEM1,
    VIR_SOME_ENUM_ITEM2,
    ...
    VIR_SOME_ENUM_ITEM_LAST
  } virSomeEnum;

  virSomeEnum val = virSomeEnumTypeFromString(str);

The problem lies in the last line because TypeFromString() can return -1
(if @str does not fall into virSomeEnum). Therefore the last line can be
rewritten as:

  virSomeEnum val = -1;

I guess you can see the problem now. There are basically two ways to get
rid of this problem. The first one is to use an intermediary integer:

  virSomEnum val;
  int tmp = virSomeEnumTypeFromString(str);

  if (tmp < 0)
    error;
  else
    val = tmp;

The other way is to move the integer into the struct and put into the
comment reference to the enum (this is what Dan is doing in the patch
you're referring to).

However, my scenario is different. Whenever setting the struct member
it's only using corresponding enum values.

> 
>> +    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.
> 

No. Using one condition is the point of these patches. IMO having two
conditions would only complicate things needlessly. The code is guarded
against "spurious" wakeups from condition.

Michal