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

[Emanuele Giuseppe Esposito <eesposit@redhat.com>]

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.
>