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


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

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