| Message ID | 20220208143513.1077229-3-eesposit@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | job: replace AioContext lock with job_mutex | expand |
On Tue, Feb 08, 2022 at 09:34:55AM -0500, 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 | 59 ++++++++++++++++++++++++++-------------------- > 1 file changed, 34 insertions(+), 25 deletions(-) > > diff --git a/include/qemu/job.h b/include/qemu/job.h > index d1192ffd61..86ec46c09e 100644 > --- a/include/qemu/job.h > +++ b/include/qemu/job.h > @@ -40,27 +40,50 @@ typedef struct JobTxn JobTxn; > * Long-running operation. > */ > typedef struct Job { > + > + /* Fields set at initialization (job_create), and never modified */ Is there a corresponding "Field protected by job_mutex" comment that separates fields that need locking?
On 10/02/2022 16:40, Stefan Hajnoczi wrote: > On Tue, Feb 08, 2022 at 09:34:55AM -0500, 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 | 59 ++++++++++++++++++++++++++-------------------- >> 1 file changed, 34 insertions(+), 25 deletions(-) >> >> diff --git a/include/qemu/job.h b/include/qemu/job.h >> index d1192ffd61..86ec46c09e 100644 >> --- a/include/qemu/job.h >> +++ b/include/qemu/job.h >> @@ -40,27 +40,50 @@ typedef struct JobTxn JobTxn; >> * Long-running operation. >> */ >> typedef struct Job { >> + >> + /* Fields set at initialization (job_create), and never modified */ > > Is there a corresponding "Field protected by job_mutex" comment that > separates fields that need locking? > That would be the comment /** Protected by job_mutex */ situated right after the field "ProgressMeter progress;". Do you want me to change it in "Fields protected by job_mutex"? Thank you, Emanuele
On Thu, Feb 10, 2022 at 05:26:52PM +0100, Emanuele Giuseppe Esposito wrote: > > > On 10/02/2022 16:40, Stefan Hajnoczi wrote: > > On Tue, Feb 08, 2022 at 09:34:55AM -0500, 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 | 59 ++++++++++++++++++++++++++-------------------- > >> 1 file changed, 34 insertions(+), 25 deletions(-) > >> > >> diff --git a/include/qemu/job.h b/include/qemu/job.h > >> index d1192ffd61..86ec46c09e 100644 > >> --- a/include/qemu/job.h > >> +++ b/include/qemu/job.h > >> @@ -40,27 +40,50 @@ typedef struct JobTxn JobTxn; > >> * Long-running operation. > >> */ > >> typedef struct Job { > >> + > >> + /* Fields set at initialization (job_create), and never modified */ > > > > Is there a corresponding "Field protected by job_mutex" comment that > > separates fields that need locking? > > > > That would be the comment > > /** Protected by job_mutex */ > > situated right after the field "ProgressMeter progress;". > > Do you want me to change it in "Fields protected by job_mutex"? I don't see it: + /** The opaque value that is passed to the completion function. */ + void *opaque; + + /* ProgressMeter API is thread-safe */ + ProgressMeter progress; + + + /** AioContext to run the job coroutine in */ + AioContext *aio_context; + + /** Reference count of the block job */ + int refcnt; Is it added by a later patch or did I miss it? Stefan
On 10/02/2022 18:35, Stefan Hajnoczi wrote: > On Thu, Feb 10, 2022 at 05:26:52PM +0100, Emanuele Giuseppe Esposito wrote: >> >> >> On 10/02/2022 16:40, Stefan Hajnoczi wrote: >>> On Tue, Feb 08, 2022 at 09:34:55AM -0500, 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 | 59 ++++++++++++++++++++++++++-------------------- >>>> 1 file changed, 34 insertions(+), 25 deletions(-) >>>> >>>> diff --git a/include/qemu/job.h b/include/qemu/job.h >>>> index d1192ffd61..86ec46c09e 100644 >>>> --- a/include/qemu/job.h >>>> +++ b/include/qemu/job.h >>>> @@ -40,27 +40,50 @@ typedef struct JobTxn JobTxn; >>>> * Long-running operation. >>>> */ >>>> typedef struct Job { >>>> + >>>> + /* Fields set at initialization (job_create), and never modified */ >>> >>> Is there a corresponding "Field protected by job_mutex" comment that >>> separates fields that need locking? >>> >> >> That would be the comment >> >> /** Protected by job_mutex */ >> >> situated right after the field "ProgressMeter progress;". >> >> Do you want me to change it in "Fields protected by job_mutex"? > > I don't see it: > > + /** The opaque value that is passed to the completion function. */ > + void *opaque; > + > + /* ProgressMeter API is thread-safe */ > + ProgressMeter progress; > + > + > + /** AioContext to run the job coroutine in */ > + AioContext *aio_context; > + > + /** Reference count of the block job */ > + int refcnt; > > Is it added by a later patch or did I miss it? > Yes sorry I forgot: it is added in patch 19, when the lock is not a nop anymore. I think this was suggested in one of the previous reviews, to avoid having a misleading comment when the fields are not yet protected. Emanuele
diff --git a/include/qemu/job.h b/include/qemu/job.h index d1192ffd61..86ec46c09e 100644 --- a/include/qemu/job.h +++ b/include/qemu/job.h @@ -40,27 +40,50 @@ 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; + + + /** 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 +135,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 +149,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 +176,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 +482,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 | 59 ++++++++++++++++++++++++++-------------------- 1 file changed, 34 insertions(+), 25 deletions(-)