On 6/16/22 16:18, Emanuele Giuseppe Esposito wrote:
In preparation to the job_lock/unlock usage, create _locked
duplicates of some functions, since they will be sometimes called with
job_mutex held (mostly within job.c),
and sometimes without (mostly from JobDrivers using the job API).
Therefore create a _locked version of such function, so that it
can be used in both cases.
List of functions duplicated as _locked:
job_is_ready (both versions are public)
job_is_completed (both versions are public)
job_is_cancelled (_locked version is public, needed by mirror.c)
job_pause_point (_locked version is static, purely done to simplify
the code)
job_cancel_requested (_locked version is static)
Note: at this stage, job_{lock/unlock} and job lock guard macros
are *nop*.
Great description, thanks!
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
Hmm, after this patch, part of public API has "called with/without lock"
comments. But there are still public job_* functions that doesn't have
this mark. That look inconsistent. I think, all public API without
_locked suffix, should be called without a lock? If so, we don't need to
write it for each function. And only mark _locked() functions with
"called with lock held" marks.
---
include/qemu/job.h | 25 +++++++++++++++++++++---
job.c | 48 ++++++++++++++++++++++++++++++++++++++++------
2 files changed, 64 insertions(+), 9 deletions(-)
[..]
-/** Returns whether the job is ready to be completed. */
+/** Just like job_is_completed, but called between job_lock and
job_unlock */
I'd prefer phrasing "called with job_lock held". You wording make me
think about
job_lock()
...
job_unlock()
foo()
job_lock()
...
job_unlock()
- foo() actually called between job_lock and job_unlock :)
(it's a nitpicking, you may ignore it :)
+bool job_is_completed_locked(Job *job);
+
+/**
+ * Returns whether the job is ready to be completed.
+ * Called with job_mutex *not* held.
+ */
bool job_is_ready(Job *job);
+/** Just like job_is_ready, but called between job_lock and
job_unlock */
+bool job_is_ready_locked(Job *job);
+
/**
* Request @job to pause at the next pause point. Must be paired with
* job_resume(). If the job is supposed to be resumed by user
action, call
diff --git a/job.c b/job.c
index cafd597ba4..c4776985c4 100644
--- a/job.c
+++ b/job.c
@@ -236,19 +236,32 @@ const char *job_type_str(const Job *job)
return JobType_str(job_type(job));
}
-bool job_is_cancelled(Job *job)
+bool job_is_cancelled_locked(Job *job)
{
/* force_cancel may be true only if cancelled is true, too */
assert(job->cancelled || !job->force_cancel);
return job->force_cancel;
}
-bool job_cancel_requested(Job *job)
+bool job_is_cancelled(Job *job)
+{
+ JOB_LOCK_GUARD();
+ return job_is_cancelled_locked(job);
+}
+
+/* Called with job_mutex held. */
+static bool job_cancel_requested_locked(Job *job)
{
return job->cancelled;
}
-bool job_is_ready(Job *job)
+bool job_cancel_requested(Job *job)
+{
+ JOB_LOCK_GUARD();
+ return job_cancel_requested_locked(job);
+}
+
+bool job_is_ready_locked(Job *job)
{
switch (job->status) {
case JOB_STATUS_UNDEFINED:
@@ -270,7 +283,13 @@ bool job_is_ready(Job *job)
return false;
}
-bool job_is_completed(Job *job)
+bool job_is_ready(Job *job)
+{
+ JOB_LOCK_GUARD();
+ return job_is_ready_locked(job);
+}
+
+bool job_is_completed_locked(Job *job)
{
switch (job->status) {
case JOB_STATUS_UNDEFINED:
@@ -292,6 +311,12 @@ bool job_is_completed(Job *job)
return false;
}
+bool job_is_completed(Job *job)
+{
+ JOB_LOCK_GUARD();
+ return job_is_completed_locked(job);
+}
+
static bool job_started(Job *job)
{
return job->co;
@@ -521,7 +546,8 @@ static void coroutine_fn job_do_yield(Job *job,
uint64_t ns)
assert(job->busy);
}
-void coroutine_fn job_pause_point(Job *job)
+/* Called with job_mutex held, but releases it temporarily. */
+static void coroutine_fn job_pause_point_locked(Job *job)
{
assert(job && job_started(job));
In this function, we should now use job_pause_point_locked(), otherwise
it looks incorrect. (I remember that lock is noop for now, but still,
let's keep think as correct as possible)