| Message ID | 20221125133518.418328-11-eesposit@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Still more coroutine and various fixes in block layer | expand |
On 11/25/22 16:35, Emanuele Giuseppe Esposito wrote: > This new annotation creates just a function wrapper that creates > a new coroutine. Actually, not just create, but create, start and wait for finish.. Maybe s/creates/starts/ > It assumes the caller is not a coroutine. > It will be the default annotation to be used in the future. > > This is much better as c_w_mixed, because it is clear if the caller > is a coroutine or not, and provides the advantage of automating > the code creation. In the future all c_w_mixed functions will be > substituted by co_wrapper. > > Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> > Reviewed-by: Kevin Wolf <kwolf@redhat.com> > --- [..] > > class FuncDecl: > - def __init__(self, return_type: str, name: str, args: str) -> None: > + def __init__(self, return_type: str, name: str, args: str, > + variant: str) -> None: I'd prefer mixed: bool parameter instead, to be more strict. > self.return_type = return_type.strip() > self.name = name.strip() > + self.struct_name = snake_to_camel(self.name) > self.args = [ParamDecl(arg.strip()) for arg in args.split(',')] > + self.create_only_co = True > + > + if 'mixed' in variant: > + self.create_only_co = False hmm, just self.create_only_co = 'mixed' not in variant ? And even better with boolean argument. > + > + subsystem, subname = self.name.split('_', 1) > + self.co_name = f'{subsystem}_co_{subname}' > + > + t = self.args[0].type > + if t == 'BlockDriverState *': > + bs = 'bs' > + elif t == 'BdrvChild *': > + bs = 'child->bs' > + else: > + bs = 'blk_bs(blk)' > + self.bs = bs > > def gen_list(self, format: str) -> str: > return ', '.join(format.format_map(arg.__dict__) for arg in self.args) > @@ -74,8 +92,9 @@ def gen_block(self, format: str) -> str: > return '\n'.join(format.format_map(arg.__dict__) for arg in self.args) > > > -# Match wrappers declared with a co_wrapper_mixed mark > -func_decl_re = re.compile(r'^int\s*co_wrapper_mixed\s*' > +# Match wrappers declared with a co_wrapper mark > +func_decl_re = re.compile(r'^int\s*co_wrapper' > + r'(?P<variant>(_[a-z][a-z0-9_]*)?)\s*' Why you allow everything here? I'd write just (?P<mixed>(_mixed)?) or (?P<marker>co_wrapper(_mixed)?) > r'(?P<wrapper_name>[a-z][a-z0-9_]*)' > r'\((?P<args>[^)]*)\);$', re.MULTILINE) > > @@ -84,7 +103,8 @@ def func_decl_iter(text: str) -> Iterator: > for m in func_decl_re.finditer(text): > yield FuncDecl(return_type='int', > name=m.group('wrapper_name'), > - args=m.group('args')) > + args=m.group('args'), > + variant=m.group('variant')) > > > def snake_to_camel(func_name: str) -> str: > @@ -97,24 +117,63 @@ def snake_to_camel(func_name: str) -> str: > return ''.join(words) > > > +# Checks if we are already in coroutine Please, do function documentation in doc-sting > +def create_g_c_w(func: FuncDecl) -> str: maybe, create_mixed_wrapper? g_c_w refer to "generated_co_wrapper" and it was dropped from everywhere already > + name = func.co_name > + struct_name = func.struct_name > + return f"""\ [..] > + > +# Assumes we are not in coroutine, and creates one move to doc-string > +def create_coroutine_only(func: FuncDecl) -> str: maybe, create_co_wrapper ? Also, in the file functions that generate code are called gen_*, so better gen_co_wrapper() and gen_co_wrapper_mixed() > + name = func.co_name > + struct_name = func.struct_name > + return f"""\ > +int {func.name}({ func.gen_list('{decl}') }) > +{{ [..] > > def gen_wrappers(input_code: str) -> str: With at least comments for new functions turned into doc-strings: Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Am 25/11/2022 um 21:32 schrieb Vladimir Sementsov-Ogievskiy: > >> class FuncDecl: >> - def __init__(self, return_type: str, name: str, args: str) -> None: >> + def __init__(self, return_type: str, name: str, args: str, >> + variant: str) -> None: > > I'd prefer mixed: bool parameter instead, to be more strict. > >> self.return_type = return_type.strip() >> self.name = name.strip() >> + self.struct_name = snake_to_camel(self.name) >> self.args = [ParamDecl(arg.strip()) for arg in args.split(',')] >> + self.create_only_co = True >> + >> + if 'mixed' in variant: >> + self.create_only_co = False > > hmm, just > > self.create_only_co = 'mixed' not in variant > > ? And even better with boolean argument. > >> + >> + subsystem, subname = self.name.split('_', 1) >> + self.co_name = f'{subsystem}_co_{subname}' >> + >> + t = self.args[0].type >> + if t == 'BlockDriverState *': >> + bs = 'bs' >> + elif t == 'BdrvChild *': >> + bs = 'child->bs' >> + else: >> + bs = 'blk_bs(blk)' >> + self.bs = bs >> def gen_list(self, format: str) -> str: >> return ', '.join(format.format_map(arg.__dict__) for arg in >> self.args) >> @@ -74,8 +92,9 @@ def gen_block(self, format: str) -> str: >> return '\n'.join(format.format_map(arg.__dict__) for arg in >> self.args) >> -# Match wrappers declared with a co_wrapper_mixed mark >> -func_decl_re = re.compile(r'^int\s*co_wrapper_mixed\s*' >> +# Match wrappers declared with a co_wrapper mark >> +func_decl_re = re.compile(r'^int\s*co_wrapper' >> + r'(?P<variant>(_[a-z][a-z0-9_]*)?)\s*' > > Why you allow everything here? > I'd write just > > (?P<mixed>(_mixed)?) > > or > > (?P<marker>co_wrapper(_mixed)?) Ok you couldn't possibly know that, but we are also adding other type of "variants": co_wrapper co_wrapper_mixed co_wrapper_bdrv_rdlock co_wrapper_mixed_bdrv_rdlock Therefore I need to keep variant : str and the regex as it is, but maybe get rid of the if else condition. I'll change the docstring of course. If you want to know more, see the thread in "[PATCH 00/15] Protect the block layer with a rwlock: part 3" Thank you, Emanuele
diff --git a/docs/devel/block-coroutine-wrapper.rst b/docs/devel/block-coroutine-wrapper.rst index 64acc8d65d..6dd2cdcab3 100644 --- a/docs/devel/block-coroutine-wrapper.rst +++ b/docs/devel/block-coroutine-wrapper.rst @@ -26,12 +26,12 @@ called ``bdrv_foo(<same args>)``. In this case the script can help. To trigger the generation: 1. You need ``bdrv_foo`` declaration somewhere (for example, in - ``block/coroutines.h``) with the ``co_wrapper_mixed`` mark, + ``block/coroutines.h``) with the ``co_wrapper`` mark, like this: .. code-block:: c - int co_wrapper_mixed bdrv_foo(<some args>); + int co_wrapper bdrv_foo(<some args>); 2. You need to feed this declaration to block-coroutine-wrapper script. For this, add the .h (or .c) file with the declaration to the @@ -46,7 +46,7 @@ Links 1. The script location is ``scripts/block-coroutine-wrapper.py``. -2. Generic place for private ``co_wrapper_mixed`` declarations is +2. Generic place for private ``co_wrapper`` declarations is ``block/coroutines.h``, for public declarations: ``include/block/block.h`` diff --git a/include/block/block-common.h b/include/block/block-common.h index ec2309055b..847e4d4626 100644 --- a/include/block/block-common.h +++ b/include/block/block-common.h @@ -42,9 +42,13 @@ * * Usage: read docs/devel/block-coroutine-wrapper.rst * - * co_wrapper_mixed functions can be called by both coroutine and - * non-coroutine context. + * There are 2 kind of specifiers: + * - co_wrapper functions can be called by only non-coroutine context, because + * they always generate a new coroutine. + * - co_wrapper_mixed functions can be called by both coroutine and + * non-coroutine context. */ +#define co_wrapper #define co_wrapper_mixed /* block.c */ diff --git a/scripts/block-coroutine-wrapper.py b/scripts/block-coroutine-wrapper.py index 56e6425356..7972d3fe01 100644 --- a/scripts/block-coroutine-wrapper.py +++ b/scripts/block-coroutine-wrapper.py @@ -2,7 +2,7 @@ """Generate coroutine wrappers for block subsystem. The program parses one or several concatenated c files from stdin, -searches for functions with the 'co_wrapper_mixed' specifier +searches for functions with the 'co_wrapper' specifier and generates corresponding wrappers on stdout. Usage: block-coroutine-wrapper.py generated-file.c FILE.[ch]... @@ -62,10 +62,28 @@ def __init__(self, param_decl: str) -> None: class FuncDecl: - def __init__(self, return_type: str, name: str, args: str) -> None: + def __init__(self, return_type: str, name: str, args: str, + variant: str) -> None: self.return_type = return_type.strip() self.name = name.strip() + self.struct_name = snake_to_camel(self.name) self.args = [ParamDecl(arg.strip()) for arg in args.split(',')] + self.create_only_co = True + + if 'mixed' in variant: + self.create_only_co = False + + subsystem, subname = self.name.split('_', 1) + self.co_name = f'{subsystem}_co_{subname}' + + t = self.args[0].type + if t == 'BlockDriverState *': + bs = 'bs' + elif t == 'BdrvChild *': + bs = 'child->bs' + else: + bs = 'blk_bs(blk)' + self.bs = bs def gen_list(self, format: str) -> str: return ', '.join(format.format_map(arg.__dict__) for arg in self.args) @@ -74,8 +92,9 @@ def gen_block(self, format: str) -> str: return '\n'.join(format.format_map(arg.__dict__) for arg in self.args) -# Match wrappers declared with a co_wrapper_mixed mark -func_decl_re = re.compile(r'^int\s*co_wrapper_mixed\s*' +# Match wrappers declared with a co_wrapper mark +func_decl_re = re.compile(r'^int\s*co_wrapper' + r'(?P<variant>(_[a-z][a-z0-9_]*)?)\s*' r'(?P<wrapper_name>[a-z][a-z0-9_]*)' r'\((?P<args>[^)]*)\);$', re.MULTILINE) @@ -84,7 +103,8 @@ def func_decl_iter(text: str) -> Iterator: for m in func_decl_re.finditer(text): yield FuncDecl(return_type='int', name=m.group('wrapper_name'), - args=m.group('args')) + args=m.group('args'), + variant=m.group('variant')) def snake_to_camel(func_name: str) -> str: @@ -97,24 +117,63 @@ def snake_to_camel(func_name: str) -> str: return ''.join(words) +# Checks if we are already in coroutine +def create_g_c_w(func: FuncDecl) -> str: + name = func.co_name + struct_name = func.struct_name + return f"""\ +int {func.name}({ func.gen_list('{decl}') }) +{{ + if (qemu_in_coroutine()) {{ + return {name}({ func.gen_list('{name}') }); + }} else {{ + {struct_name} s = {{ + .poll_state.bs = {func.bs}, + .poll_state.in_progress = true, + +{ func.gen_block(' .{name} = {name},') } + }}; + + s.poll_state.co = qemu_coroutine_create({name}_entry, &s); + + return bdrv_poll_co(&s.poll_state); + }} +}}""" + + +# Assumes we are not in coroutine, and creates one +def create_coroutine_only(func: FuncDecl) -> str: + name = func.co_name + struct_name = func.struct_name + return f"""\ +int {func.name}({ func.gen_list('{decl}') }) +{{ + {struct_name} s = {{ + .poll_state.bs = {func.bs}, + .poll_state.in_progress = true, + +{ func.gen_block(' .{name} = {name},') } + }}; + assert(!qemu_in_coroutine()); + + s.poll_state.co = qemu_coroutine_create({name}_entry, &s); + + return bdrv_poll_co(&s.poll_state); +}}""" + + def gen_wrapper(func: FuncDecl) -> str: assert not '_co_' in func.name assert func.return_type == 'int' assert func.args[0].type in ['BlockDriverState *', 'BdrvChild *', 'BlockBackend *'] - subsystem, subname = func.name.split('_', 1) - - name = f'{subsystem}_co_{subname}' + name = func.co_name + struct_name = func.struct_name - t = func.args[0].type - if t == 'BlockDriverState *': - bs = 'bs' - elif t == 'BdrvChild *': - bs = 'child->bs' - else: - bs = 'blk_bs(blk)' - struct_name = snake_to_camel(name) + creation_function = create_g_c_w + if func.create_only_co: + creation_function = create_coroutine_only return f"""\ /* @@ -136,23 +195,7 @@ def gen_wrapper(func: FuncDecl) -> str: aio_wait_kick(); }} -int {func.name}({ func.gen_list('{decl}') }) -{{ - if (qemu_in_coroutine()) {{ - return {name}({ func.gen_list('{name}') }); - }} else {{ - {struct_name} s = {{ - .poll_state.bs = {bs}, - .poll_state.in_progress = true, - -{ func.gen_block(' .{name} = {name},') } - }}; - - s.poll_state.co = qemu_coroutine_create({name}_entry, &s); - - return bdrv_poll_co(&s.poll_state); - }} -}}""" +{creation_function(func)}""" def gen_wrappers(input_code: str) -> str: