qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: Stefan Hajnoczi <stefanha@gmail.com>
To: Paolo Bonzini <pbonzini@redhat.com>
Cc: qemu-devel@nongnu.org, famz@redhat.com
Subject: Re: [Qemu-devel] [PATCH 4/5] coroutine-lock: make qemu_co_enter_next thread-safe
Date: Mon, 29 Jan 2018 13:26:21 +0000	[thread overview]
Message-ID: <20180129132621.GD20446@stefanha-x1.localdomain> (raw)
In-Reply-To: <20180125175949.7780-5-pbonzini@redhat.com>

[-- Attachment #1: Type: text/plain, Size: 4643 bytes --]

On Thu, Jan 25, 2018 at 06:59:48PM +0100, Paolo Bonzini wrote:
> qemu_co_queue_next does not need to release and re-acquire the mutex,
> because the queued coroutine does not run immediately.  However, this
> does not hold for qemu_co_enter_next.  Now that qemu_co_queue_wait
> can synchronize (via QemuLockable) with code that is not running in
> coroutine context, it's important that code using qemu_co_enter_next
> can easily use a standardized locking idiom.
> 
> First of all, qemu_co_enter_next must use aio_co_wake to restart the
> coroutine.  Second, the function gains a second argument, a QemuLockable*,
> and the comments of qemu_co_queue_next and qemu_co_queue_restart_all
> are adjusted to clarify the difference.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  fsdev/qemu-fsdev-throttle.c |  4 ++--
>  include/qemu/coroutine.h    | 15 ++++++++++-----
>  util/qemu-coroutine-lock.c  | 10 ++++++++--
>  3 files changed, 20 insertions(+), 9 deletions(-)
> 
> diff --git a/fsdev/qemu-fsdev-throttle.c b/fsdev/qemu-fsdev-throttle.c
> index 49eebb5412..1dc07fbc12 100644
> --- a/fsdev/qemu-fsdev-throttle.c
> +++ b/fsdev/qemu-fsdev-throttle.c
> @@ -20,13 +20,13 @@
>  static void fsdev_throttle_read_timer_cb(void *opaque)
>  {
>      FsThrottle *fst = opaque;
> -    qemu_co_enter_next(&fst->throttled_reqs[false]);
> +    qemu_co_enter_next(&fst->throttled_reqs[false], NULL);
>  }
>  
>  static void fsdev_throttle_write_timer_cb(void *opaque)
>  {
>      FsThrottle *fst = opaque;
> -    qemu_co_enter_next(&fst->throttled_reqs[true]);
> +    qemu_co_enter_next(&fst->throttled_reqs[true], NULL);
>  }
>  
>  void fsdev_throttle_parse_opts(QemuOpts *opts, FsThrottle *fst, Error **errp)
> diff --git a/include/qemu/coroutine.h b/include/qemu/coroutine.h
> index 1e5f0957e6..6fdbc837c0 100644
> --- a/include/qemu/coroutine.h
> +++ b/include/qemu/coroutine.h
> @@ -188,21 +188,26 @@ void qemu_co_queue_init(CoQueue *queue);
>  void coroutine_fn qemu_co_queue_wait_impl(CoQueue *queue, QemuLockable *lock);
>  
>  /**
> - * Restarts the next coroutine in the CoQueue and removes it from the queue.
> + * Removes the next coroutine from the CoQueue; it will run as soon as the
> + * current one yields.
>   *
> - * Returns true if a coroutine was restarted, false if the queue is empty.
> + * Returns true if a coroutine was removed, false if the queue is empty.
>   */
>  bool coroutine_fn qemu_co_queue_next(CoQueue *queue);
>  
>  /**
> - * Restarts all coroutines in the CoQueue and leaves the queue empty.
> + * Empties the CoQueue; all coroutines in it will run in FIFO orer as soon
> + * as the current one yields.
>   */
>  void coroutine_fn qemu_co_queue_restart_all(CoQueue *queue);
>  
>  /**
> - * Enter the next coroutine in the queue
> + * Immediately enter the next coroutine in the queue.  Release the mutex
> + * while it runs.

s/mutex/lock/

Is this doc comment correct?  I see multiple cases due to aio_co_wake():

1. Called from coroutine context with current AioContext matching next
   coroutine's AioContext: arrange for next coroutine to be entered
   *after* we yield (not "immediately").

2. Called from non-coroutine context with current AioContext matching
   next coroutine's AioContext: immediately enter next coroutine.

3. Called from different AioContext than next coroutine: arrange for
   next coroutine to be entered at some point.

>   */
> -bool qemu_co_enter_next(CoQueue *queue);
> +#define qemu_co_enter_next(queue, lock) \
> +    qemu_co_enter_next_impl(queue, QEMU_MAKE_LOCKABLE(lock))
> +bool qemu_co_enter_next_impl(CoQueue *queue, QemuLockable *lock);
>  
>  /**
>   * Checks if the CoQueue is empty.
> diff --git a/util/qemu-coroutine-lock.c b/util/qemu-coroutine-lock.c
> index 2a66fc1467..78fb79acf8 100644
> --- a/util/qemu-coroutine-lock.c
> +++ b/util/qemu-coroutine-lock.c
> @@ -132,7 +132,7 @@ void coroutine_fn qemu_co_queue_restart_all(CoQueue *queue)
>      qemu_co_queue_do_restart(queue, false);
>  }
>  
> -bool qemu_co_enter_next(CoQueue *queue)
> +bool qemu_co_enter_next_impl(CoQueue *queue, QemuLockable *lock)
>  {
>      Coroutine *next;
>  
> @@ -142,7 +142,13 @@ bool qemu_co_enter_next(CoQueue *queue)
>      }
>  
>      QSIMPLEQ_REMOVE_HEAD(&queue->entries, co_queue_next);
> -    qemu_coroutine_enter(next);
> +    if (lock) {
> +        qemu_lockable_unlock(lock);
> +    }
> +    aio_co_wake(next);
> +    if (lock) {
> +        qemu_lockable_lock(lock);
> +    }
>      return true;
>  }
>  
> -- 
> 2.14.3
> 
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

  parent reply	other threads:[~2018-01-29 13:26 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-01-25 17:59 [Qemu-devel] [PATCH v3 0/5] coroutine-lock: polymorphic CoQueue Paolo Bonzini
2018-01-25 17:59 ` [Qemu-devel] [PATCH 1/5] test-coroutine: add simple CoMutex test Paolo Bonzini
2018-01-29 13:26   ` Stefan Hajnoczi
2018-01-25 17:59 ` [Qemu-devel] [PATCH 2/5] lockable: add QemuLockable Paolo Bonzini
2018-01-25 20:13   ` Eric Blake
2018-01-26  3:11   ` Fam Zheng
2018-01-26  5:24   ` Fam Zheng
2018-01-29 11:42   ` Stefan Hajnoczi
2018-01-29 14:30     ` Paolo Bonzini
2018-01-25 17:59 ` [Qemu-devel] [PATCH 3/5] coroutine-lock: convert CoQueue to use QemuLockable Paolo Bonzini
2018-01-29 13:26   ` Stefan Hajnoczi
2018-01-25 17:59 ` [Qemu-devel] [PATCH 4/5] coroutine-lock: make qemu_co_enter_next thread-safe Paolo Bonzini
2018-01-25 20:15   ` Eric Blake
2018-01-29 13:26   ` Stefan Hajnoczi [this message]
2018-02-01 15:28     ` Paolo Bonzini
2018-01-25 17:59 ` [Qemu-devel] [PATCH 5/5] curl: convert to CoQueue Paolo Bonzini
2018-01-29 13:27   ` Stefan Hajnoczi
2018-01-25 18:08 ` [Qemu-devel] [PATCH v3 0/5] coroutine-lock: polymorphic CoQueue no-reply
2018-01-25 20:31 ` Eric Blake
  -- strict thread matches above, loose matches on Subject: below --
2018-02-01 21:29 [Qemu-devel] [PATCH v4 " Paolo Bonzini
2018-02-01 21:29 ` [Qemu-devel] [PATCH 4/5] coroutine-lock: make qemu_co_enter_next thread-safe Paolo Bonzini
2018-02-03 15:39 [Qemu-devel] [PATCH v5 0/5] coroutine-lock: polymorphic CoQueue Paolo Bonzini
2018-02-03 15:39 ` [Qemu-devel] [PATCH 4/5] coroutine-lock: make qemu_co_enter_next thread-safe Paolo Bonzini
2018-02-03 20:57   ` Richard Henderson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180129132621.GD20446@stefanha-x1.localdomain \
    --to=stefanha@gmail.com \
    --cc=famz@redhat.com \
    --cc=pbonzini@redhat.com \
    --cc=qemu-devel@nongnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).