Re: [RFC PATCH 4/6] job.h: categorize job fields

2021-07-12 Thread Emanuele Giuseppe Esposito 




On 08/07/2021 13:02, Stefan Hajnoczi wrote:

On Wed, Jul 07, 2021 at 06:58:11PM +0200, Emanuele Giuseppe Esposito wrote:

-/** AioContext to run the job coroutine in */
+/**
+ * AioContext to run the job coroutine in.
+ * Atomic.
+ */
  AioContext *aio_context;


This isn't accessed using atomic operations, so I'm not sure why it's
documented as atomic?

Maybe this is unnecessary, but from what I understand right now when we 
want to change the AioContext of a child node, we need to acquire its 
AioContext lock, and then we try to set it. Without AioContext locks, my 
understanding is that this has to be protected somehow. Therefore I 
thought of setting this pointer atomically (actual code that does this 
is in patch 5).


Again same reasoning style applies here: this patch just adds a bounce 
of comments, but merging it with next one would just make it more 
unreadable.


Emanuele

Re: [RFC PATCH 4/6] job.h: categorize job fields

2021-07-08 Thread Stefan Hajnoczi 
On Wed, Jul 07, 2021 at 06:58:11PM +0200, Emanuele Giuseppe Esposito wrote:
> -/** AioContext to run the job coroutine in */
> +/**
> + * AioContext to run the job coroutine in.
> + * Atomic.
> + */
>  AioContext *aio_context;

This isn't accessed using atomic operations, so I'm not sure why it's
documented as atomic?


signature.asc
Description: PGP signature

[RFC PATCH 4/6] job.h: categorize job fields

2021-07-07 Thread Emanuele Giuseppe Esposito 
This makes it easier to understand what needs to be protected
by a lock and what doesn't.

Signed-off-by: Emanuele Giuseppe Esposito 
---
 include/qemu/job.h | 101 -
 1 file changed, 82 insertions(+), 19 deletions(-)

diff --git a/include/qemu/job.h b/include/qemu/job.h
index ba2f9b2660..4421d08d93 100644
--- a/include/qemu/job.h
+++ b/include/qemu/job.h
@@ -40,24 +40,40 @@ typedef struct JobTxn JobTxn;
  * Long-running operation.
  */
 typedef struct Job {
-/** The ID of the job. May be NULL for internal jobs. */
+/**
+ * The ID of the job. May be NULL for internal jobs.
+ * Set it in job_create and just read.
+ */
 char *id;
 
-/** The type of this job. */
+/**
+ * The type of this job.
+ * Set it in job_create and just read.
+ */
 const JobDriver *driver;
 
-/** Reference count of the block job */
+/**
+ * Reference count of the block job.
+ * Protected by job_mutex.
+ */
 int refcnt;
 
-/** Current state; See @JobStatus for details. */
+/**
+ * Current state; See @JobStatus for details.
+ * Protected by job_mutex.
+ */
 JobStatus status;
 
-/** AioContext to run the job coroutine in */
+/**
+ * AioContext to run the job coroutine in.
+ * Atomic.
+ */
 AioContext *aio_context;
 
 /**
  * The coroutine that executes the job.  If not NULL, it is reentered when
  * busy is false and the job is cancelled.
+ * Set it in job_create and just read.
  */
 Coroutine *co;
 
@@ -70,13 +86,15 @@ typedef struct Job {
 /**
  * Counter for pause request. If non-zero, the block job is either paused,
  * or if busy == true will pause itself as soon as possible.
+ * Protected by job_mutex.
  */
 int pause_count;
 
 /**
  * Set to false by the job while the coroutine has yielded and may be
  * re-entered by job_enter(). There may still be I/O or event loop activity
- * pending. Accessed under block_job_mutex (in blockjob.c).
+ * pending.
+ * Protected by job_mutex.
  *
  * When the job is deferred to the main loop, busy is true as long as the
  * bottom half is still pending.
@@ -86,12 +104,14 @@ typedef struct Job {
 /**
  * Set to true by the job while it is in a quiescent state, where
  * no I/O or event loop activity is pending.
+ * Protected by job_mutex.
  */
 bool paused;
 
 /**
  * Set to true if the job is paused by user.  Can be unpaused with the
  * block-job-resume QMP command.
+ * Protected by job_mutex.
  */
 bool user_paused;
 
@@ -100,22 +120,33 @@ typedef struct Job {
  * always be tested just before toggling the busy flag from false
  * to true.  After a job has been cancelled, it should only yield
  * if #aio_poll will ("sooner or later") reenter the coroutine.
+ * Protected by job_mutex.
  */
 bool cancelled;
 
 /**
  * Set to true if the job should abort immediately without waiting
  * for data to be in sync.
+ * Protected by job_mutex.
  */
 bool force_cancel;
 
-/** Set to true when the job has deferred work to the main loop. */
+/**
+ * Set to true when the job has deferred work to the main loop.
+ * Protected by job_mutex.
+ */
 bool deferred_to_main_loop;
 
-/** True if this job should automatically finalize itself */
+/**
+ * True if this job should automatically finalize itself.
+ * Set it in job_create and just read.
+ */
 bool auto_finalize;
 
-/** True if this job should automatically dismiss itself */
+/**
+ * True if this job should automatically dismiss itself.
+ * Set it in job_create and just read.
+ */
 bool auto_dismiss;
 
 ProgressMeter progress;
@@ -124,6 +155,7 @@ typedef struct Job {
  * Return code from @run and/or @prepare callback(s).
  * Not final until the job has reached the CONCLUDED status.
  * 0 on success, -errno on failure.
+ * Protected by job_mutex.
  */
 int ret;
 
@@ -131,37 +163,68 @@ typedef struct Job {
  * Error object for a failed job.
  * If job->ret is nonzero and an error object was not set, it will be set
  * to strerror(-job->ret) during job_completed.
+ * Protected by job_mutex.
  */
 Error *err;
 
-/** The completion function that will be called when the job completes.  */
+/**
+ * The completion function that will be called when the job completes.
+ * Set it in job_create and just read.
+ */
 BlockCompletionFunc *cb;
 
-/** The opaque value that is passed to the completion function.  */
+/**
+ * The opaque value that is passed to the completion function.
+ * Set it in job_create and just read.
+ */
 void *opaque;
 
-/** Notifiers called when a cancelled job is finalised */
+/**
+ * Notifiers called when a cancelled job is finalised.
+