Re: [PATCH 2/2] block: Add no_coroutine_fn and coroutine_mixed_fn marker

[Kevin Wolf]

Am 15.12.2022 um 18:44 hat Paolo Bonzini geschrieben:
> From: Alberto Faria <afaria@redhat.com>
> 
> Add more annotations to functions, describing valid and invalid
> calls from coroutine to non-coroutine context.
> 
> When applied to a function, no_coroutine_fn advertises that it should
> not be called from coroutine_fn functions.  This can be because the
> function blocks or, in the case of generated_co_wrapper, to enforce
> that coroutine_fn functions directly call the coroutine_fn that backs
> the generated_co_wrapper.
> 
> coroutine_mixed_fn instead is for function that can be called in
> both coroutine and non-coroutine context, but will suspend when
> called in coroutine context.  Annotating them is a first step
> towards enforcing that non-annotated functions are absolutely
> not going to suspend.
> 
> These can be used for example with the vrc tool from
> https://github.com/bonzini/vrc:
> 
>     # find functions that *really* cannot be called from no_coroutine_fn
>     (vrc) load --loader clang 
> libblock.fa.p/meson-generated_.._block_block-gen.c.o
>     # The comma is an "AND".  The "path" here consists of a single node
>     (vrc) paths [no_coroutine_fn,!coroutine_mixed_fn]
>     bdrv_remove_persistent_dirty_bitmap
>     bdrv_create
>     bdrv_can_store_new_dirty_bitmap
> 
>     # find how coroutine_fns end up calling a mixed function
>     (vrc) load --loader clang --force libblock.fa.p/*.c.o
>     # regular expression search
>     (vrc) paths [coroutine_fn] [!no_coroutine_fn]* [coroutine_mixed_fn]
>     ...
>     bdrv_pread <- vhdx_log_write <- vhdx_log_write_and_flush <- vhdx_co_writev
>     ...
> 
> Signed-off-by: Alberto Faria <afaria@redhat.com>
> [Rebase, add coroutine_mixed_fn. - Paolo]
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  include/block/block-common.h | 11 +++++++----
>  include/qemu/coroutine.h     | 33 +++++++++++++++++++++++++++++++++
>  2 files changed, 40 insertions(+), 4 deletions(-)
> 
> diff --git a/include/block/block-common.h b/include/block/block-common.h
> index 4749c46a5e7e..cce79bd00135 100644
> --- a/include/block/block-common.h
> +++ b/include/block/block-common.h
> @@ -50,11 +50,14 @@
>   * - co_wrapper_mixed_bdrv_rdlock are co_wrapper_mixed functions but
>   *   automatically take and release the graph rdlock when creating a new
>   *   coroutine.
> + *
> + * These functions should not be called from a coroutine_fn; instead,
> + * call the wrapped function directly.
>   */
> -#define co_wrapper
> -#define co_wrapper_mixed
> -#define co_wrapper_bdrv_rdlock
> -#define co_wrapper_mixed_bdrv_rdlock
> +#define co_wrapper                     no_coroutine_fn
> +#define co_wrapper_mixed               no_coroutine_fn coroutine_mixed_fn
> +#define co_wrapper_bdrv_rdlock         no_coroutine_fn
> +#define co_wrapper_mixed_bdrv_rdlock   no_coroutine_fn coroutine_mixed_fn
>  
>  #include "block/dirty-bitmap.h"
>  #include "block/blockjob.h"
> diff --git a/include/qemu/coroutine.h b/include/qemu/coroutine.h
> index b0c97f6fb7ad..5f5ab8136a3a 100644
> --- a/include/qemu/coroutine.h
> +++ b/include/qemu/coroutine.h
> @@ -28,6 +28,27 @@
>   * These functions are re-entrant and may be used outside the global mutex.
>   */
>  
> +/**
> + * Mark a function that can suspend when executed in coroutine context,
> + * but can handle running in non-coroutine context too.
> + *
> + * Functions that execute in coroutine context cannot be called directly from
> + * normal functions.  In the future it would be nice to enable compiler or
> + * static checker support for catching such errors.  This annotation might 
> make
> + * it possible and in the meantime it serves as documentation.
> + *
> + * For example:
> + *
> + *   static void coroutine_fn foo(void) {

s/coroutine_fn/coroutine_mixed_fn/

> + *       ....
> + *   }
> + */
> +#ifdef __clang__
> +#define coroutine_mixed_fn 
> __attribute__((__annotate__("coroutine_mixed_fn")))
> +#else
> +#define coroutine_mixed_fn
> +#endif
> +
>  /**
>   * Mark a function that executes in coroutine context
>   *
> @@ -48,6 +69,18 @@
>  #define coroutine_fn
>  #endif
>  
> +/**
> + * Mark a function that should never be called from a coroutine context

Maybe this could be phrased better, because coroutine_mixed_fn are
functions that are specifically meant to be called from ambiguous
context, which of course includes coroutine context.

Something like "...when the caller knows that it is in coroutine
context"?

> + * This typically means that there is an analogous, coroutine_fn function 
> that
> + * should be used instead.
> + */
> +#ifdef __clang__
> +#define no_coroutine_fn __attribute__((__annotate__("no_coroutine_fn")))
> +#else
> +#define no_coroutine_fn
> +#endif
> +

Kevin