| Message ID | 20220314133707.2206082-6-eesposit@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | job: replace AioContext lock with job_mutex | expand |
Am 14.03.2022 um 14:36 hat Emanuele Giuseppe Esposito geschrieben: > 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*. > > Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> > --- > include/qemu/job.h | 25 +++++++++++++++++++++--- > job.c | 48 ++++++++++++++++++++++++++++++++++++++++------ > 2 files changed, 64 insertions(+), 9 deletions(-) > > diff --git a/include/qemu/job.h b/include/qemu/job.h > index 6000463126..aa33d091b1 100644 > --- a/include/qemu/job.h > +++ b/include/qemu/job.h > @@ -473,21 +473,40 @@ const char *job_type_str(const Job *job); > /** Returns true if the job should not be visible to the management layer. */ > bool job_is_internal(Job *job); > > -/** Returns whether the job is being cancelled. */ > +/** > + * Returns whether the job is being cancelled. > + * Called with job_mutex *not* held. > + */ > bool job_is_cancelled(Job *job); > > +/** Just like job_is_cancelled, but called between job_lock and job_unlock */ > +bool job_is_cancelled_locked(Job *job); > + > /** > * Returns whether the job is scheduled for cancellation (at an > * indefinite point). > + * Called with job_mutex *not* held. > */ > bool job_cancel_requested(Job *job); > > -/** Returns whether the job is in a completed state. */ > +/** > + * Returns whether the job is in a completed state. > + * Called with job_mutex *not* held. > + */ > bool job_is_completed(Job *job); > > -/** Returns whether the job is ready to be completed. */ > +/** Same as job_is_completed(), but assumes job_lock is held. */ > +bool job_is_completed_locked(Job *job); Any reason why this comment is phrased differently than for job_is_cancelled_locked()? I don't mind which one we use, but if they should express the same thing, it would be better to have the same wording. If they should express different things, it need to be clearer what they are. Also, I assume job_mutex is meant because job_lock() is a function, not the lock that is held. > +/** > + * Returns whether the job is ready to be completed. > + * Called with job_mutex *not* held. > + */ > bool job_is_ready(Job *job); > > +/** Same as job_is_ready(), but assumes job_lock is held. */ > +bool job_is_ready_locked(Job *job); Same as above. > /** > * 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 Kevin
Am 03/06/2022 um 18:17 schrieb Kevin Wolf: > Am 14.03.2022 um 14:36 hat Emanuele Giuseppe Esposito geschrieben: >> 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*. >> >> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> >> --- >> include/qemu/job.h | 25 +++++++++++++++++++++--- >> job.c | 48 ++++++++++++++++++++++++++++++++++++++++------ >> 2 files changed, 64 insertions(+), 9 deletions(-) >> >> diff --git a/include/qemu/job.h b/include/qemu/job.h >> index 6000463126..aa33d091b1 100644 >> --- a/include/qemu/job.h >> +++ b/include/qemu/job.h >> @@ -473,21 +473,40 @@ const char *job_type_str(const Job *job); >> /** Returns true if the job should not be visible to the management layer. */ >> bool job_is_internal(Job *job); >> >> -/** Returns whether the job is being cancelled. */ >> +/** >> + * Returns whether the job is being cancelled. >> + * Called with job_mutex *not* held. >> + */ >> bool job_is_cancelled(Job *job); >> >> +/** Just like job_is_cancelled, but called between job_lock and job_unlock */ >> +bool job_is_cancelled_locked(Job *job); >> + >> /** >> * Returns whether the job is scheduled for cancellation (at an >> * indefinite point). >> + * Called with job_mutex *not* held. >> */ >> bool job_cancel_requested(Job *job); >> >> -/** Returns whether the job is in a completed state. */ >> +/** >> + * Returns whether the job is in a completed state. >> + * Called with job_mutex *not* held. >> + */ >> bool job_is_completed(Job *job); >> >> -/** Returns whether the job is ready to be completed. */ >> +/** Same as job_is_completed(), but assumes job_lock is held. */ >> +bool job_is_completed_locked(Job *job); > > Any reason why this comment is phrased differently than for > job_is_cancelled_locked()? I don't mind which one we use, but if they > should express the same thing, it would be better to have the same > wording. If they should express different things, it need to be clearer > what they are. > Makes sense, I will switch to the same format as job_is_cancelled_locked(). Emanuele > Also, I assume job_mutex is meant because job_lock() is a function, not > the lock that is held. > >> +/** >> + * Returns whether the job is ready to be completed. >> + * Called with job_mutex *not* held. >> + */ >> bool job_is_ready(Job *job); >> >> +/** Same as job_is_ready(), but assumes job_lock is held. */ >> +bool job_is_ready_locked(Job *job); > > Same as above. > >> /** >> * 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 > > Kevin >
On Mon, Mar 14, 2022 at 09:36:54AM -0400, 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*. > > Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> > --- > include/qemu/job.h | 25 +++++++++++++++++++++--- > job.c | 48 ++++++++++++++++++++++++++++++++++++++++------ > 2 files changed, 64 insertions(+), 9 deletions(-) Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
diff --git a/include/qemu/job.h b/include/qemu/job.h index 6000463126..aa33d091b1 100644 --- a/include/qemu/job.h +++ b/include/qemu/job.h @@ -473,21 +473,40 @@ const char *job_type_str(const Job *job); /** Returns true if the job should not be visible to the management layer. */ bool job_is_internal(Job *job); -/** Returns whether the job is being cancelled. */ +/** + * Returns whether the job is being cancelled. + * Called with job_mutex *not* held. + */ bool job_is_cancelled(Job *job); +/** Just like job_is_cancelled, but called between job_lock and job_unlock */ +bool job_is_cancelled_locked(Job *job); + /** * Returns whether the job is scheduled for cancellation (at an * indefinite point). + * Called with job_mutex *not* held. */ bool job_cancel_requested(Job *job); -/** Returns whether the job is in a completed state. */ +/** + * Returns whether the job is in a completed state. + * Called with job_mutex *not* held. + */ bool job_is_completed(Job *job); -/** Returns whether the job is ready to be completed. */ +/** Same as job_is_completed(), but assumes job_lock is held. */ +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); +/** Same as job_is_ready(), but assumes job_lock is held. */ +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..853f42050e 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 temporarly. */ +static void coroutine_fn job_pause_point_locked(Job *job) { assert(job && job_started(job)); @@ -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(); job_ref(job); aio_context_acquire(job->aio_context);
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*. Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> --- include/qemu/job.h | 25 +++++++++++++++++++++--- job.c | 48 ++++++++++++++++++++++++++++++++++++++++------ 2 files changed, 64 insertions(+), 9 deletions(-)