| Message ID | 20220616131835.2004262-3-eesposit@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | job: replace AioContext lock with job_mutex | expand |
On 6/16/22 16:18, Emanuele Giuseppe Esposito wrote: > Categorize the fields in struct Job to understand which ones > need to be protected by the job mutex and which don't. > > Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> > --- > include/qemu/job.h | 61 +++++++++++++++++++++++++++------------------- > 1 file changed, 36 insertions(+), 25 deletions(-) > > diff --git a/include/qemu/job.h b/include/qemu/job.h > index d1192ffd61..876e13d549 100644 > --- a/include/qemu/job.h > +++ b/include/qemu/job.h > @@ -40,27 +40,52 @@ typedef struct JobTxn JobTxn; > * Long-running operation. > */ > typedef struct Job { > + > + /* Fields set at initialization (job_create), and never modified */ > + > /** The ID of the job. May be NULL for internal jobs. */ > char *id; > > - /** The type of this job. */ > + /** > + * The type of this job. > + * All callbacks are called with job_mutex *not* held. > + */ > const JobDriver *driver; > > - /** Reference count of the block job */ > - int refcnt; > - > - /** Current state; See @JobStatus for details. */ > - JobStatus status; > - > - /** AioContext to run the job coroutine in */ > - AioContext *aio_context; > - > /** > * The coroutine that executes the job. If not NULL, it is reentered when > * busy is false and the job is cancelled. > + * Initialized in job_start() > */ > Coroutine *co; > > + /** True if this job should automatically finalize itself */ > + bool auto_finalize; > + > + /** True if this job should automatically dismiss itself */ > + bool auto_dismiss; > + > + /** The completion function that will be called when the job completes. */ > + BlockCompletionFunc *cb; > + > + /** The opaque value that is passed to the completion function. */ > + void *opaque; > + > + /* ProgressMeter API is thread-safe */ > + ProgressMeter progress; > + > + > + /** Protected by AioContext lock */ Previous groups stats with '/*'. Should /** be substituted by /* ? > + > + /** AioContext to run the job coroutine in */ > + AioContext *aio_context; Not sure how much is it protected. Probably we read it without locking. But that should go away anyway. > + > + /** Reference count of the block job */ > + int refcnt; > + > + /** Current state; See @JobStatus for details. */ > + JobStatus status; > + > /** > * Timer that is used by @job_sleep_ns. Accessed under job_mutex (in > * job.c). > @@ -112,14 +137,6 @@ typedef struct Job { > /** Set to true when the job has deferred work to the main loop. */ > bool deferred_to_main_loop; > > - /** True if this job should automatically finalize itself */ > - bool auto_finalize; > - > - /** True if this job should automatically dismiss itself */ > - bool auto_dismiss; > - > - ProgressMeter progress; > - > /** > * Return code from @run and/or @prepare callback(s). > * Not final until the job has reached the CONCLUDED status. > @@ -134,12 +151,6 @@ typedef struct Job { > */ > Error *err; > > - /** The completion function that will be called when the job completes. */ > - BlockCompletionFunc *cb; > - > - /** The opaque value that is passed to the completion function. */ > - void *opaque; > - > /** Notifiers called when a cancelled job is finalised */ > NotifierList on_finalize_cancelled; > > @@ -167,6 +178,7 @@ typedef struct Job { > > /** > * Callbacks and other information about a Job driver. > + * All callbacks are invoked with job_mutex *not* held. Should this be in this patch? Seems related. But will we have a lot more comments like this in later patches? > */ > struct JobDriver { > > @@ -472,7 +484,6 @@ void job_yield(Job *job); > */ > void coroutine_fn job_sleep_ns(Job *job, int64_t ns); > > - I'd drop this, looks like accidental unrelated style fixing. > /** Returns the JobType of a given Job. */ > JobType job_type(const Job *job); > as is, or with dropped 1-2 last hunks: Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
diff --git a/include/qemu/job.h b/include/qemu/job.h index d1192ffd61..876e13d549 100644 --- a/include/qemu/job.h +++ b/include/qemu/job.h @@ -40,27 +40,52 @@ typedef struct JobTxn JobTxn; * Long-running operation. */ typedef struct Job { + + /* Fields set at initialization (job_create), and never modified */ + /** The ID of the job. May be NULL for internal jobs. */ char *id; - /** The type of this job. */ + /** + * The type of this job. + * All callbacks are called with job_mutex *not* held. + */ const JobDriver *driver; - /** Reference count of the block job */ - int refcnt; - - /** Current state; See @JobStatus for details. */ - JobStatus status; - - /** AioContext to run the job coroutine in */ - AioContext *aio_context; - /** * The coroutine that executes the job. If not NULL, it is reentered when * busy is false and the job is cancelled. + * Initialized in job_start() */ Coroutine *co; + /** True if this job should automatically finalize itself */ + bool auto_finalize; + + /** True if this job should automatically dismiss itself */ + bool auto_dismiss; + + /** The completion function that will be called when the job completes. */ + BlockCompletionFunc *cb; + + /** The opaque value that is passed to the completion function. */ + void *opaque; + + /* ProgressMeter API is thread-safe */ + ProgressMeter progress; + + + /** Protected by AioContext lock */ + + /** AioContext to run the job coroutine in */ + AioContext *aio_context; + + /** Reference count of the block job */ + int refcnt; + + /** Current state; See @JobStatus for details. */ + JobStatus status; + /** * Timer that is used by @job_sleep_ns. Accessed under job_mutex (in * job.c). @@ -112,14 +137,6 @@ typedef struct Job { /** Set to true when the job has deferred work to the main loop. */ bool deferred_to_main_loop; - /** True if this job should automatically finalize itself */ - bool auto_finalize; - - /** True if this job should automatically dismiss itself */ - bool auto_dismiss; - - ProgressMeter progress; - /** * Return code from @run and/or @prepare callback(s). * Not final until the job has reached the CONCLUDED status. @@ -134,12 +151,6 @@ typedef struct Job { */ Error *err; - /** The completion function that will be called when the job completes. */ - BlockCompletionFunc *cb; - - /** The opaque value that is passed to the completion function. */ - void *opaque; - /** Notifiers called when a cancelled job is finalised */ NotifierList on_finalize_cancelled; @@ -167,6 +178,7 @@ typedef struct Job { /** * Callbacks and other information about a Job driver. + * All callbacks are invoked with job_mutex *not* held. */ struct JobDriver { @@ -472,7 +484,6 @@ void job_yield(Job *job); */ void coroutine_fn job_sleep_ns(Job *job, int64_t ns); - /** Returns the JobType of a given Job. */ JobType job_type(const Job *job);
Categorize the fields in struct Job to understand which ones need to be protected by the job mutex and which don't. Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> --- include/qemu/job.h | 61 +++++++++++++++++++++++++++------------------- 1 file changed, 36 insertions(+), 25 deletions(-)