Re: [libvirt] [PATCH 1/4] qemu: Introduce qemuDomainAgentJob
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(a)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 */