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?
>
> And job_do_yield() takes lock by itself. How to resolve it?
You mean the real_job_lock/unlock taken in job_do_yield?
>
>> @@ -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);
>
>