| Message ID | 20220314133707.2206082-3-eesposit@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | job: replace AioContext lock with job_mutex | expand |
Am 14.03.2022 um 14:36 hat Emanuele Giuseppe Esposito geschrieben: > 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> I suppose it might be a result of moving things back and forth between patches, but this patch doesn't really define separate categories. > 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 */ This is clearly a comment starting a category, but I can't see any other comment indicating that another category would start. > /** 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; > + > + And the end of the series, this is where the cutoff is and the rest is: /** Protected by job_mutex */ With this in mind, it seems correct to me that everything above progress is indeed never changed after creating the job. Of course, it's hard to tell without looking at the final result, so if you have to respin for some reason, it would be good to mark the end of the section more clearly for the intermediate state to make sense. Kevin
Am 03/06/2022 um 18:00 schrieb Kevin Wolf: > Am 14.03.2022 um 14:36 hat Emanuele Giuseppe Esposito geschrieben: >> 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> > > I suppose it might be a result of moving things back and forth between > patches, but this patch doesn't really define separate categories. > >> 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 */ > > This is clearly a comment starting a category, but I can't see any other > comment indicating that another category would start. > >> /** 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; >> + >> + > > And the end of the series, this is where the cutoff is and the rest is: > > /** Protected by job_mutex */ > > With this in mind, it seems correct to me that everything above progress > is indeed never changed after creating the job. Of course, it's hard to > tell without looking at the final result, so if you have to respin for > some reason, it would be good to mark the end of the section more > clearly for the intermediate state to make sense. How can I do that? I left two empty lines in this patch, I don't know what to use to signal the end of this category. Emanuele
On 6/7/22 15:20, Emanuele Giuseppe Esposito wrote: > > > Am 03/06/2022 um 18:00 schrieb Kevin Wolf: >> Am 14.03.2022 um 14:36 hat Emanuele Giuseppe Esposito geschrieben: >>> 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> >> >> I suppose it might be a result of moving things back and forth between >> patches, but this patch doesn't really define separate categories. >> >>> 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 */ >> >> This is clearly a comment starting a category, but I can't see any other >> comment indicating that another category would start. >> >>> /** 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; >>> + >>> + >> >> And the end of the series, this is where the cutoff is and the rest is: >> >> /** Protected by job_mutex */ >> >> With this in mind, it seems correct to me that everything above progress >> is indeed never changed after creating the job. Of course, it's hard to >> tell without looking at the final result, so if you have to respin for >> some reason, it would be good to mark the end of the section more >> clearly for the intermediate state to make sense. > > How can I do that? I left two empty lines in this patch, I don't know > what to use to signal the end of this category. Can you already add "/** Protected by AioContext lock */" in this patch and then change it later? Paolo
Am 07/06/2022 um 17:41 schrieb Paolo Bonzini: > On 6/7/22 15:20, Emanuele Giuseppe Esposito wrote: >> >> >> Am 03/06/2022 um 18:00 schrieb Kevin Wolf: >>> Am 14.03.2022 um 14:36 hat Emanuele Giuseppe Esposito geschrieben: >>>> 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> >>> >>> I suppose it might be a result of moving things back and forth between >>> patches, but this patch doesn't really define separate categories. >>> >>>> 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 */ >>> >>> This is clearly a comment starting a category, but I can't see any other >>> comment indicating that another category would start. >>> >>>> /** 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; >>>> + >>>> + >>> >>> And the end of the series, this is where the cutoff is and the rest is: >>> >>> /** Protected by job_mutex */ >>> >>> With this in mind, it seems correct to me that everything above progress >>> is indeed never changed after creating the job. Of course, it's hard to >>> tell without looking at the final result, so if you have to respin for >>> some reason, it would be good to mark the end of the section more >>> clearly for the intermediate state to make sense. >> >> How can I do that? I left two empty lines in this patch, I don't know >> what to use to signal the end of this category. > > Can you already add "/** Protected by AioContext lock */" in this patch > and then change it later? Makes sense, thanks. Emanuele > > Paolo >
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(-)