Re: [PATCH v2 2/9] block-copy: add missing coroutine_fn annotations

[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