Re: [RFC PATCH v2 0/8] Removal of AioContext lock, bs->parents and ->children: new rwlock

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

Am 30/04/2022 um 07:17 schrieb Stefan Hajnoczi:
> On Thu, Apr 28, 2022 at 11:56:09PM +0200, Emanuele Giuseppe Esposito wrote:
>>
>>
>> Am 28/04/2022 um 12:45 schrieb Stefan Hajnoczi:
>>> On Wed, Apr 27, 2022 at 08:55:35AM +0200, Emanuele Giuseppe Esposito wrote:
>>>>
>>>>
>>>> Am 26/04/2022 um 10:51 schrieb Emanuele Giuseppe Esposito:
>>>>> Luckly, most of the cases where we recursively go through a graph are
>>>>> the BlockDriverState callback functions in block_int-common.h
>>>>> In order to understand what to protect, I categorized the callbacks in
>>>>> block_int-common.h depending on the type of function that calls them:
>>>>>
>>>>> 1) If the caller is a generated_co_wrapper, this function must be
>>>>>    protected by rdlock. The reason is that generated_co_wrapper create
>>>>>    coroutines that run in the given bs AioContext, so it doesn't matter
>>>>>    if we are running in the main loop or not, the coroutine might run
>>>>>    in an iothread.
>>>>> 2) If the caller calls it directly, and has the GLOBAL_STATE_CODE() macro,
>>>>>    then the function is safe. The main loop is the writer and thus won't
>>>>>    read and write at the same time.
>>>>> 3) If the caller calls it directly, but has not the GLOBAL_STATE_CODE()
>>>>>    macro, then we need to check the callers and see case-by-case if the
>>>>>    caller is in the main loop, if it needs to take the lock, or delegate
>>>>>    this duty to its caller (to reduce the places where to take it).
>>>>>
>>>>> I used the vrc script (https://github.com/bonzini/vrc) to get help finding
>>>>> all the callers of a callback. Using its filter function, I can
>>>>> omit all functions protected by the added lock to avoid having duplicates
>>>>> when querying for new callbacks.
>>>>
>>>> I was wondering, if a function is in category (3) and runs in an
>>>> Iothread but the function itself is not (currently) recursive, meaning
>>>> it doesn't really traverse the graph or calls someone that traverses it,
>>>> should I add the rdlock anyways or not?
>>>>
>>>> Example: bdrv_co_drain_end
>>>>
>>>> Pros:
>>>>    + Covers if in future a new recursive callback for a new/existing
>>>>      BlockDriver is implemented.
>>>>    + Covers also the case where I or someone missed the recursive part.
>>>>
>>>> Cons:
>>>>    - Potentially introducing an unnecessary critical section.
>>>>
>>>> What do you think?
>>>
>>> ->bdrv_co_drain_end() is a callback function. Do you mean whether its
>>> caller, bdrv_drain_invoke_entry(), should take the rdlock around
>>> ->bdrv_co_drain_end()?
>>
>> Yes. The problem is that the coroutine is created in bs AioContext, so
>> it might be in an iothread.
>>
>>>
>>> Going up further in the call chain (and maybe switching threads),
>>> bdrv_do_drained_end() has QLIST_FOREACH(child, &bs->children, next) so
>>> it needs protection. If the caller of bdrv_do_drained_end() holds then
>>> rdlock then I think none of the child functions (including
>>> ->bdrv_co_drain_end()) need to take it explicitly.
>>
>> Regarding bdrv_do_drained_end and similar, they are either running in
>> the main loop (or they will be, if coming from a coroutine) or in the
>> iothread running the AioContext of the bs involved.
>>
>> I think that most of the drains except for mirror.c are coming from main
>> loop. I protected mirror.c in patch 8, even though right now I am not
>> really sure that what I did is necessary, since the bh will be scheduled
>> in the main loop.
>>
>> Therefore we don't really need locks around drains.
> 
> Are you saying rdlock isn't necessary in the main loop because nothing
> can take the wrlock while our code is executing in the main loop?

Yes, that's the idea.
If I am not mistaken (and I hope I am not), only the main loop currently
modifies/is allowed to modify the graph.

The only case where currently we need to take the rdlock in main loop is
when we have the case

simplified_flush_callback(bs) {
	for (child in bs)
		bdrv_flush(child->bs);
}

some_function() {
	GLOBAL_STATE_CODE();
	/* assume bdrv_get_aio_context(bs) != qemu_in_main_thread() */

	bdrv_flush(bs);
		co = coroutine_create(bdrv_get_aio_context(bs))
		qemu_coroutine_enter(co, simplified_flush_callback)
}
> 
> Stefan
>