[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [RFC PATCH 2/6] job: _locked functions and public job_lock/unlock fo

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.


- /* List of jobs */
+    /* List of jobs. Protected by job_mutex. */
      QLIST_HEAD(, Job) jobs;
- /* Reference count */
+    /* Reference count. Atomic. */
      int refcnt;


reply via email to

[Prev in Thread] Current Thread [Next in Thread]