[Qemu-devel] [PATCH v3 0/2] Documentation for coroutine annotations

[Qemu-devel] [PATCH v3 0/2] Documentation for coroutine annotations

[Charlie Shepherd]

These patches were the first two from my GSoC series and were reasonably
straight-forward and well accepted. Gabriel and I are hoping the patches from
GSoC can be merged before I start my job in December, so I'm starting by sending
the simple parts of the overall patchset, when they are merged then I will redo
the later parts in several smaller and more manageable patchsets.

Charlie Shepherd (2):
  Add an explanation of when a function should be marked coroutine_fn
  Rename qemu_coroutine_self to qemu_coroutine_self_int and add an
    annotated wrapper

 coroutine-gthread.c           |  2 +-
 coroutine-sigaltstack.c       |  2 +-
 coroutine-ucontext.c          |  2 +-
 coroutine-win32.c             |  2 +-
 include/block/coroutine.h     |  8 ++++++++
 include/block/coroutine_int.h |  1 +
 qemu-coroutine.c              | 15 ++++++++++++++-
 7 files changed, 27 insertions(+), 5 deletions(-)

-- 
1.8.4.rc3

[Qemu-devel] [PATCH v3 1/2] Add an explanation of when a function should be marked coroutine_fn

[Charlie Shepherd]

Coroutine functions that can yield directly or indirectly should be annotated
with a coroutine_fn annotation. Add an explanation to that effect in
include/block/coroutine.h.
---
 include/block/coroutine.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/include/block/coroutine.h b/include/block/coroutine.h
index 4232569..e11a587 100644
--- a/include/block/coroutine.h
+++ b/include/block/coroutine.h
@@ -38,6 +38,14 @@
  * static checker support for catching such errors.  This annotation might make
  * it possible and in the meantime it serves as documentation.
  *
