From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58990) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZovkR-0001G0-Ht for qemu-devel@nongnu.org; Wed, 21 Oct 2015 11:56:20 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZovkN-0000xB-AF for qemu-devel@nongnu.org; Wed, 21 Oct 2015 11:56:19 -0400 Date: Wed, 21 Oct 2015 11:56:08 -0400 From: Jeff Cody Message-ID: <20151021155608.GC6466@localhost.localdomain> References: <1445393209-26545-1-git-send-email-famz@redhat.com> <1445393209-26545-5-git-send-email-famz@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1445393209-26545-5-git-send-email-famz@redhat.com> Subject: Re: [Qemu-devel] [Qemu-block] [PATCH v5 04/12] aio: introduce aio_{disable, enable}_external List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Fam Zheng Cc: kwolf@redhat.com, pbonzini@redhat.com, qemu-block@nongnu.org, qemu-devel@nongnu.org, stefanha@redhat.com On Wed, Oct 21, 2015 at 10:06:41AM +0800, Fam Zheng wrote: > Signed-off-by: Fam Zheng > --- > aio-posix.c | 3 ++- > aio-win32.c | 3 ++- > include/block/aio.h | 37 +++++++++++++++++++++++++++++++++++++ > 3 files changed, 41 insertions(+), 2 deletions(-) > > diff --git a/aio-posix.c b/aio-posix.c > index f0f9122..0467f23 100644 > --- a/aio-posix.c > +++ b/aio-posix.c > @@ -261,7 +261,8 @@ bool aio_poll(AioContext *ctx, bool blocking) > > /* fill pollfds */ > QLIST_FOREACH(node, &ctx->aio_handlers, node) { > - if (!node->deleted && node->pfd.events) { > + if (!node->deleted && node->pfd.events > + && aio_node_check(ctx, node->is_external)) { > add_pollfd(node); > } > } > diff --git a/aio-win32.c b/aio-win32.c > index 3110d85..43c4c79 100644 > --- a/aio-win32.c > +++ b/aio-win32.c > @@ -309,7 +309,8 @@ bool aio_poll(AioContext *ctx, bool blocking) > /* fill fd sets */ > count = 0; > QLIST_FOREACH(node, &ctx->aio_handlers, node) { > - if (!node->deleted && node->io_notify) { > + if (!node->deleted && node->io_notify > + && aio_node_check(ctx, node->is_external)) { > events[count++] = event_notifier_get_handle(node->e); > } > } > diff --git a/include/block/aio.h b/include/block/aio.h > index 12f1141..80151d1 100644 > --- a/include/block/aio.h > +++ b/include/block/aio.h > @@ -122,6 +122,8 @@ struct AioContext { > > /* TimerLists for calling timers - one per clock type */ > QEMUTimerListGroup tlg; > + > + int external_disable_cnt; > }; > > /** > @@ -375,4 +377,39 @@ static inline void aio_timer_init(AioContext *ctx, > */ > int64_t aio_compute_timeout(AioContext *ctx); > > +/** > + * aio_disable_external: > + * @ctx: the aio context > + * > + * Disable the furthur processing of clients. s/furthur/further The comment below specifically references external clients - I think the comments for aio_disable_external / aio_enable_external should be worded similarly, so there is no confusion. > + */ > +static inline void aio_disable_external(AioContext *ctx) > +{ > + atomic_inc(&ctx->external_disable_cnt); > +} > + > +/** > + * aio_enable_external: > + * @ctx: the aio context > + * > + * Disable the processing of external clients. Should this comment read "Enable" instead of "Disable"? > + */ > +static inline void aio_enable_external(AioContext *ctx) > +{ > + atomic_dec(&ctx->external_disable_cnt); Should we assert(ctx->external_disable_cnt >= 0)? Additional comment: the function names aio_enable_external() and aio_disable_external() may be a bit misleading (particularly aio_enable_external()). It doesn't do a blanket enable of external aio (i.e., it does not just blindly do ctx->external_disable_cnt = 0). Perhaps something like aio_external_disable_inc/dec()? (I'm not real fond of that, either). Just something for thought. > +} > + > +/** > + * aio_node_check: > + * @ctx: the aio context > + * @is_external: Whether or not the checked node is an external event source. > + * > + * Check if the node's is_external flag is okey to be polled by the ctx at this s/okey/okay > + * moment. True means green light. > + */ > +static inline bool aio_node_check(AioContext *ctx, bool is_external) > +{ > + return !is_external || !atomic_read(&ctx->external_disable_cnt); > +} > + It seems a little odd to me to have this helper function take the is_external bool field from the node as the argument - any reason to do that, rather than pass in the AioHandler and have aio_node_check() parse whatever fields it deems necessary from it? > #endif > -- > 2.4.3 > >