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 > <eespo...@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 >
