Re: [PATCH v4 3/5] block: generate coroutine-wrapper code

[Vladimir Sementsov-Ogievskiy]

27.05.2020 00:30, Eric Blake wrote:
> On 5/25/20 5:07 AM, Vladimir Sementsov-Ogievskiy wrote:
> > We have a very frequent pattern of creating coroutine from function
> > with several arguments:
> >
> >    - create structure to pack parameters
> >    - create _entry function to call original function taking parameters
> >      from struct
> >    - do different magic to handle completion: set ret to NOT_DONE or
> >      EINPROGRESS, use separate bool for void functions
> >    - fill the struct and create coroutine from _entry function and this
> >      struct as a parameter
> >    - do coroutine enter and BDRV_POLL_WHILE loop
> >
> > Let's reduce code duplication. Here:
> >
> > Functional part (BDRV_POLL_WHILE loop, aio_wait_kick()) moved to
> > (non-generated) block/block-gen.h
> >
> > Mechanical part (arguments packing, different kind of needed wrappers)
> > are generated from template by scripts/coroutine-wrapper.py to
> > resulting file block/block-gen.c
> >
> > Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> > ---
>
> > @@ -175,6 +177,10 @@ generated-files-y += $(TRACE_SOURCES)
> >   generated-files-y += $(BUILD_DIR)/trace-events-all
> >   generated-files-y += .git-submodule-status
> > +COROUTINE_HEADERS = include/block/block.h block/coroutines.h
> > +block/block-gen.c: $(COROUTINE_HEADERS) 
> > $(SRC_PATH)/scripts/coroutine-wrapper.py
> > +    $(call quiet-command, cat $(COROUTINE_HEADERS) | $(SRC_PATH)/scripts/coroutine-wrapper.py > 
> > $@,"GEN","$(TARGET_DIR)$@")
> > +
>
> Not VPATH-friendly; I posted a proposed fixup! separately.

Thanks!

> >   trace-group-name = $(shell dirname $1 | sed -e 's/[^a-zA-Z0-9]/_/g')
> >   tracetool-y = $(SRC_PATH)/scripts/tracetool.py
> > diff --git a/Makefile.objs b/Makefile.objs
> > index 99774cfd25..8cb20f94c3 100644
> > --- a/Makefile.objs
> > +++ b/Makefile.objs
> > @@ -14,7 +14,7 @@ chardev-obj-y = chardev/
> >   authz-obj-y = authz/
> >   block-obj-y = block/ block/monitor/ nbd/ scsi/
> > -block-obj-y += block.o blockjob.o job.o
> > +block-obj-y += block.o blockjob.o job.o block/block-gen.o
>
> It may be cleaner to add this in block/Makefile.objs rather than in top-level 
> Makefile.objs.  In fact, rearranging your mail a bit...
>
>  > diff --git a/block/Makefile.objs b/block/Makefile.objs
>  > index 3635b6b4c1..05e4d033c1 100644
>  > --- a/block/Makefile.objs
>  > +++ b/block/Makefile.objs
>  > @@ -45,6 +45,7 @@ block-obj-y += crypto.o
>  >   block-obj-y += aio_task.o
>  >   block-obj-y += backup-top.o
>  >   block-obj-y += filter-compress.o
>  > +block-obj-y += block-gen.o
>  >   common-obj-y += monitor/
>  >
>  >   block-obj-y += stream.o
>
> ...you did just that.  Dropping the change to top-level Makefile.objs seems to 
> make no difference to a correct build.

Oops, yes.

> > +++ b/block/block-gen.h
> > @@ -0,0 +1,55 @@
> > +/*
> > + * Block layer I/O functions
>
> Is this still the best one-line summary for this file?  Especially since...

Yes, it's outdated. Maybe

Block coroutine wrapping core, used by auto-generated block/block-gen.c

> > +
> > +/* This function is called at the end of generated coroutine entries. */
> > +static inline void bdrv_poll_co__on_exit(void)
> > +{
> > +    aio_wait_kick();
> > +}
> > +
> > +/* Base structure for argument packing structures */
> > +typedef struct BdrvPollCo {
> > +    BlockDriverState *bs;
> > +    bool in_progress;
> > +    int ret;
> > +    Coroutine *co; /* Keep pointer here for debugging */
> > +} BdrvPollCo;
> > +
> > +static inline int bdrv_poll_co(BdrvPollCo *s)
> > +{
> > +    assert(!qemu_in_coroutine());
> > +
> > +    bdrv_coroutine_enter(s->bs, s->co);
> > +    BDRV_POLL_WHILE(s->bs, s->in_progress);
> > +
> > +    return s->ret;
> > +}
>
> This part looks fine.
>
>
> > +++ b/include/block/generated-co-wrapper.h
> > @@ -0,0 +1,35 @@
> > +/*
> > + * Block layer I/O functions
>
> ...you repeat it here?
>
> > +/*
> > + * generated_co_wrapper
> > + * Function specifier, which does nothing but marking functions to be
> > + * generated by scripts/coroutine-wrapper.py
> > + */
> > +#define generated_co_wrapper
> > +
> > +#endif /* BLOCK_GENERATED_CO_WRAPPER_H */
>
> Not sure if a separate header was needed for this, but I guess it doesn't hurt. 
>  I might have just used block_int.h.

