Re: [PATCH RFC v2 3/3] qapi/iothread: introduce poll-weight parameter for aio-poll

[Markus Armbruster <armbru@redhat.com>]

Jaehoon Kim <jhkim@linux.ibm.com> writes:

> Introduce a configurable poll-weight parameter for adaptive polling
> in IOThread. This parameter replaces the hardcoded POLL_WEIGHT_SHIFT
> constant, allowing runtime control over how much the most recent
> event interval affects the next polling duration calculation.
>
> The poll-weight parameter uses a shift value where larger values
> decrease the weight of the current interval, enabling more gradual
> adjustments. When set to 0, a default value of 3 is used (meaning
> the current interval contributes approximately 1/8 to the weighted
> average).
>
> This patch also removes the hardcoded default values for poll-grow
> and poll-shrink parameters from the grow_polling_time() and
> shrink_polling_time() functions, as these defaults are now properly
> initialized in iothread.c during IOThread creation.
>
> Signed-off-by: Jaehoon Kim <jhkim@linux.ibm.com>

[...]

> diff --git a/qapi/misc.json b/qapi/misc.json
> index 28c641fe2f..39d17010bc 100644
> --- a/qapi/misc.json
> +++ b/qapi/misc.json
> @@ -85,6 +85,12 @@

Note: this is IOThreadInfo, used only as return value of
query-iothreads.

>  # @poll-shrink: how many ns will be removed from polling time, 0 means
>  #     that it's not configured (since 2.9)
>  #
> +# @poll-weight: the weight factor for adaptive polling.
> +#     Determines how much the current event interval contributes to
> +#     the next polling time calculation.  Valid values are 1 or
> +#     greater.  0 selects the system default value which is current 3

Does query-iothreads actually return 0?  I'd expect it to return the
value that is actually used.

> +#     (since 10.2)

11.1 most likely.

> +#
>  # @aio-max-batch: maximum number of requests in a batch for the AIO
>  #     engine, 0 means that the engine will use its default (since 6.1)
>  #
> @@ -96,6 +102,7 @@
>             'poll-max-ns': 'int',
>             'poll-grow': 'int',
>             'poll-shrink': 'int',
> +           'poll-weight': 'int',
>             'aio-max-batch': 'int' } }
>  
>  ##
> diff --git a/qapi/qom.json b/qapi/qom.json
> index c653248f85..feb80b6cfe 100644
> --- a/qapi/qom.json
> +++ b/qapi/qom.json
> @@ -606,6 +606,11 @@
>  #     algorithm detects it is spending too long polling without
>  #     encountering events.  0 selects a default behaviour (default: 0)
>  #
> +# @poll-weight: the weight factor for adaptive polling.
> +#     Determines how much the current event interval contributes to
> +#     the next polling time calculation.  Valid values are 1 or
> +#     greater.  If set to 0, the default value of 3 is used.

The commit message hints what the valid values mean, the doc comment
doesn't even that.  Do users need to know?

Code [*] below uses it like time >> poll_weight, where @time is int64_t.
poll_weight > 63 is undefined behavior, which is a no-no.  Please reject
such values.  poll_weight == 64 results in zero.  Is that useful?

Missing: (default: 0) (since 11.1)

