Patchwork [07/10] coroutine: split qemu-coroutine-lock.c

login
register
mail settings
Submitter Lai Jiangshan
Date April 3, 2012, 8:38 a.m.
Message ID <1333442297-18932-7-git-send-email-laijs@cn.fujitsu.com>
Download mbox | patch
Permalink /patch/150360/
State New
Headers show

Comments

Lai Jiangshan - April 3, 2012, 8:38 a.m.
queues are not just internal things for locks, split them.

Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
---
 Makefile.objs          |    2 +-
 qemu-coroutine-lock.c  |   49 +------------------------------
 qemu-coroutine-queue.c |   76 ++++++++++++++++++++++++++++++++++++++++++++++++
 trace-events           |    4 ++-
 4 files changed, 81 insertions(+), 50 deletions(-)
 create mode 100644 qemu-coroutine-queue.c
Paolo Bonzini - April 3, 2012, 11:32 a.m.
Il 03/04/2012 10:38, Lai Jiangshan ha scritto:
> queues are not just internal things for locks, split them.
> 
> Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
> ---
>  Makefile.objs          |    2 +-
>  qemu-coroutine-lock.c  |   49 +------------------------------
>  qemu-coroutine-queue.c |   76 ++++++++++++++++++++++++++++++++++++++++++++++++
>  trace-events           |    4 ++-
>  4 files changed, 81 insertions(+), 50 deletions(-)
>  create mode 100644 qemu-coroutine-queue.c
> 
> diff --git a/Makefile.objs b/Makefile.objs
> index 226b01d..1f0cc31 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -27,7 +27,7 @@ oslib-obj-$(CONFIG_POSIX) += oslib-posix.o qemu-thread-posix.o
>  #######################################################################
>  # coroutines
>  coroutine-obj-y = qemu-coroutine.o qemu-coroutine-lock.o qemu-coroutine-io.o
> -coroutine-obj-y += qemu-coroutine-sleep.o
> +coroutine-obj-y += qemu-coroutine-sleep.o qemu-coroutine-queue.o
>  ifeq ($(CONFIG_UCONTEXT_COROUTINE),y)
>  coroutine-obj-$(CONFIG_POSIX) += coroutine-ucontext.o
>  else
> diff --git a/qemu-coroutine-lock.c b/qemu-coroutine-lock.c
> index 159d66d..2bfbd41 100644
> --- a/qemu-coroutine-lock.c
> +++ b/qemu-coroutine-lock.c
> @@ -1,5 +1,5 @@
>  /*
> - * coroutine queues and locks
> + * coroutine locks
>   *
>   * Copyright (c) 2011 Kevin Wolf <kwolf@redhat.com>
>   *
> @@ -28,53 +28,6 @@
>  #include "qemu-queue.h"
>  #include "trace.h"
>  
> -void qemu_co_queue_init(CoQueue *queue)
> -{
> -    QTAILQ_INIT(&queue->entries);
> -}
> -
> -void coroutine_fn qemu_co_queue_wait(CoQueue *queue)
> -{
> -    Coroutine *self = qemu_coroutine_self();
> -    QTAILQ_INSERT_TAIL(&queue->entries, self, co_queue_next);
> -    qemu_coroutine_yield();
> -    assert(qemu_in_coroutine());
> -}
> -
> -void coroutine_fn qemu_co_queue_wait_insert_head(CoQueue *queue)
> -{
> -    Coroutine *self = qemu_coroutine_self();
> -    QTAILQ_INSERT_HEAD(&queue->entries, self, co_queue_next);
> -    qemu_coroutine_yield();
> -    assert(qemu_in_coroutine());
> -}
> -
> -bool qemu_co_queue_next(CoQueue *queue)
> -{
> -    Coroutine *next;
> -
> -    next = QTAILQ_FIRST(&queue->entries);
> -    if (next) {
> -        QTAILQ_REMOVE(&queue->entries, next, co_queue_next);
> -        trace_qemu_co_queue_next(next);
> -        qemu_co_runnable_schedule(next);
> -    }
> -
> -    return (next != NULL);
> -}
> -
> -void qemu_co_queue_restart_all(CoQueue *queue)
> -{
> -    while (qemu_co_queue_next(queue)) {
> -        /* Do nothing */
> -    }
> -}
> -
> -bool qemu_co_queue_empty(CoQueue *queue)
> -{
> -    return (QTAILQ_FIRST(&queue->entries) == NULL);
> -}
> -
>  void qemu_co_mutex_init(CoMutex *mutex)
>  {
>      memset(mutex, 0, sizeof(*mutex));
> diff --git a/qemu-coroutine-queue.c b/qemu-coroutine-queue.c
> new file mode 100644
> index 0000000..b90818d
> --- /dev/null
> +++ b/qemu-coroutine-queue.c
> @@ -0,0 +1,76 @@
> +/*
> + * coroutine queues
> + *
> + * Copyright (c) 2011 Kevin Wolf <kwolf@redhat.com>
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#include "qemu-common.h"
> +#include "qemu-coroutine.h"
> +#include "qemu-coroutine-int.h"
> +#include "qemu-queue.h"
> +#include "trace.h"
> +
> +void qemu_co_queue_init(CoQueue *queue)
> +{
> +    QTAILQ_INIT(&queue->entries);
> +}
> +
> +void coroutine_fn qemu_co_queue_wait(CoQueue *queue)
> +{
> +    Coroutine *self = qemu_coroutine_self();
> +    QTAILQ_INSERT_TAIL(&queue->entries, self, co_queue_next);
> +    qemu_coroutine_yield();
> +    assert(qemu_in_coroutine());
> +}
> +
> +void coroutine_fn qemu_co_queue_wait_insert_head(CoQueue *queue)
> +{
> +    Coroutine *self = qemu_coroutine_self();
> +    QTAILQ_INSERT_HEAD(&queue->entries, self, co_queue_next);
> +    qemu_coroutine_yield();
> +    assert(qemu_in_coroutine());
> +}
> +
> +bool qemu_co_queue_next(CoQueue *queue)
> +{
> +    Coroutine *next;
> +
> +    next = QTAILQ_FIRST(&queue->entries);
> +    if (next) {
> +        QTAILQ_REMOVE(&queue->entries, next, co_queue_next);
> +        trace_qemu_co_queue_next(next);
> +        qemu_co_runnable_schedule(next);
> +    }
> +
> +    return (next != NULL);
> +}
> +
> +void qemu_co_queue_restart_all(CoQueue *queue)
> +{
> +    while (qemu_co_queue_next(queue)) {
> +        /* Do nothing */
> +    }
> +}
> +
> +bool qemu_co_queue_empty(CoQueue *queue)
> +{
> +    return (QTAILQ_FIRST(&queue->entries) == NULL);
> +}
> diff --git a/trace-events b/trace-events
> index 39b9c84..b35e8de 100644
> --- a/trace-events
> +++ b/trace-events
> @@ -561,8 +561,10 @@ qemu_coroutine_enter(void *from, void *to, void *opaque) "from %p to %p opaque %
>  qemu_coroutine_yield(void *from, void *to) "from %p to %p"
>  qemu_coroutine_terminate(void *co) "self %p"
>  
> -# qemu-coroutine-lock.c
> +# qemu-coroutine-queue.c
>  qemu_co_queue_next(void *next) "next %p"
> +
> +# qemu-coroutine-lock.c
>  qemu_co_mutex_lock_entry(void *mutex, void *self) "mutex %p self %p"
>  qemu_co_mutex_lock_return(void *mutex, void *self) "mutex %p self %p"
>  qemu_co_mutex_unlock_entry(void *mutex, void *self) "mutex %p self %p"

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

