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

[Paolo Bonzini]

On 4/1/22 10:05, Emanuele Giuseppe Esposito wrote:
> > 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.

You're right, CPU_FOREACH only appears in start_exclusive; so likewise 
you only need to walk the list in bdrv_graph_wrlock, i.e. only under BQL.

My thought was that, within the implementation, you'll need a mutex to 
protect has_waiter, and protecting the list with the same mutex made 
sense to me.  But indeed it's not necessary.

Paolo

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