Features/CoroutineFnCleanup: Difference between revisions

From QEMU
No edit summary
 
(24 intermediate revisions by the same user not shown)
Line 1: Line 1:
== Hand-written bdrv_co_* functions ==
== Hand-written wrappers for coroutine functions ==
# bdrv_pread/bdrv_pwrite have different calling convention than bdrv_co_pread/bdrv_co_pwrite
# bdrv_pread/bdrv_pwrite have different calling convention than bdrv_co_pread/bdrv_co_pwrite [Alberto]
#* Fix using Coccinelle?
#* Fix using Coccinelle
#* Switch bdrv_pread/bdrv_pwrite to generated_co_wrapper
# Missing bdrv_co_pwrite_sync, bdrv_co_pwrite_zeroes
# Missing bdrv_co_pwrite_sync, bdrv_co_pwrite_zeroes
#* Adjust code in block/io.c and introduce generated_co_wrapper
#* Adjust code in block/io.c and introduce generated_co_wrapper
# Use generated_co_wrapper for block-backend.c too, removing the need for declarations in block/coroutines.h [Alberto]
# Probably more will be found with custom matching tools (see below)
== Converting callbacks to coroutines ==
Conversion of GLOBAL_STATE_CODE() callbacks to coroutines depends on removal of the AioContext lock:
* from coroutine context, IO_OR_GS_CODE() functions such as "drain_begin" have to be called with the AioContext lock taken
* to call them with the lock taken, the code has to run in the BDS's home iothread...
* ... and that is not possible for GLOBAL_STATE_CODE() functions unless the BDS home iothread is the main thread (which cannot be guaranteed)
IO_CODE() callbacks can be converted to coroutines.  Even for functions may be GLOBAL_STATE_CODE(), the underlying callbacks can be an IO_CODE() coroutine_fn. For example:
* bdrv_inactivate, which does I/O, can be converted to a coroutine_fn callback.  It is only implemented by qcow2, so it can be called from bdrv_close instead of qcow2_close. The call in bdrv_inactivate_recurse would go through a generated_co_wrapper in block/coroutines.h, e.g. bdrv_do_inactivate_one.
* It should be possible to convert the bdrv_snapshot_goto callback too. Likewise, bdrv_snapshot_goto would call a generated_co_wrapper, e.g. bdrv_do_snapshot_goto.


== Annotations ==
== Annotations ==
# Add coroutine_mixed_fn annotation; superset of generated_co_wrapper, also includes:
# Add coroutine_mixed_fn annotation; functions that call qemu_in_coroutine().  Leaves are:
#* all generated_co_wrapper functions
#* bdrv_create
#* bdrv_create
#* bdrv_can_store_new_dirty_bitmap, bdrv_remove_persistent_dirty_bitmap
#* bdrv_can_store_new_dirty_bitmap, bdrv_remove_persistent_dirty_bitmap
Line 16: Line 29:
#* nvme_get_free_req
#* nvme_get_free_req
#* schedule_next_request
#* schedule_next_request
# Detect call from non-coroutine_fn to coroutine_mixed_fn
# Detect call from non-coroutine_fn/coroutine_mixed_fn to coroutine_fn (libclang/clang-tidy)
#* Add coroutine_mixed_fn to caller
#* Remove coroutine_fn from callee, or add it to caller (check against https://patchew.org/QEMU/20220509103019.215041-1-pbonzini@redhat.com/)
#* Possibly add fixit to rewrite
#* Possibly add fixit to rewrite
# Detect call from coroutine_fn to coroutine_mixed_fn (libclang/clang-tidy)
# Detect call from coroutine_fn to generated_co_wrapper (libclang/clang-tidy)
#* Use _co_ version instead.
#* Possibly add fixit to rewrite
#* Possibly add fixit to rewrite
#* See also
# Detect call from non-coroutine_fn to coroutine_mixed_fn (libclang/clang-tidy)
#* Change caller to coroutine_mixed_fn too
#* If there is none, coroutine_mixed_fn can be changed to coroutine_fn


== Clang-query examples ==
== Clang-query examples ==
Invoking clang-query:
Invoking clang-query:
# Change definition of coroutine_fn to <tt>#define coroutine_fn=__attribute__((annotate("coroutine")))</tt>
# Change definition of coroutine_fn to <tt>#define coroutine_fn __attribute__((annotate("coroutine")))</tt>
# Invoke <tt>clang-query -p ''build-path'' block/io.c</tt>
# Invoke <tt>clang-query -p ''build-path'' block/io.c</tt>