... and you could move the runnable bits to the new file too.

Paolo
Kevin Wolf - April 3, 2012, 11:47 a.m.
Am 03.04.2012 10:38, schrieb Lai Jiangshan:
> queues are not just internal things for locks, split them.
> 
> Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
> ---
>  Makefile.objs          |    2 +-
>  qemu-coroutine-lock.c  |   49 +------------------------------
>  qemu-coroutine-queue.c |   76 ++++++++++++++++++++++++++++++++++++++++++++++++
>  trace-events           |    4 ++-
>  4 files changed, 81 insertions(+), 50 deletions(-)

What's the point? qemu-coroutine-lock.c is already a small file (171
lines) and all functions in it are doing related things (they
synchronise coroutines). Splitting a small cohesive file into two tiny
halves isn't an improvement, IMO.

Kevin

Patch

diff --git a/Makefile.objs b/Makefile.objs
index 226b01d..1f0cc31 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -27,7 +27,7 @@  oslib-obj-$(CONFIG_POSIX) += oslib-posix.o qemu-thread-posix.o
 #######################################################################
 # coroutines
 coroutine-obj-y = qemu-coroutine.o qemu-coroutine-lock.o qemu-coroutine-io.o
-coroutine-obj-y += qemu-coroutine-sleep.o
+coroutine-obj-y += qemu-coroutine-sleep.o qemu-coroutine-queue.o
 ifeq ($(CONFIG_UCONTEXT_COROUTINE),y)
 coroutine-obj-$(CONFIG_POSIX) += coroutine-ucontext.o
 else
