[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 2/9] block-copy: add missing coroutine_fn annotations
From: |
Emanuele Giuseppe Esposito |
Subject: |
Re: [PATCH 2/9] block-copy: add missing coroutine_fn annotations |
Date: |
Fri, 4 Nov 2022 08:35:08 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.2.0 |
Am 03/11/2022 um 19:36 schrieb Paolo Bonzini:
> On 11/3/22 19:06, Kevin Wolf wrote:
>> I think it can make sense to have coroutine_fn as a documentation for
>> things that are only ever called in a coroutine even if they could
>> theoretically also work outside of coroutine context.
>>
>> Otherwise, when we want to introduce a coroutine_fn call somewhere, it's
>> not only less obvious that it's even possible to do, but we'll have to
>> add potentially many additional coroutine_fn annotations in the whole
>> call chain in an otherwise unrelated patch.
>
> This is true. On the other hand, coroutine_fn also means "this is
> allowed to suspend", which may have implications on the need for locking
> in the caller. So you need to judge case-by-case.
>
> If there are good reasons to add the note, you could add an assertion
> that you are qemu_in_coroutine(), which notes that you are in a
> coroutine but you don't suspend. In this case however I don't think
> it's likely that there will be a coroutine_fn call added later.
>
> I guess I tend to err on the side of "it's good that it's not obvious
> that you can call a coroutine_fn", but I also need to correct myself as
> qemu_coroutine_yield() is not the only leaf; there is also
> qemu_co_queue_next() and qemu_co_queue_restart_all(), which are
> coroutine_fn because they rely on the queuing behavior of
> aio_co_enter(). In this case I violated my own rule because it is
> always a bug to call these functions outside coroutine context.
>
But isn't it a bug also not to mark a function _only_ called by
coroutine_fn? My point is that if this function is an implementation of
a BlockDriver callback marked as coroutine_fn (like in patch 6 with
vmdk), then it would make sense.
This is actually the point of this serie (which I might not have
explained well in the cover letter), every function marked here is
eventually called by/calling a BlockDriver callback marked as coroutine_fn.
Currently we have something like this:
BlockDriver {
void coroutine_fn (*bdrv_A)(void) = implA;
}
void coroutine_fn implA() {
funcB();
funcC();
}
void funcB() {}; <--- missing coroutine_fn?
void funcC() {}; <--- missing coroutine_fn?
In addition, as I understand draining is not allowed in coroutines. If a
function/callback only running in coroutines is not marked as
coroutine_fn, then it will be less obvious to notice that draining is
not allowed there.
Emanuele
- [PATCH 7/9] block: bdrv_create_file is a coroutine_fn, (continued)
- [PATCH 7/9] block: bdrv_create_file is a coroutine_fn, Emanuele Giuseppe Esposito, 2022/11/03
- [PATCH 3/9] nbd/server.c: add missing coroutine_fn annotations, Emanuele Giuseppe Esposito, 2022/11/03
- [PATCH 8/9] block: bdrv_create is never called in non-coroutine context, Emanuele Giuseppe Esposito, 2022/11/03
- [PATCH 2/9] block-copy: add missing coroutine_fn annotations, Emanuele Giuseppe Esposito, 2022/11/03
- Re: [PATCH 2/9] block-copy: add missing coroutine_fn annotations, Paolo Bonzini, 2022/11/03
- Re: [PATCH 2/9] block-copy: add missing coroutine_fn annotations, Kevin Wolf, 2022/11/03
- Re: [PATCH 2/9] block-copy: add missing coroutine_fn annotations, Paolo Bonzini, 2022/11/03
- Re: [PATCH 2/9] block-copy: add missing coroutine_fn annotations,
Emanuele Giuseppe Esposito <=
- Re: [PATCH 2/9] block-copy: add missing coroutine_fn annotations, Paolo Bonzini, 2022/11/04
- Re: [PATCH 2/9] block-copy: add missing coroutine_fn annotations, Emanuele Giuseppe Esposito, 2022/11/04
- Re: [PATCH 2/9] block-copy: add missing coroutine_fn annotations, Paolo Bonzini, 2022/11/04
- Re: [PATCH 2/9] block-copy: add missing coroutine_fn annotations, Kevin Wolf, 2022/11/04
[PATCH 6/9] block/vmdk: add missing coroutine_fn annotations, Emanuele Giuseppe Esposito, 2022/11/03
[PATCH 9/9] block/dirty-bitmap: remove unnecessary qemu_in_coroutine() case, Emanuele Giuseppe Esposito, 2022/11/03