On Thu, Mar 26, 2026 at 01:17:44PM -0500, JAEHOON KIM wrote: > On 3/25/2026 12:22 PM, Stefan Hajnoczi wrote: > > On Mon, Mar 23, 2026 at 08:54:49AM -0500, Jaehoon Kim wrote: > > > Nodes are no longer added to poll_aio_handlers when adaptive polling is > > > disabled, preventing unnecessary try_poll_mode() calls. Additionally, > > > aio_poll() skips try_poll_mode() when timeout is 0. > > Skipping when timeout is 0 seems risky to me. VIRTIO devices disable > > guest kicks when polling mode is started. When aio_poll(ctx, > > blocking=false) is called, we will skip polling and > > ctx->fdmon_ops->need_wait(ctx) won't detect an event either. aio_poll() > > will return without noticing that the VIRTIO device's AioHandler is > > ready. > > > > Is skipping when timeout 0 necessary for performance or can it be > > dropped from the patch? > > > > Aside from this: > > > > Reviewed-by: Stefan Hajnoczi <[email protected]> > > Thank you for the review! > Removing the (timeout != 0) check does not cause any issue on my side, > so I plan to remove it in the next version. > > However, I'd like to clarify: when timeout is 0, is there any concern > in the try_poll_mode function below that I might be missing? > I just want to make sure I understand correctly. > > static bool try_poll_mode(AioContext *ctx, AioHandlerList *ready_list, > int64_t *timeout) > { > AioHandler *node; > int64_t max_ns; > > if (QLIST_EMPTY_RCU(&ctx->poll_aio_handlers)) { > return false; > } > > max_ns = 0; > QLIST_FOREACH(node, &ctx->poll_aio_handlers, node_poll) { > max_ns = MAX(max_ns, node->poll.ns); > } > max_ns = qemu_soonest_timeout(*timeout, max_ns); > > if (max_ns && !ctx->fdmon_ops->need_wait(ctx)) {
I think you are pointing out that max_ns will be 0 and therefore polling will be skipped? At least this is what came to mind when I read this code again. That is a problem because the exact scenario I described in my reply to you can already happen in the existing code before your patch :(. Avoiding the timeout != 0 check in your patch would help contain the bug in try_poll_mode() rather than extending it to aio_poll() as well. Thanks for pointing this out! Stefan
signature.asc
Description: PGP signature
