Re: [PATCH v7 05/18] job.h: add _locked duplicates for job API functions called with and without job_mutex

[Vladimir Sementsov-Ogievskiy]

On 6/22/22 17:26, Emanuele Giuseppe Esposito wrote:

Am 21/06/2022 um 17:03 schrieb Vladimir Sementsov-Ogievskiy:

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 <stefa...@redhat.com>
Signed-off-by: Emanuele Giuseppe Esposito <eespo...@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)

I miss your point here. What is incorrect?

Function called with lock held. But it calls job_pause_point(), which do lock the mutex. 
This will deadlock. That doesn't deadlock only because our mutex is noop. That's why I 
say "it looks incorrect".

And job_do_yield() takes lock by itself. How to resolve it?

You mean the real_job_lock/unlock taken in job_do_yield?

Yes.. Hmm, but we can consider real_job_lock as something other, that can be 
taken under job_lock.

   @@ -552,6 +578,12 @@ void coroutine_fn job_pause_point(Job *job)
       }
   }
   +void coroutine_fn job_pause_point(Job *job)
+{
+    JOB_LOCK_GUARD();
+    job_pause_point_locked(job);
+}
+
   void job_yield(Job *job)
   {
       assert(job->busy);
@@ -949,11 +981,15 @@ static void job_completed(Job *job)
       }
   }
   -/** Useful only as a type shim for aio_bh_schedule_oneshot. */
+/**
+ * Useful only as a type shim for aio_bh_schedule_oneshot.
+ * Called with job_mutex *not* held.
+ */
   static void job_exit(void *opaque)
   {
       Job *job = (Job *)opaque;
       AioContext *ctx;
+    JOB_LOCK_GUARD();

That's not part of this patch.. Doesn't relate to "add _locked duplicates"

         job_ref(job);
       aio_context_acquire(job->aio_context);

--
Best regards,
Vladimir