Re: [RFC PATCH 0/5] Removal of AioContext lock, bs->parents and ->children: proof of concept

[Emanuele Giuseppe Esposito]

Am 31/03/2022 um 18:40 schrieb Paolo Bonzini:
> On 3/31/22 15:51, Emanuele Giuseppe Esposito wrote:
>>
>> bdrv_graph_list_wrlock <-> start_exclusive
>> bdrv_graph_list_wrunlock <-> end_exclusive
>> bdrv_graph_list_rdlock <-> cpu_exec_start
>> bdrv_graph_list_rdunlock <-> cpu_exec_end
> 
> This wouldn't protect the list but the whole graph, i.e. the parents and
> children of all BDSes.  So the functions would be:
> 
>   bdrv_graph_wrlock <-> start_exclusive
>   bdrv_graph_wrunlock <-> end_exclusive
>   bdrv_graph_rdlock <-> cpu_exec_start
>   bdrv_graph_rdunlock <-> cpu_exec_end
> 
> 
> The list itself would be used internally to implement the write-side
> lock and unlock primitives, but it would not be protected by the above
> functions.  So there would be a couple additional functions:
> 
>   bdrv_graph_list_lock <-> cpu_list_lock
>   bdrv_graph_list_unlock <-> cpu_list_unlock

The list would be graph_bdrv_states, why do we need to protect it with a
lock? Currently it is protected by BQL, and theoretically only
bdrv_graph_wrlock iterates on it. And as we defined in the assertion
below, wrlock is always in the main loop too.

> 
>> +void bdrv_graph_list_rdlock(BlockDriverState *bs);
>> +void bdrv_graph_list_rdunlock(BlockDriverState *bs);
> 
> Apart from the naming change, these two would be coroutine_fn.
> 
>> +#define BS_GRAPH_READER(bs) /* in main loop OR bs->reading_graph */
>> +#define BS_GRAPH_WRITER(bs) /* in main loop AND bs->bs_graph_pending_op
> 
> bs_graph_pending_op is not part of bs->, it is a global variable
> (corresponding to pending_cpus in cpus-common.c).  I would call it
> bs_graph_pending_reader since you have "has_writer" below.

Hmm maybe it is better to go with has_waiter and pending_op. This is
because as "pending" we always have 1 write and zero or more reads.

Rest makes sense.

Emanuele
> 
> Also, this second #define does not need an argument, and is really the
> same as assert_bdrv_graph_writable(bs).  So perhaps you can rename the
> first one to assert_bdrv_graph_readable(bs).
> 
>>
>> +    /*
>> +     * If true, the main loop is modifying the graph.
>> +     * bs cannot read the graph.
>> +     * Protected by bs_graph_list_lock.
>> +     */
>> +    bool has_writer;
> 
> Note that it's "has_waiter" in cpus-common.c. :)  has_writer is fine too.
> 
> Paolo
>