[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 2/9] block-copy: add missing coroutine_fn annotations
From: |
Emanuele Giuseppe Esposito |
Subject: |
Re: [PATCH v2 2/9] block-copy: add missing coroutine_fn annotations |
Date: |
Tue, 15 Nov 2022 16:41:24 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.2.0 |
To sum up on what was discussed in this serie, I don't really see any
strong objection against these patches, so I will soon send v3 which is
pretty much the same except for patch 1, which will be removed.
I think these patches are useful and will be even more meaningful to the
reviewer when in the next few days I send all the rwlock patches.
What has been discussed so far (using QEMU_IN_COROUTINE, using some sort
of tool to automate everything, etc.) has been noted and as I understand
will be researched by Alberto.
Thank you,
Emanuele
Am 10/11/2022 um 11:52 schrieb Paolo Bonzini:
> On Wed, Nov 9, 2022 at 1:24 PM Emanuele Giuseppe Esposito
> <eesposit@redhat.com> wrote:
>>>> What I do know is that it's extremely confusing to understand if a
>>>> function that is *not* marked as coroutine_fn is actually being called
>>>> also from coroutines or not.
>
> Agreed. This is a huge point in favor of pushing coroutine wrappers as
> far up in the call stack as possible, because it means more
> coroutine_fns and fewer mixed functions.
>
>>> This is a lot better than our "coroutine_fn" sign, which actually do no
>>> check (and can't do). Don't you plan to swap a "coroutine_fn" noop
>>> marker with more meaningful IN_COROUTINE(); (or something like this,
>>> which just do assert(qemu_in_coroutine())) at start of the function? It
>>> would be a lot safer.
>>
>> CCing also Alberto and Paolo
>>
>> So basically I think what we need is something that scans the whole
>> block layer code and puts the right coroutine_fn annotations (or
>> assertions, if you want) in the right places.
>
> coroutine_fn markers are done by Alberto's static analyzer, which I
> used to add coroutine_fn pretty much everywhere in the code base where
> they are *needed*. My rules are simple:
>
> * there MUST be no calls from non-coroutine_fn to coroutine_fn, this is
> obvious
>
> * there MUST be no blocking in coroutine_fn
>
> * there SHOULD be no calls from coroutine_fn to generated_co_wrapper;
> use the wrapped *_co_* function directly instead.
>
> To catch the last one, or possibly the last two, Alberto added
> no_coroutine_fn. In a perfect world non-marked functions would be
> "valid either in coroutine or non-coroutine function": they would call
> neither coroutine_fns nor no_coroutine_fns.
>
> This is unfortunately easier said than done, but in order to move
> towards that case, I think we can look again at vrc and extend it with
> new commands. Alberto's work covers *local* tests, looking at one
> caller and one callee at a time. With vrc's knowledge of the global
> call graph, we can find *all* paths from a coroutine_fn to a
> generated_co_wrapper, including those that go through unmarked
> functions. Then there are two cases:
>
> * if the unmarked function is never called from outside a coroutine,
> call the wrapped function and change it to coroutine_fn
>
> * if the unmarked function can be called from outside a coroutine,
> change it to a coroutine_fn (renaming it) and add a
> generated_co_wrapper. Rinse and repeat.
>
>> However, it would be nice to assign this to someone and do this
>> automatically, not doing it by hand. I am not sure if Alberto static
>> analyzer is currently capable of doing that.
>
> I think the first step has to be done by hand, because it entails
> creating new generated_co_wrappers. Checking for regressions can then
> be done automatically though.
>
> Paolo
>
- Re: [PATCH v2 1/9] block: call bdrv_co_drain_begin in a coroutine, (continued)
- [PATCH v2 9/9] block/dirty-bitmap: remove unnecessary qemu_in_coroutine() case, Emanuele Giuseppe Esposito, 2022/11/04
- [PATCH v2 2/9] block-copy: add missing coroutine_fn annotations, Emanuele Giuseppe Esposito, 2022/11/04
- Re: [PATCH v2 2/9] block-copy: add missing coroutine_fn annotations, Vladimir Sementsov-Ogievskiy, 2022/11/08
- Re: [PATCH v2 2/9] block-copy: add missing coroutine_fn annotations, Emanuele Giuseppe Esposito, 2022/11/08
- Re: [PATCH v2 2/9] block-copy: add missing coroutine_fn annotations, Vladimir Sementsov-Ogievskiy, 2022/11/08
- Re: [PATCH v2 2/9] block-copy: add missing coroutine_fn annotations, Vladimir Sementsov-Ogievskiy, 2022/11/08
- Re: [PATCH v2 2/9] block-copy: add missing coroutine_fn annotations, Emanuele Giuseppe Esposito, 2022/11/09
- Re: [PATCH v2 2/9] block-copy: add missing coroutine_fn annotations, Alberto Faria, 2022/11/09
- Re: [PATCH v2 2/9] block-copy: add missing coroutine_fn annotations, Paolo Bonzini, 2022/11/10
- Re: [PATCH v2 2/9] block-copy: add missing coroutine_fn annotations,
Emanuele Giuseppe Esposito <=
- Re: [PATCH v2 2/9] block-copy: add missing coroutine_fn annotations, Paolo Bonzini, 2022/11/16
[PATCH v2 3/9] nbd/server.c: add missing coroutine_fn annotations, Emanuele Giuseppe Esposito, 2022/11/04
[PATCH v2 4/9] block-backend: replace bdrv_*_above with blk_*_above, Emanuele Giuseppe Esposito, 2022/11/04
[PATCH v2 6/9] block/vmdk: add missing coroutine_fn annotations, Emanuele Giuseppe Esposito, 2022/11/04
[PATCH v2 8/9] block: bdrv_create is never called in non-coroutine context, Emanuele Giuseppe Esposito, 2022/11/04
[PATCH v2 7/9] block: bdrv_create_file is a coroutine_fn, Emanuele Giuseppe Esposito, 2022/11/04
Re: [PATCH v2 0/9] Still more coroutine and various fixes in block layer, Paolo Bonzini, 2022/11/04