+ * A function must be marked with coroutine_fn if it can yield execution, either
+ * directly or indirectly.
+ *
+ * Some functions dynamically determine whether to yield or not based on
+ * whether they are executing in a coroutine context or not. These functions
+ * do not need to be annotated coroutine_fn. Note that this practice is
+ * deprecated and is being phased out, new code should not do this.
+ *
  * For example:
  *
  *   static void coroutine_fn foo(void) {
-- 
1.8.4.rc3

[Qemu-devel] [PATCH v3 2/2] Rename qemu_coroutine_self to qemu_coroutine_self_int and add an annotated wrapper

[Charlie Shepherd]

While it only really makes sense to call qemu_coroutine_self() in a coroutine
context, some coroutine internals need to call it from functions not annotated
as coroutine_fn, so add an annotated wrapper and rename the implementation
versions to qemu_coroutine_self_int.
---
 coroutine-gthread.c           |  2 +-
 coroutine-sigaltstack.c       |  2 +-
 coroutine-ucontext.c          |  2 +-
 coroutine-win32.c             |  2 +-
 include/block/coroutine_int.h |  1 +
 qemu-coroutine.c              | 15 ++++++++++++++-
 6 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/coroutine-gthread.c b/coroutine-gthread.c
index d3e5b99..a913aeb 100644
--- a/coroutine-gthread.c
+++ b/coroutine-gthread.c
@@ -194,7 +194,7 @@ CoroutineAction qemu_coroutine_switch(Coroutine *from_,
     return from->action;
 }
 
-Coroutine *qemu_coroutine_self(void)
+Coroutine *qemu_coroutine_self_int(void)
 {
     CoroutineGThread *co = get_coroutine_key();
     if (!co) {
diff --git a/coroutine-sigaltstack.c b/coroutine-sigaltstack.c
index 3de0bb3..0556539 100644
--- a/coroutine-sigaltstack.c
+++ b/coroutine-sigaltstack.c
@@ -277,7 +277,7 @@ CoroutineAction qemu_coroutine_switch(Coroutine *from_, Coroutine *to_,
     return ret;
 }
 
-Coroutine *qemu_coroutine_self(void)
+Coroutine *qemu_coroutine_self_int(void)
 {
     CoroutineThreadState *s = coroutine_get_thread_state();
 
diff --git a/coroutine-ucontext.c b/coroutine-ucontext.c
index 4bf2cde..27d1b79 100644
--- a/coroutine-ucontext.c
+++ b/coroutine-ucontext.c
@@ -210,7 +210,7 @@ CoroutineAction qemu_coroutine_switch(Coroutine *from_, Coroutine *to_,
     return ret;
 }
 
-Coroutine *qemu_coroutine_self(void)
+Coroutine *qemu_coroutine_self_int(void)
 {
     CoroutineThreadState *s = coroutine_get_thread_state();
 
diff --git a/coroutine-win32.c b/coroutine-win32.c
index edc1f72..3f1f79b 100644
--- a/coroutine-win32.c
+++ b/coroutine-win32.c
@@ -77,7 +77,7 @@ void qemu_coroutine_delete(Coroutine *co_)
     g_free(co);
 }
 
-Coroutine *qemu_coroutine_self(void)
+Coroutine *qemu_coroutine_self_int(void)
 {
     if (!current) {
         current = &leader.base;
diff --git a/include/block/coroutine_int.h b/include/block/coroutine_int.h
index f133d65..f6191ad 100644
--- a/include/block/coroutine_int.h
+++ b/include/block/coroutine_int.h
@@ -48,6 +48,7 @@ Coroutine *qemu_coroutine_new(void);
 void qemu_coroutine_delete(Coroutine *co);
 CoroutineAction qemu_coroutine_switch(Coroutine *from, Coroutine *to,
                                       CoroutineAction action);
+Coroutine *qemu_coroutine_self_int(void);
 void coroutine_fn qemu_co_queue_run_restart(Coroutine *co);
 
 #endif
diff --git a/qemu-coroutine.c b/qemu-coroutine.c
index 4708521..563e6ec 100644
--- a/qemu-coroutine.c
+++ b/qemu-coroutine.c
@@ -108,7 +108,7 @@ static void coroutine_swap(Coroutine *from, Coroutine *to)
 
 void qemu_coroutine_enter(Coroutine *co, void *opaque)
 {
-    Coroutine *self = qemu_coroutine_self();
+    Coroutine *self = qemu_coroutine_self_int();
 
     trace_qemu_coroutine_enter(self, co, opaque);
 
@@ -137,3 +137,16 @@ void coroutine_fn qemu_coroutine_yield(void)
     self->caller = NULL;
     coroutine_swap(self, to);
 }
+
+Coroutine *coroutine_fn qemu_coroutine_self(void)
+{
+    /* Call the internal version of this function, which is
+     * non-coroutine_fn and can therefore be called from from
+     * non-coroutine contexts.  Internally we know it's always possible
+     * to pull a Coroutine* out of thin air (or thread-local storage).
+     * External callers shouldn't assume they can always get a
+     * Coroutine* since we may not be in coroutine context, hence the
+     * external version of this function.
+     */
+    return qemu_coroutine_self_int();
+}
-- 
1.8.4.rc3

Re: [Qemu-devel] [PATCH v3 0/2] Documentation for coroutine annotations

[Gabriel Kerneis]

On Sun, Oct 27, 2013 at 04:23:54PM +0100, Charlie Shepherd wrote:
> These patches were the first two from my GSoC series and were reasonably
> straight-forward and well accepted. Gabriel and I are hoping the patches from
> GSoC can be merged before I start my job in December, so I'm starting by sending
> the simple parts of the overall patchset, when they are merged then I will redo
> the later parts in several smaller and more manageable patchsets.

The patches look good, I just reviewed them again.  They cannot be applied
because you forgot --signoff.

Also, I think it would be more consistent if you added the following patches in
the same series:
- add blocking_fn (I sent it to qemu-devel some time ago),
- protect coroutine_fn and blocking_fn definition with #ifndef (to allow
  redefining them easily on the command line with extra cflags).

Could you resend the series with these suggestions?

Thanks,
-- 
Gabriel

Re: [Qemu-devel] [PATCH v3 0/2] Documentation for coroutine annotations

[Charlie Shepherd]

On 27/10/2013 20:37, Gabriel Kerneis wrote:
> On Sun, Oct 27, 2013 at 04:23:54PM +0100, Charlie Shepherd wrote:
>> These patches were the first two from my GSoC series and were reasonably
>> straight-forward and well accepted. Gabriel and I are hoping the patches from
>> GSoC can be merged before I start my job in December, so I'm starting by sending
>> the simple parts of the overall patchset, when they are merged then I will redo
>> the later parts in several smaller and more manageable patchsets.
> The patches look good, I just reviewed them again.  They cannot be applied
> because you forgot --signoff.
>
> Also, I think it would be more consistent if you added the following patches in
> the same series:
> - add blocking_fn (I sent it to qemu-devel some time ago),
> - protect coroutine_fn and blocking_fn definition with #ifndef (to allow
>    redefining them easily on the command line with extra cflags).
>
> Could you resend the series with these suggestions?

Good points, thanks, resent with your suggested changes.


Charlie