Line 31: Line 47:


;<tt>match functionDecl(allOf(unless(hasAttr("attr::Annotate")),hasDescendant(callExpr(callee(hasAttr("attr::Annotate"))).bind("callee"))))</tt>
;<tt>match functionDecl(allOf(unless(hasAttr("attr::Annotate")),hasDescendant(callExpr(callee(hasAttr("attr::Annotate"))).bind("callee"))))</tt>
:call from non-coroutine_fn to coroutine_fn
:call from non-coroutine_fn to coroutine_fn (doesn't actually check the string in the annotate attribute)
;<tt>match varDecl(hasType(namedDecl(hasName("BlockDriver"))), hasInitializer(anything()))</tt>
;<tt>match varDecl(hasType(namedDecl(hasName("BlockDriver"))), hasInitializer(anything()))</tt>
:declaration of BlockDriver struct, with initializer
:declaration of BlockDriver struct, with initializer
Line 37: Line 53:
:indirect call
:indirect call


== Clang references ==
=== Clang references ===
https://devblogs.microsoft.com/cppblog/exploring-clang-tooling-part-1-extending-clang-tidy/
https://devblogs.microsoft.com/cppblog/exploring-clang-tooling-part-1-extending-clang-tidy/


Line 43: Line 59:


https://devblogs.microsoft.com/cppblog/exploring-clang-tooling-part-3-rewriting-code-with-clang-tidy/
https://devblogs.microsoft.com/cppblog/exploring-clang-tooling-part-3-rewriting-code-with-clang-tidy/
== VRC / path search ==
[https://github.com/bonzini/vrc VRC] is a tool to query a call graph. The call graph can be obtained from either GCC debug dumps (-fdump-rtl-expand) or clang (experimental).
The query language is based on regular expressions:
;<tt>[label1]</tt>
:Find function with the specified <tt>__attribute((annotate))</tt> annotation
;<tt>funcname</tt>, <tt>/regex/</tt>
:Find functions with the given name or matching the regex.
;<tt>...</tt>
:Zero or more functions (same as <tt>.*</tt> in grep, see below)
;<tt>( ... | ... )</tt>
:Alternation
Anything except <tt>...</tt> can be followed by a star to mean 0 or more occurrences.
<tt>[label1]</tt> is actually its own mini-language to query individual nodes:
;<tt>[label1,label2]</tt>
:Find function with all the specified <tt>__attribute((annotate))</tt> annotations
:<tt>[]</tt> matches all functions, i.e. <tt>...</tt> is a clearer alternative to <tt>[]*</tt>
;<tt>![label1|label2]</tt>
:Find function with none of the specified <tt>__attribute((annotate))</tt> annotations
;<tt>["func1"]</tt>, <tt>[/regex/]</tt>
:Match on function name, for example <tt>[coroutine_fn, !"qemu_coroutine_yield"]</tt>.
It is also possible to walk through edges; for example, coroutine candidates can be labeled with a single command: '''<tt>label coroutine_fn_candidate [!coroutine_fn,!coroutine_mixed_fn,coroutine_fn:callees,![!coroutine_fn:callees]]</tt>''' i.e. find the functions that are
* not coroutine_fn or coroutine_mixed_fn
* called by coroutine_fn
* not called by non-coroutine_fn
While the query language currently can only be used from the command line, it is possible to construct the AST from a Python program and therefore build static analysis that can be run at "make check" time.
===Sample call path queries===
;<tt>paths ![coroutine_fn|coroutine_mixed_fn] [coroutine_fn]</tt>
:Find incorrect calls to coroutine_fn
;<tt>paths [coroutine_fn] [no_coroutine_fn]</tt>
:Find incorrect calls to no_coroutine_fn
;<tt>paths [coroutine_fn] ![coroutine_fn|coroutine_mixed_fn]</tt>
;<tt>paths ![coroutine_fn] ![coroutine_fn|coroutine_mixed_fn]</tt>
:Find functions called only from functions that are coroutine_fn, and respectively not coroutine_fn. Functions only found in the first set are candidates for labeling as coroutine_fn, as in the "label" command above.
;<tt>label coroutine_fn_candidate [!coroutine_fn,!coroutine_mixed_fn,coroutine_fn:callees,![!coroutine_fn:callees]]<br>paths [coroutine_fn_candidate] ... qemu_coroutine_yield</tt>
:Find <tt>coroutine_fn_candidate</tt>s (see above) that ultimately suspend.  These are slam-dunk candidates for labeling as coroutine_fn.
;<tt>label suspending_fn ["qemu_coroutine_yield":all_callers]<br>paths [coroutine_fn_candidate,suspending_fn]</tt>
:More efficient version of the above
;<tt>paths [coroutine_mixed_fn,!coroutine_fn:callers]</tt>
:Find mixed functions called from functions that are not coroutine_fn.  Any coroutine_mixed_fn functions ''not'' on the list are candidates for changing to coroutine_fn
;<tt>paths [coroutine_mixed_fn,![coroutine_mixed_fn,!coroutine_fn:callers]]</tt>
:Find the "bad" coroutine_mixed_fn as mentioned in the previous query.
;<tt>label suspending_fn ["qemu_coroutine_yield":all_callers]<br>paths [coroutine_mixed_fn,![suspending_fn:callers]]</tt>
:Find coroutine_mixed_fn that will never suspend the current coroutine because they're actually never called in coroutine context.  These are candidates for switching to no_coroutine_fn.
;<tt>label no_coroutine_candidates [coroutine_mixed_fn,![suspending_fn:callers]]<br>paths [coroutine_fn|[coroutine_mixed_fn,!no_coroutine_candidates]] [no_coroutine_candidates]</tt>
:Find calls that need to be fixed before switching the <tt>no_coroutine_candidate</tt>s to <tt>no_coroutine_fn</tt>.

Latest revision as of 11:04, 12 April 2023

Hand-written wrappers for coroutine functions

  1. bdrv_pread/bdrv_pwrite have different calling convention than bdrv_co_pread/bdrv_co_pwrite [Alberto]
    • Fix using Coccinelle
    • Switch bdrv_pread/bdrv_pwrite to generated_co_wrapper
  2. Missing bdrv_co_pwrite_sync, bdrv_co_pwrite_zeroes
    • Adjust code in block/io.c and introduce generated_co_wrapper
  3. Use generated_co_wrapper for block-backend.c too, removing the need for declarations in block/coroutines.h [Alberto]
  4. Probably more will be found with custom matching tools (see below)

Converting callbacks to coroutines

Conversion of GLOBAL_STATE_CODE() callbacks to coroutines depends on removal of the AioContext lock:

  • from coroutine context, IO_OR_GS_CODE() functions such as "drain_begin" have to be called with the AioContext lock taken
  • to call them with the lock taken, the code has to run in the BDS's home iothread...
  • ... and that is not possible for GLOBAL_STATE_CODE() functions unless the BDS home iothread is the main thread (which cannot be guaranteed)

IO_CODE() callbacks can be converted to coroutines. Even for functions may be GLOBAL_STATE_CODE(), the underlying callbacks can be an IO_CODE() coroutine_fn. For example:

  • bdrv_inactivate, which does I/O, can be converted to a coroutine_fn callback. It is only implemented by qcow2, so it can be called from bdrv_close instead of qcow2_close. The call in bdrv_inactivate_recurse would go through a generated_co_wrapper in block/coroutines.h, e.g. bdrv_do_inactivate_one.
  • It should be possible to convert the bdrv_snapshot_goto callback too. Likewise, bdrv_snapshot_goto would call a generated_co_wrapper, e.g. bdrv_do_snapshot_goto.

Annotations

  1. Add coroutine_mixed_fn annotation; functions that call qemu_in_coroutine(). Leaves are:
    • all generated_co_wrapper functions
    • bdrv_create
    • bdrv_can_store_new_dirty_bitmap, bdrv_remove_persistent_dirty_bitmap
    • bdrv_drain_all_begin, bdrv_do_drained_begin, bdrv_do_drained_end
    • qio_channel_readv_full_all_eof, qio_channel_writev_full
    • qcow2_open, qcow2_has_zero_init
    • bdrv_qed_open
    • qmp_dispatch
    • nvme_get_free_req
    • schedule_next_request
  2. Detect call from non-coroutine_fn/coroutine_mixed_fn to coroutine_fn (libclang/clang-tidy)
  3. Detect call from coroutine_fn to generated_co_wrapper (libclang/clang-tidy)
    • Use _co_ version instead.
    • Possibly add fixit to rewrite
  4. Detect call from non-coroutine_fn to coroutine_mixed_fn (libclang/clang-tidy)
    • Change caller to coroutine_mixed_fn too
    • If there is none, coroutine_mixed_fn can be changed to coroutine_fn

Clang-query examples

Invoking clang-query:

  1. Change definition of coroutine_fn to #define coroutine_fn __attribute__((annotate("coroutine")))
  2. Invoke clang-query -p build-path block/io.c

Example of clang-query uses:

match functionDecl(allOf(unless(hasAttr("attr::Annotate")),hasDescendant(callExpr(callee(hasAttr("attr::Annotate"))).bind("callee"))))
call from non-coroutine_fn to coroutine_fn (doesn't actually check the string in the annotate attribute)
match varDecl(hasType(namedDecl(hasName("BlockDriver"))), hasInitializer(anything()))
declaration of BlockDriver struct, with initializer
match functionDecl(isExpansionInMainFile(), forEachDescendant(callExpr(callee(unless(functionDecl()))).bind("call"))).bind("root")'
indirect call

Clang references

https://devblogs.microsoft.com/cppblog/exploring-clang-tooling-part-1-extending-clang-tidy/

https://devblogs.microsoft.com/cppblog/exploring-clang-tooling-part-2-examining-the-clang-ast-with-clang-query/

https://devblogs.microsoft.com/cppblog/exploring-clang-tooling-part-3-rewriting-code-with-clang-tidy/

VRC / path search

VRC is a tool to query a call graph. The call graph can be obtained from either GCC debug dumps (-fdump-rtl-expand) or clang (experimental).

The query language is based on regular expressions:

[label1]
Find function with the specified __attribute((annotate)) annotation
funcname, /regex/
Find functions with the given name or matching the regex.
...
Zero or more functions (same as .* in grep, see below)
( ... | ... )
Alternation

Anything except ... can be followed by a star to mean 0 or more occurrences.

[label1] is actually its own mini-language to query individual nodes:

[label1,label2]
Find function with all the specified __attribute((annotate)) annotations
[] matches all functions, i.e. ... is a clearer alternative to []*
![label1|label2]
Find function with none of the specified __attribute((annotate)) annotations
["func1"], [/regex/]
Match on function name, for example [coroutine_fn, !"qemu_coroutine_yield"].

It is also possible to walk through edges; for example, coroutine candidates can be labeled with a single command: label coroutine_fn_candidate [!coroutine_fn,!coroutine_mixed_fn,coroutine_fn:callees,![!coroutine_fn:callees]] i.e. find the functions that are

  • not coroutine_fn or coroutine_mixed_fn
  • called by coroutine_fn
  • not called by non-coroutine_fn

While the query language currently can only be used from the command line, it is possible to construct the AST from a Python program and therefore build static analysis that can be run at "make check" time.

Sample call path queries

paths ![coroutine_fn|coroutine_mixed_fn] [coroutine_fn]
Find incorrect calls to coroutine_fn
paths [coroutine_fn] [no_coroutine_fn]
Find incorrect calls to no_coroutine_fn
paths [coroutine_fn] ![coroutine_fn|coroutine_mixed_fn]
paths ![coroutine_fn] ![coroutine_fn|coroutine_mixed_fn]
Find functions called only from functions that are coroutine_fn, and respectively not coroutine_fn. Functions only found in the first set are candidates for labeling as coroutine_fn, as in the "label" command above.
label coroutine_fn_candidate [!coroutine_fn,!coroutine_mixed_fn,coroutine_fn:callees,![!coroutine_fn:callees]]
paths [coroutine_fn_candidate] ... qemu_coroutine_yield
Find coroutine_fn_candidates (see above) that ultimately suspend. These are slam-dunk candidates for labeling as coroutine_fn.
label suspending_fn ["qemu_coroutine_yield":all_callers]
paths [coroutine_fn_candidate,suspending_fn]
More efficient version of the above
paths [coroutine_mixed_fn,!coroutine_fn:callers]
Find mixed functions called from functions that are not coroutine_fn. Any coroutine_mixed_fn functions not on the list are candidates for changing to coroutine_fn
paths [coroutine_mixed_fn,![coroutine_mixed_fn,!coroutine_fn:callers]]
Find the "bad" coroutine_mixed_fn as mentioned in the previous query.
label suspending_fn ["qemu_coroutine_yield":all_callers]
paths [coroutine_mixed_fn,![suspending_fn:callers]]
Find coroutine_mixed_fn that will never suspend the current coroutine because they're actually never called in coroutine context. These are candidates for switching to no_coroutine_fn.
label no_coroutine_candidates [coroutine_mixed_fn,![suspending_fn:callers]]
paths [coroutine_fn|[coroutine_mixed_fn,!no_coroutine_candidates]] [no_coroutine_candidates]
Find calls that need to be fixed before switching the no_coroutine_candidates to no_coroutine_fn.