Re: [PATCH v2 4/4] util/event-loop-base: Introduce options to set the thread pool size

2022-03-14 Thread Nicolas Saenz Julienne 
On Mon, 2022-03-14 at 13:35 +, Stefan Hajnoczi wrote:
> On Fri, Mar 11, 2022 at 11:40:30AM +0100, Nicolas Saenz Julienne wrote:
> > On Thu, 2022-03-10 at 10:45 +, Stefan Hajnoczi wrote:
> > > On Thu, Mar 03, 2022 at 04:13:07PM +0100, Nicolas Saenz Julienne wrote:
> > > > @@ -537,10 +546,19 @@
> > > >  # 0 means that the engine will use its default
> > > >  # (default:0, since 6.1)
> > > >  #
> > > > +# @thread-pool-min: minimum number of threads readily available in the 
> > > > thread
> > > > +#   pool
> > > > +#   (default:0, since 6.2)
> > > > +#
> > > > +# @thread-pool-max: maximum number of threads the thread pool can 
> > > > contain
> > > > +#   (default:64, since 6.2)
> > > 
> > > Here and elsewhere:
> > > s/6.2/7.1/
> > 
> > Yes, forgot to mention it was a placeholder, as I wasn't sure what version 
> > to
> > target.
> > 
> > > > @@ -294,6 +314,36 @@ void thread_pool_submit(ThreadPool *pool, 
> > > > ThreadPoolFunc *func, void *arg)
> > > >  thread_pool_submit_aio(pool, func, arg, NULL, NULL);
> > > >  }
> > > >  
> > > > +void thread_pool_update_params(ThreadPool *pool, AioContext *ctx)
> > > > +{
> > > > +qemu_mutex_lock(>lock);
> > > > +
> > > > +pool->min_threads = ctx->thread_pool_min;
> > > > +pool->max_threads = ctx->thread_pool_max;
> > > > +
> > > > +/*
> > > > + * We either have to:
> > > > + *  - Increase the number available of threads until over the 
> > > > min_threads
> > > > + *threshold.
> > > > + *  - Decrease the number of available threads until under the 
> > > > max_threads
> > > > + *threshold.
> > > > + *  - Do nothing. the current number of threads fall in between 
> > > > the min and
> > > > + *max thresholds. We'll let the pool manage itself.
> > > > + */
> > > > +for (int i = pool->cur_threads; i < pool->min_threads; i++) {
> > > > +spawn_thread(pool);
> > > > +}
> > > > +
> > > > +while (pool->cur_threads > pool->max_threads) {
> > > > +qemu_sem_post(>sem);
> > > > +qemu_mutex_unlock(>lock);
> > > > +qemu_cond_wait(>worker_stopped, >lock);
> > > > +qemu_mutex_lock(>lock);
> > > 
> > > Same question as Patch 1. This looks incorrect because qemu_cond_wait()
> > > already drops pool->lock if it needs to block.
> > 
> > Yes, I'll fix that.
> > 
> > > Also, why wait? If worker threads are blocked for some reason then our
> > > thread will block too.
> > 
> > Exiting thread_pool_update_params() before honoring the new constraints is a
> > source of potential race conditions (having to worry for situations where
> > cur_threads > max_threads), and on systems where determinism is important 
> > it's
> > crucial to have a clear boundary between 'unsafe' and 'safe' states.
> 
> On the other hand it creates a reliability problem where a random worker
> thread can block all of QEMU. Maybe it's better to let a blocked worker
> thread terminate eventually than to hang QEMU?

OK, fair enough. I'll switch to that behaviour.

Thanks!

-- 
Nicolás Sáenz

Re: [PATCH v2 4/4] util/event-loop-base: Introduce options to set the thread pool size

2022-03-14 Thread Stefan Hajnoczi 
On Fri, Mar 11, 2022 at 11:40:30AM +0100, Nicolas Saenz Julienne wrote:
> On Thu, 2022-03-10 at 10:45 +, Stefan Hajnoczi wrote:
> > On Thu, Mar 03, 2022 at 04:13:07PM +0100, Nicolas Saenz Julienne wrote:
> > > @@ -537,10 +546,19 @@
> > >  # 0 means that the engine will use its default
> > >  # (default:0, since 6.1)
> > >  #
> > > +# @thread-pool-min: minimum number of threads readily available in the 
> > > thread
> > > +#   pool
> > > +#   (default:0, since 6.2)
> > > +#
> > > +# @thread-pool-max: maximum number of threads the thread pool can contain
> > > +#   (default:64, since 6.2)
> > 
> > Here and elsewhere:
> > s/6.2/7.1/
> 
> Yes, forgot to mention it was a placeholder, as I wasn't sure what version to
> target.
> 
> > > @@ -294,6 +314,36 @@ void thread_pool_submit(ThreadPool *pool, 
> > > ThreadPoolFunc *func, void *arg)
> > >  thread_pool_submit_aio(pool, func, arg, NULL, NULL);
> > >  }
> > >  
> > > +void thread_pool_update_params(ThreadPool *pool, AioContext *ctx)
> > > +{
> > > +qemu_mutex_lock(>lock);
> > > +
> > > +pool->min_threads = ctx->thread_pool_min;
> > > +pool->max_threads = ctx->thread_pool_max;
> > > +
> > > +/*
> > > + * We either have to:
> > > + *  - Increase the number available of threads until over the 
> > > min_threads
> > > + *threshold.
> > > + *  - Decrease the number of available threads until under the 
> > > max_threads
> > > + *threshold.
> > > + *  - Do nothing. the current number of threads fall in between the 
> > > min and
> > > + *max thresholds. We'll let the pool manage itself.
> > > + */
> > > +for (int i = pool->cur_threads; i < pool->min_threads; i++) {
> > > +spawn_thread(pool);
> > > +}
> > > +
> > > +while (pool->cur_threads > pool->max_threads) {
> > > +qemu_sem_post(>sem);
> > > +qemu_mutex_unlock(>lock);
> > > +qemu_cond_wait(>worker_stopped, >lock);
> > > +qemu_mutex_lock(>lock);
> > 
> > Same question as Patch 1. This looks incorrect because qemu_cond_wait()
> > already drops pool->lock if it needs to block.
> 
> Yes, I'll fix that.
> 
> > Also, why wait? If worker threads are blocked for some reason then our
> > thread will block too.
> 
> Exiting thread_pool_update_params() before honoring the new constraints is a
> source of potential race conditions (having to worry for situations where
> cur_threads > max_threads), and on systems where determinism is important it's
> crucial to have a clear boundary between 'unsafe' and 'safe' states.

On the other hand it creates a reliability problem where a random worker
thread can block all of QEMU. Maybe it's better to let a blocked worker
thread terminate eventually than to hang QEMU?

Stefan


signature.asc
Description: PGP signature

Re: [PATCH v2 4/4] util/event-loop-base: Introduce options to set the thread pool size

2022-03-11 Thread Nicolas Saenz Julienne 
On Thu, 2022-03-10 at 10:45 +, Stefan Hajnoczi wrote:
> On Thu, Mar 03, 2022 at 04:13:07PM +0100, Nicolas Saenz Julienne wrote:
> > @@ -537,10 +546,19 @@
> >  # 0 means that the engine will use its default
> >  # (default:0, since 6.1)
> >  #
> > +# @thread-pool-min: minimum number of threads readily available in the 
> > thread
> > +#   pool
> > +#   (default:0, since 6.2)
> > +#
> > +# @thread-pool-max: maximum number of threads the thread pool can contain
> > +#   (default:64, since 6.2)
> 
> Here and elsewhere:
> s/6.2/7.1/

Yes, forgot to mention it was a placeholder, as I wasn't sure what version to
target.

> > @@ -294,6 +314,36 @@ void thread_pool_submit(ThreadPool *pool, 
> > ThreadPoolFunc *func, void *arg)
> >  thread_pool_submit_aio(pool, func, arg, NULL, NULL);
> >  }
> >  
> > +void thread_pool_update_params(ThreadPool *pool, AioContext *ctx)
> > +{
> > +qemu_mutex_lock(>lock);
> > +
> > +pool->min_threads = ctx->thread_pool_min;
> > +pool->max_threads = ctx->thread_pool_max;
> > +
> > +/*
> > + * We either have to:
> > + *  - Increase the number available of threads until over the 
> > min_threads
> > + *threshold.
> > + *  - Decrease the number of available threads until under the 
> > max_threads
> > + *threshold.
> > + *  - Do nothing. the current number of threads fall in between the 
> > min and
> > + *max thresholds. We'll let the pool manage itself.
> > + */
> > +for (int i = pool->cur_threads; i < pool->min_threads; i++) {
> > +spawn_thread(pool);
> > +}
> > +
> > +while (pool->cur_threads > pool->max_threads) {
> > +qemu_sem_post(>sem);
> > +qemu_mutex_unlock(>lock);
> > +qemu_cond_wait(>worker_stopped, >lock);
> > +qemu_mutex_lock(>lock);
> 
> Same question as Patch 1. This looks incorrect because qemu_cond_wait()
> already drops pool->lock if it needs to block.

Yes, I'll fix that.

> Also, why wait? If worker threads are blocked for some reason then our
> thread will block too.

Exiting thread_pool_update_params() before honoring the new constraints is a
source of potential race conditions (having to worry for situations where
cur_threads > max_threads), and on systems where determinism is important it's
crucial to have a clear boundary between 'unsafe' and 'safe' states.

Thanks,

-- 
Nicolás Sáenz

Re: [PATCH v2 4/4] util/event-loop-base: Introduce options to set the thread pool size

2022-03-10 Thread Stefan Hajnoczi 
On Thu, Mar 03, 2022 at 04:13:07PM +0100, Nicolas Saenz Julienne wrote:
> @@ -537,10 +546,19 @@
>  # 0 means that the engine will use its default
>  # (default:0, since 6.1)
>  #
> +# @thread-pool-min: minimum number of threads readily available in the thread
> +#   pool
> +#   (default:0, since 6.2)
> +#
> +# @thread-pool-max: maximum number of threads the thread pool can contain
> +#   (default:64, since 6.2)

Here and elsewhere:
s/6.2/7.1/

> @@ -294,6 +314,36 @@ void thread_pool_submit(ThreadPool *pool, ThreadPoolFunc 
> *func, void *arg)
>  thread_pool_submit_aio(pool, func, arg, NULL, NULL);
>  }
>  
> +void thread_pool_update_params(ThreadPool *pool, AioContext *ctx)
> +{
> +qemu_mutex_lock(>lock);
> +
> +pool->min_threads = ctx->thread_pool_min;
> +pool->max_threads = ctx->thread_pool_max;
> +
> +/*
> + * We either have to:
> + *  - Increase the number available of threads until over the min_threads
> + *threshold.
> + *  - Decrease the number of available threads until under the 
> max_threads
> + *threshold.
> + *  - Do nothing. the current number of threads fall in between the min 
> and
> + *max thresholds. We'll let the pool manage itself.
> + */
> +for (int i = pool->cur_threads; i < pool->min_threads; i++) {
> +spawn_thread(pool);
> +}
> +
> +while (pool->cur_threads > pool->max_threads) {
> +qemu_sem_post(>sem);
> +qemu_mutex_unlock(>lock);
> +qemu_cond_wait(>worker_stopped, >lock);
> +qemu_mutex_lock(>lock);

Same question as Patch 1. This looks incorrect because qemu_cond_wait()
already drops pool->lock if it needs to block.

Also, why wait? If worker threads are blocked for some reason then our
thread will block too.


signature.asc
Description: PGP signature

[PATCH v2 4/4] util/event-loop-base: Introduce options to set the thread pool size

2022-03-03 Thread Nicolas Saenz Julienne 
The thread pool regulates itself: when idle, it kills threads until
empty, when in demand, it creates new threads until full. This behaviour
doesn't play well with latency sensitive workloads where the price of
creating a new thread is too high. For example, when paired with qemu's
'-mlock', or using safety features like SafeStack, creating a new thread
has been measured take multiple milliseconds.

In order to mitigate this let's introduce a new 'EventLoopBaase'
property to set the thread pool size. The threads will be created during
the pool's initialization or upon updating the property's value, remain
available during its lifetime regardless of demand, and destroyed upon
freeing it. A properly characterized workload will then be able to
configure the pool to avoid any latency spikes.

Signed-off-by: Nicolas Saenz Julienne 
---

Changes since v1:
 - Add INT_MAX check
 - Have copy of thread pool sizes in AioContext to properly decouple
   both instances
 - More coherent variable naming
 - Handle case where max_threads decreases
 - Code comments

 event-loop-base.c| 23 +
 include/block/aio.h  | 10 ++
 include/block/thread-pool.h  |  3 ++
 include/sysemu/event-loop-base.h |  4 +++
 iothread.c   |  3 ++
 qapi/qom.json| 22 ++--
 util/aio-posix.c |  1 +
 util/async.c | 20 +++
 util/main-loop.c |  9 +
 util/thread-pool.c   | 59 +---
 10 files changed, 148 insertions(+), 6 deletions(-)

diff --git a/event-loop-base.c b/event-loop-base.c
index e7f99a6ec8..d5be4dc6fc 100644
--- a/event-loop-base.c
+++ b/event-loop-base.c
@@ -14,6 +14,7 @@
 #include "qemu/osdep.h"
 #include "qom/object_interfaces.h"
 #include "qapi/error.h"
+#include "block/thread-pool.h"
 #include "sysemu/event-loop-base.h"
 
 typedef struct {
@@ -21,9 +22,22 @@ typedef struct {
 ptrdiff_t offset; /* field's byte offset in EventLoopBase struct */
 } EventLoopBaseParamInfo;
 
+static void event_loop_base_instance_init(Object *obj)
+{
+EventLoopBase *base = EVENT_LOOP_BASE(obj);
+
+base->thread_pool_max = THREAD_POOL_MAX_THREADS_DEFAULT;
+}
+
 static EventLoopBaseParamInfo aio_max_batch_info = {
 "aio-max-batch", offsetof(EventLoopBase, aio_max_batch),
 };
+static EventLoopBaseParamInfo thread_pool_min_info = {
+"thread-pool-min", offsetof(EventLoopBase, thread_pool_min),
+};
+static EventLoopBaseParamInfo thread_pool_max_info = {
+"thread-pool-max", offsetof(EventLoopBase, thread_pool_max),
+};
 
 static void event_loop_base_get_param(Object *obj, Visitor *v,
 const char *name, void *opaque, Error **errp)
@@ -95,12 +109,21 @@ static void event_loop_base_class_init(ObjectClass *klass, 
void *class_data)
   event_loop_base_get_param,
   event_loop_base_set_param,
   NULL, _max_batch_info);
+object_class_property_add(klass, "thread-pool-min", "int",
+  event_loop_base_get_param,
+  event_loop_base_set_param,
+  NULL, _pool_min_info);
+object_class_property_add(klass, "thread-pool-max", "int",
+  event_loop_base_get_param,
+  event_loop_base_set_param,
+  NULL, _pool_max_info);
 }
 
 static const TypeInfo event_loop_base_info = {
 .name = TYPE_EVENT_LOOP_BASE,
 .parent = TYPE_OBJECT,
 .instance_size = sizeof(EventLoopBase),
+.instance_init = event_loop_base_instance_init,
 .class_size = sizeof(EventLoopBaseClass),
 .class_init = event_loop_base_class_init,
 .abstract = true,
diff --git a/include/block/aio.h b/include/block/aio.h
index 5634173b12..d128558f1d 100644
--- a/include/block/aio.h
+++ b/include/block/aio.h
@@ -192,6 +192,8 @@ struct AioContext {
 QSLIST_HEAD(, Coroutine) scheduled_coroutines;
 QEMUBH *co_schedule_bh;
 
+int thread_pool_min;
+int thread_pool_max;
 /* Thread pool for performing work and receiving completion callbacks.
  * Has its own locking.
  */
@@ -769,4 +771,12 @@ void aio_context_set_poll_params(AioContext *ctx, int64_t 
max_ns,
 void aio_context_set_aio_params(AioContext *ctx, int64_t max_batch,
 Error **errp);
 
+/**
+ * aio_context_set_thread_pool_params:
+ * @ctx: the aio context
+ * @min: min number of threads to have readily available in the thread pool
+ * @min: max number of threads the thread pool can contain
+ */
+void aio_context_set_thread_pool_params(AioContext *ctx, int64_t min,
+int64_t max, Error **errp);
 #endif
diff --git a/include/block/thread-pool.h b/include/block/thread-pool.h
index 7dd7d730a0..2020bcc92d 100644
--- a/include/block/thread-pool.h
+++ b/include/block/thread-pool.h
@@ -20,6