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 
---



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

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

[Eric Blake]

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 
---



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


  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.



+++ 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...


+
+/* 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.



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).



+++ 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