> +#
>  # The @aio-max-batch option is available since 6.1.
>  #
>  # Since: 2.0
> @@ -614,7 +619,8 @@
>    'base': 'EventLoopBaseProperties',
>    'data': { '*poll-max-ns': 'int',
>              '*poll-grow': 'int',
> -            '*poll-shrink': 'int' } }
> +            '*poll-shrink': 'int',
> +            '*poll-weight': 'int' } }
>  
>  ##
>  # @MainLoopProperties:
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 69e5a874c1..8ddf6c8d36 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -6413,7 +6413,7 @@ SRST
>  
>              CN=laptop.example.com,O=Example Home,L=London,ST=London,C=GB
>  
> -    ``-object iothread,id=id,poll-max-ns=poll-max-ns,poll-grow=poll-grow,poll-shrink=poll-shrink,aio-max-batch=aio-max-batch``
> +    ``-object iothread,id=id,poll-max-ns=poll-max-ns,poll-grow=poll-grow,poll-shrink=poll-shrink,poll-weight=poll-weight,aio-max-batch=aio-max-batch``
>          Creates a dedicated event loop thread that devices can be
>          assigned to. This is known as an IOThread. By default device
>          emulation happens in vCPU threads or the main event loop thread.
> @@ -6449,6 +6449,11 @@ SRST
>          the polling time when the algorithm detects it is spending too
>          long polling without encountering events.
>  
> +        The ``poll-weight`` parameter is the weight factor used in the
> +        adaptive polling algorithm. It determines how much the most
> +        recent event interval affects the calculation of the next
> +        polling duration.
> +
>          The ``aio-max-batch`` parameter is the maximum number of requests
>          in a batch for the AIO engine, 0 means that the engine will use
>          its default.
> diff --git a/tests/unit/test-nested-aio-poll.c b/tests/unit/test-nested-aio-poll.c
> index 9ab1ad08a7..4c38f36fd4 100644
> --- a/tests/unit/test-nested-aio-poll.c
> +++ b/tests/unit/test-nested-aio-poll.c
> @@ -81,7 +81,7 @@ static void test(void)
>      qemu_set_current_aio_context(td.ctx);
>  
>      /* Enable polling */
> -    aio_context_set_poll_params(td.ctx, 1000000, 2, 2, &error_abort);
> +    aio_context_set_poll_params(td.ctx, 1000000, 2, 2, 3, &error_abort);
>  
>      /* Make the event notifier active (set) right away */
>      event_notifier_init(&td.poll_notifier, 1);
> diff --git a/util/aio-posix.c b/util/aio-posix.c
> index 2b3522f2f9..13b7f94911 100644
> --- a/util/aio-posix.c
> +++ b/util/aio-posix.c
> @@ -29,7 +29,6 @@
>  
>  /* Stop userspace polling on a handler if it isn't active for some time */
>  #define POLL_IDLE_INTERVAL_NS (7 * NANOSECONDS_PER_SECOND)
> -#define POLL_WEIGHT_SHIFT   (3)
>  
>  static void adjust_block_ns(AioContext *ctx, int64_t block_ns);
>  static void grow_polling_time(AioContext *ctx, int64_t block_ns);
> @@ -593,10 +592,6 @@ static void shrink_polling_time(AioContext *ctx, int64_t block_ns)
>      int64_t old = ctx->poll_ns;
>      int64_t shrink = ctx->poll_shrink;
>  
> -    if (shrink == 0) {
> -        shrink = 2;
> -    }
> -
>      if (block_ns < (ctx->poll_ns / shrink)) {
>          ctx->poll_ns /= shrink;
>      }
> @@ -610,10 +605,6 @@ static void grow_polling_time(AioContext *ctx, int64_t block_ns)
>      int64_t old = ctx->poll_ns;
>      int64_t grow = ctx->poll_grow;
>  
> -    if (grow == 0) {
> -        grow = 2;
> -    }
> -
>      if (block_ns > ctx->poll_ns * grow) {
>          ctx->poll_ns = block_ns;
>      } else {
> @@ -640,8 +631,8 @@ static void adjust_block_ns(AioContext *ctx, int64_t block_ns)
>               * poll.ns to smooth out polling time adjustments.
>               */
>              node->poll.ns = node->poll.ns
> -                ? (node->poll.ns - (node->poll.ns >> POLL_WEIGHT_SHIFT))
> -                + (block_ns >> POLL_WEIGHT_SHIFT) : block_ns;
> +                ? (node->poll.ns - (node->poll.ns >> ctx->poll_weight))
> +                + (block_ns >> ctx->poll_weight) : block_ns;

[*] This is the use of @poll-weight referred to above.

>  
>              if (node->poll.ns > ctx->poll_max_ns) {
>                  node->poll.ns = 0;
> @@ -831,7 +822,8 @@ void aio_context_destroy(AioContext *ctx)
>  }
>  
>  void aio_context_set_poll_params(AioContext *ctx, int64_t max_ns,
> -                                 int64_t grow, int64_t shrink, Error **errp)
> +                                 int64_t grow, int64_t shrink,
> +                                 int64_t weight, Error **errp)
>  {
>      AioHandler *node;
>  
> @@ -848,6 +840,7 @@ void aio_context_set_poll_params(AioContext *ctx, int64_t max_ns,
>      ctx->poll_max_ns = max_ns;
>      ctx->poll_grow = grow;
>      ctx->poll_shrink = shrink;
> +    ctx->poll_weight = weight;
>      ctx->poll_ns = 0;
>  
>      aio_notify(ctx);

[...]