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

[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

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

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