|
From: | Emanuele Giuseppe Esposito |
Subject: | Re: [RFC PATCH 2/6] job: _locked functions and public job_lock/unlock for next patch |
Date: | Mon, 12 Jul 2021 10:43:07 +0200 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.10.1 |
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.
[Prev in Thread] | Current Thread | [Next in Thread] |