Than we'll have to include block_int.h in block.h.. Is it OK? This is the 
reason why I decided to keep it separate.

> > diff --git a/block.c b/block.c
> > index 7f06e82880..c1132ab323 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -4640,43 +4640,6 @@ int coroutine_fn bdrv_co_check(BlockDriverState *bs,
> >       return bs->drv->bdrv_co_check(bs, res, fix);
> >   }
> > -typedef struct CheckCo {
> > -    BlockDriverState *bs;
> > -    BdrvCheckResult *res;
> > -    BdrvCheckMode fix;
> > -    int ret;
> > -} CheckCo;
>
> This patch is doing two things - introducing a new generator script that scans 
> the code for generated_co_wrapper tags, _and_ adds the tags in as many places 
> as possible.  It makes for a big patch.  Better might have been to introduce 
> the script and the concept of a tag in one patch but not actually tag any new 
> functions (so the generated file is basically empty, but you prove the build 
> works and can audit the script without being bogged down by the mechanical 
> changes), then do a separate patch with adding the tags and deleting the code 
> now covered by the generator (which will be mostly mechanical).

No problem, will do.

> > +++ b/scripts/coroutine-wrapper.py
> > @@ -0,0 +1,168 @@
> > +#!/usr/bin/env python3
>
> My python review skills are weak, so you'll probably want another reviewer here 
> (although I _can_ state that I checked the generated block/block-gen.c file and 
> it makes sense).
>
>
> > +import re
> > +from typing import List, Iterator
> > +
> > +header = '''/*
> > + * File is generated by scripts/coroutine-wrapper.py
> > + */
>
> It's worth also generating a short copyright blurb into the generated file.
>
> > +
> > +#include "qemu/osdep.h"
> > +#include "block/coroutines.h"
> > +#include "block/block-gen.h"'''
> > +
> > +template = """
>
> Why ''' above and """ here?  While python accepts both forms, consistency is 
> desirable.  I have no idea what pylint thinks of your code, though, so I may be completely 
> overlooking idiomatic styles.
''' is used just to not interfer with " around include files.. So I tend to generally use 
""" for multiline strings, but the above is and exception just for internal " 
characters.

> > +
> > +/*
> > + * Wrappers for $name$
> > + */
> > +
> > +typedef struct $struct_name$ {
> > +    BdrvPollCo poll_state;
> > +    $fields$
> > +} $struct_name$;
> > +
> > +static void coroutine_fn $name$_entry(void *opaque)
> > +{
> > +    $struct_name$ *s = opaque;
> > +
> > +    $assign_ret$$name$($args_from_s$);
>
> Creates a long line. Doesn't matter since this is generated code, so I don't 
> know if it's worth trying to coerce python into generating something that is 
> wrapped nicely.
>
> > +
> > +    s->poll_state.in_progress = false;
> > +
> > +    bdrv_poll_co__on_exit();
>
> That function only calls aio_wait_kick().  Why did you not want to call that 
> directly here, instead of going through such a one-line wrapper function?

To keep polling logic out of generated code, in block/block-gen.h, like 
bdrv_poll_co. Does it worth doing - I'm not sure. Up to maintainers.

> > +}
> > +
> > +$ret_type$ $wrapper_name$($args_def$)
> > +{
> > +    if (qemu_in_coroutine()) {
> > +        $do_return$$name$($arg_names$);
>
> Would it be any simpler to teach bdrv_co_invalidate_cache() to return a value, 
> so that you can eliminate the need for $do_return$ and just hard-code 'return ' 
> here?  The .bdrv_co_invalidate_cache driver callback can still return void, and 
> just have the wrapper in block.c return 0. But if you want to do that, it would 
> be a separate prerequisite patch.

Hmm. I thought, that it's not a good reason to add a always-success return 
value.. But stop! bdrv_invalidate_cache is a void functions with errp argument, 
so it makes sense to add a return value anyway. Will do.

> > +    } else {
> > +        $struct_name$ s = {
> > +            .poll_state.bs = $bs$,
> > +            .poll_state.in_progress = true,
> > +
> > +            $initializers$
> > +        };
> > +
> > +        s.poll_state.co = qemu_coroutine_create($name$_entry, &s);
> > +
> > +        $do_return$bdrv_poll_co(&s.poll_state);
>
> Another spot impacted by that one outlier.
>
> > +    }
> > +}"""
> > +
> > +# We want to use python string.format() formatter, which uses curly brackets
> > +# as separators. But it's not comfortable with C. So, we used dollars instead,
> > +# and now is the time to escape curly brackets and convert dollars.
> > +template = template.replace('{', '{{').replace('}', '}}')
> > +template = re.sub(r'\$(\w+)\$', r'{\1}', template)
>
> I'll leave it to others to decide if your approach has a more idiomatic python 
> solution.  I'm used to the qapi generator using code like this in 
> script/qapi/visit.py:
>
> def gen_visit_members_decl(name):
>      return mcgen('''
>
> void visit_type_%(c_name)s_members(Visitor *v, %(c_name)s *obj, Error **errp);
> ''',
>                   c_name=c_name(name))
>
> which has the same end goal of inserting named tags into a format string, but 
> which uses %(tag) for tags rather than your novel approach of $tag$ with 
> post-processing.

Note, that actually not %(tag) but %(tag)s

%(name)s notation comes from % interpolation operator.. As I understand, in 
modern python using .format(), f-strings and templates is preferable.
But I agree, that introducing new hand-made notation is not a great idea. I 
think, I'll move to string.Template or f-strings.

> > +
> > +
> > +class ParamDecl:
> > +    param_re = re.compile(r'(?P<decl>'
> > +                          r'(?P<type>.*[ *])'
> > +                          r'(?P<name>[a-z][a-z0-9_]*)'
> > +                          r')')
> > +
> > +    def __init__(self, param_decl: str) -> None:
> > +        m = self.param_re.match(param_decl.strip())
> > +        self.decl = m.group('decl')
> > +        self.type = m.group('type')
> > +        self.name = m.group('name')
> > +
> > +
> > +class FuncDecl:
> > +    def __init__(self, return_type: str, name: str, args: str) -> None:
> > +        self.return_type = return_type.strip()
> > +        self.name = name.strip()
> > +        self.args: List[ParamDecl] = []
> > +        self.args = [ParamDecl(arg) for arg in args.split(',')]
> > +
> > +    def get_args_decl(self) -> str:
> > +        return ', '.join(arg.decl for arg in self.args)
> > +
> > +    def get_arg_names(self) -> str:
> > +        return ', '.join(arg.name for arg in self.args)
> > +
> > +    def gen_struct_fields(self) -> str:
> > +        return '\n    '.join(f'{arg.decl};' for arg in self.args)
> > +
> > +    def gen_struct_initializers(self, indent: int) -> str:
> > +        sep = '\n' + ' ' * indent
> > +        return sep.join(f'.{a.name} = {a.name},' for a in self.args)
> > +
> > +
> > +# Match wrappers declaration, with generated_co_wrapper mark
>
> s/declaration, with/declared with a/
>
> > +func_decl_re = re.compile(r'^(?P<return_type>(int|void))'
> > +                          r'\s*generated_co_wrapper\s*'
> > +                          r'(?P<wrapper_name>[a-z][a-z0-9_]*)'
> > +                          r'\((?P<args>[^)]*)\);$', re.MULTILINE)
> > +
> > +
> > +def func_decl_iter(text: str) -> Iterator:
> > +    for m in func_decl_re.finditer(text):
> > +        yield FuncDecl(return_type=m.group('return_type'),
> > +                       name=m.group('wrapper_name'),
> > +                       args=m.group('args'))
> > +
> > +
> > +def struct_name(func_name: str) -> str:
> > +    """some_function_name -> SomeFunctionName"""
> > +    words = func_name.split('_')
> > +    words = [w[0].upper() + w[1:] for w in words]
> > +    return ''.join(words)
> > +
> > +
> > +def make_wrapper(func: FuncDecl) -> str:
> > +    assert func.name.startswith('bdrv_')
> > +    co_name = 'bdrv_co_' + func.name[5:]
> > +
> > +    has_ret = func.return_type != 'void'
> > +
> > +    params = {
> > +        'name': co_name,
> > +        'do_return': 'return ' if has_ret else '',
> > +        'assign_ret': 's->poll_state.ret = ' if has_ret else '',
>
> See my comments above about possibly normalizing the one outlier of 
> bdrv_co_invalidate_cache first.
>
> > +        'struct_name': struct_name(co_name),
> > +        'wrapper_name': func.name,
> > +        'ret_type': func.return_type,
> > +        'args_def': func.get_args_decl(),
> > +        'arg_names': func.get_arg_names(),
> > +        'fields': func.gen_struct_fields(),
> > +        'initializers': func.gen_struct_initializers(12),
>
> 12 looks like a magic number.
>
> > +        'args_from_s': ', '.join(f's->{a.name}' for a in func.args),
> > +    }
> > +
> > +    if func.args[0].type == 'BlockDriverState *':
> > +        params['bs'] = 'bs'
> > +    else:
> > +        assert func.args[0].type == 'BdrvChild *'
> > +        params['bs'] = 'child->bs'
> > +
> > +    return template.format(**params)
> > +
> > +
> > +if __name__ == '__main__':
> > +    import sys
> > +
> > +    print(header)
> > +    for func in func_decl_iter(sys.stdin.read()):
> > +        print(make_wrapper(func))


--
Best regards,
Vladimir