diff --git a/qemu-coroutine-lock.c b/qemu-coroutine-lock.c
index 159d66d..2bfbd41 100644
--- a/qemu-coroutine-lock.c
+++ b/qemu-coroutine-lock.c
@@ -1,5 +1,5 @@ 
 /*
- * coroutine queues and locks
+ * coroutine locks
  *
  * Copyright (c) 2011 Kevin Wolf <kwolf@redhat.com>
  *
@@ -28,53 +28,6 @@ 
 #include "qemu-queue.h"
 #include "trace.h"
 
-void qemu_co_queue_init(CoQueue *queue)
-{
-    QTAILQ_INIT(&queue->entries);
-}
-
-void coroutine_fn qemu_co_queue_wait(CoQueue *queue)
-{
-    Coroutine *self = qemu_coroutine_self();
-    QTAILQ_INSERT_TAIL(&queue->entries, self, co_queue_next);
-    qemu_coroutine_yield();
-    assert(qemu_in_coroutine());
-}
-
-void coroutine_fn qemu_co_queue_wait_insert_head(CoQueue *queue)
-{
-    Coroutine *self = qemu_coroutine_self();
-    QTAILQ_INSERT_HEAD(&queue->entries, self, co_queue_next);
-    qemu_coroutine_yield();
-    assert(qemu_in_coroutine());
-}
-
-bool qemu_co_queue_next(CoQueue *queue)
-{
-    Coroutine *next;
-
-    next = QTAILQ_FIRST(&queue->entries);
-    if (next) {
-        QTAILQ_REMOVE(&queue->entries, next, co_queue_next);
-        trace_qemu_co_queue_next(next);
-        qemu_co_runnable_schedule(next);
-    }
-
-    return (next != NULL);
-}
-
-void qemu_co_queue_restart_all(CoQueue *queue)
-{
-    while (qemu_co_queue_next(queue)) {
-        /* Do nothing */
-    }
-}
-
-bool qemu_co_queue_empty(CoQueue *queue)
-{
-    return (QTAILQ_FIRST(&queue->entries) == NULL);
-}
-
 void qemu_co_mutex_init(CoMutex *mutex)
 {
     memset(mutex, 0, sizeof(*mutex));
diff --git a/qemu-coroutine-queue.c b/qemu-coroutine-queue.c
new file mode 100644
index 0000000..b90818d
--- /dev/null
+++ b/qemu-coroutine-queue.c
@@ -0,0 +1,76 @@ 
+/*
+ * coroutine queues
+ *
+ * Copyright (c) 2011 Kevin Wolf <kwolf@redhat.com>
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qemu-common.h"
+#include "qemu-coroutine.h"
+#include "qemu-coroutine-int.h"
+#include "qemu-queue.h"
+#include "trace.h"
+
+void qemu_co_queue_init(CoQueue *queue)
+{
+    QTAILQ_INIT(&queue->entries);
+}
+
+void coroutine_fn qemu_co_queue_wait(CoQueue *queue)
+{
+    Coroutine *self = qemu_coroutine_self();
+    QTAILQ_INSERT_TAIL(&queue->entries, self, co_queue_next);
+    qemu_coroutine_yield();
+    assert(qemu_in_coroutine());
+}
+
+void coroutine_fn qemu_co_queue_wait_insert_head(CoQueue *queue)
+{
+    Coroutine *self = qemu_coroutine_self();
+    QTAILQ_INSERT_HEAD(&queue->entries, self, co_queue_next);
+    qemu_coroutine_yield();
+    assert(qemu_in_coroutine());
+}
+
+bool qemu_co_queue_next(CoQueue *queue)
+{
+    Coroutine *next;
+
+    next = QTAILQ_FIRST(&queue->entries);
+    if (next) {
+        QTAILQ_REMOVE(&queue->entries, next, co_queue_next);
+        trace_qemu_co_queue_next(next);
+        qemu_co_runnable_schedule(next);
+    }
+
+    return (next != NULL);
+}
+
+void qemu_co_queue_restart_all(CoQueue *queue)
+{
+    while (qemu_co_queue_next(queue)) {
+        /* Do nothing */
+    }
+}
+
+bool qemu_co_queue_empty(CoQueue *queue)
+{
+    return (QTAILQ_FIRST(&queue->entries) == NULL);
+}
diff --git a/trace-events b/trace-events
index 39b9c84..b35e8de 100644
--- a/trace-events
+++ b/trace-events
@@ -561,8 +561,10 @@  qemu_coroutine_enter(void *from, void *to, void *opaque) "from %p to %p opaque %
 qemu_coroutine_yield(void *from, void *to) "from %p to %p"
 qemu_coroutine_terminate(void *co) "self %p"
 
-# qemu-coroutine-lock.c
+# qemu-coroutine-queue.c
 qemu_co_queue_next(void *next) "next %p"
+
+# qemu-coroutine-lock.c
 qemu_co_mutex_lock_entry(void *mutex, void *self) "mutex %p self %p"
 qemu_co_mutex_lock_return(void *mutex, void *self) "mutex %p self %p"
 qemu_co_mutex_unlock_entry(void *mutex, void *self) "mutex %p self %p"