On 08/07/2021 12:50, Stefan Hajnoczi wrote:
On Wed, Jul 07, 2021 at 06:58:09PM +0200, Emanuele Giuseppe Esposito wrote:
diff --git a/job.c b/job.c
index 872bbebb01..96fb8e9730 100644
--- a/job.c
+++ b/job.c
@@ -32,6 +32,10 @@
#include "trace/trace-root.h"
#include "qapi/qapi-events-job.h"
+/* job_mutex protexts the jobs list, but also the job operations. */
+static QemuMutex job_mutex;
It's unclear what protecting "job operations" means. I would prefer a
fine-grained per-job lock that protects the job's fields instead of a
global lock with an unclear scope.
As I wrote in the cover letter, I wanted to try to keep things as simple
as possible with a global lock. It is possible to try and have a per-job
lock, but I don't know how complex will that be then.
I will try and see what I can do.
Maybe "job_mutex protexts the jobs list, but also makes the job API
thread-safe"?
+
+/* Protected by job_mutex */
static QLIST_HEAD(, Job) jobs = QLIST_HEAD_INITIALIZER(jobs);
/* Job State Transition Table */
@@ -64,27 +68,22 @@ bool JobVerbTable[JOB_VERB__MAX][JOB_STATUS__MAX] = {
/* Transactional group of jobs */
struct JobTxn {
- /* Is this txn being cancelled? */
+ /* Is this txn being cancelled? Atomic.*/
bool aborting;
The comment says atomic but this field is not accessed using atomic
operations (at least at this point in the patch series)?
Yes sorry I messed up the hunks in one-two patches. These comments were
supposed to be on patch 4 "job.h: categorize job fields". Even though
that might also not be ideal, since that patch just introduces the
comments, without applying the locking/protection yet.
On the other side, if I merge everything together in patch 5, it will be
even harder to read.
Emanuele
- /* List of jobs */
+ /* List of jobs. Protected by job_mutex. */
QLIST_HEAD(, Job) jobs;
- /* Reference count */
+ /* Reference count. Atomic. */
int refcnt;
Same.
