| Message ID | 1376598879-18976-14-git-send-email-alex@alex.org.uk |
|---|---|
| State | New |
| Headers | show |
于 2013-8-16 4:34, Alex Bligh 写道: > Calculate the timeout in aio_ctx_prepare taking into account > the timers attached to the AioContext. > > Alter aio_ctx_check similarly. > > Signed-off-by: Alex Bligh <alex@alex.org.uk> > --- > async.c | 13 +++++++++++-- > 1 file changed, 11 insertions(+), 2 deletions(-) > > diff --git a/async.c b/async.c > index 2b9ba9b..d8656cc 100644 > --- a/async.c > +++ b/async.c > @@ -150,13 +150,14 @@ aio_ctx_prepare(GSource *source, gint *timeout) > { > AioContext *ctx = (AioContext *) source; > QEMUBH *bh; > + int deadline; > > for (bh = ctx->first_bh; bh; bh = bh->next) { > if (!bh->deleted && bh->scheduled) { > if (bh->idle) { > /* idle bottom halves will be polled at least > * every 10ms */ > - *timeout = 10; > + *timeout = qemu_soonest_timeout(*timeout, 10); glib will not set *timeout to a meaningful value before calling aio_ctx_prepare(), right? If so, "*timeout = 10" should be used. > } else { > /* non-idle bottom halves will be executed > * immediately */ > @@ -166,6 +167,14 @@ aio_ctx_prepare(GSource *source, gint *timeout) > } > } > > + deadline = qemu_timeout_ns_to_ms(timerlistgroup_deadline_ns(&ctx->tlg)); > + if (deadline == 0) { > + *timeout = 0; > + return true; > + } else { > + *timeout = qemu_soonest_timeout(*timeout, deadline); > + } > + > return false; > } > > @@ -180,7 +189,7 @@ aio_ctx_check(GSource *source) > return true; > } > } > - return aio_pending(ctx); > + return aio_pending(ctx) || (timerlistgroup_deadline_ns(&ctx->tlg) == 0); > } > > static gboolean >
>> >> for (bh = ctx->first_bh; bh; bh = bh->next) { >> if (!bh->deleted && bh->scheduled) { >> if (bh->idle) { >> /* idle bottom halves will be polled at least >> * every 10ms */ >> - *timeout = 10; >> + *timeout = qemu_soonest_timeout(*timeout, 10); > glib will not set *timeout to a meaningful value before calling > aio_ctx_prepare(), right? If so, "*timeout = 10" should be used. I don't think that's correct. Each _prepare routine is called and has the abilitiy to alter the timeout but need not set it at all. Indeed it should not set it as there may already be a shorter timeout there. Here's the code before I touched it: aio_ctx_prepare(GSource *source, gint *timeout) { AioContext *ctx = (AioContext *) source; QEMUBH *bh; for (bh = ctx->first_bh; bh; bh = bh->next) { if (!bh->deleted && bh->scheduled) { if (bh->idle) { /* idle bottom halves will be polled at least * every 10ms */ *timeout = 10; } else { /* non-idle bottom halves will be executed * immediately */ *timeout = 0; return true; } } } return false; } You'll note that what this does is: a) if there are no bottom halves, leaves *timeout as is b) if there is a non-idle bottom half, set timeout to 0 c) else set timeout to 10 if there are only idle bottom halves. Arguably (c) is a bug if timeout was shorter than 10 on entry but the whole of idle bhs are a bit of a bodge. This is fixed by my series. If you are asking WHERE it gets set to -1, I don't claim to be a glib expert but I believe it's the line gint source_timeout = -1 around line 2816 in glib/gmain.c
于 2013-8-20 14:48, Alex Bligh 写道: >>> >>> for (bh = ctx->first_bh; bh; bh = bh->next) { >>> if (!bh->deleted && bh->scheduled) { >>> if (bh->idle) { >>> /* idle bottom halves will be polled at least >>> * every 10ms */ >>> - *timeout = 10; >>> + *timeout = qemu_soonest_timeout(*timeout, 10); >> glib will not set *timeout to a meaningful value before calling >> aio_ctx_prepare(), right? If so, "*timeout = 10" should be used. > > > I don't think that's correct. Each _prepare routine is called > and has the abilitiy to alter the timeout but need not > set it at all. Indeed it should not set it as there may already > be a shorter timeout there. > > Here's the code before I touched it: > > aio_ctx_prepare(GSource *source, gint *timeout) > { > AioContext *ctx = (AioContext *) source; > QEMUBH *bh; > > for (bh = ctx->first_bh; bh; bh = bh->next) { > if (!bh->deleted && bh->scheduled) { > if (bh->idle) { > /* idle bottom halves will be polled at least > * every 10ms */ > *timeout = 10; > } else { > /* non-idle bottom halves will be executed > * immediately */ > *timeout = 0; > return true; > } > } > } > > return false; > } > > You'll note that what this does is: > a) if there are no bottom halves, leaves *timeout as is > b) if there is a non-idle bottom half, set timeout to 0 > c) else set timeout to 10 if there are only idle bottom > halves. > > Arguably (c) is a bug if timeout was shorter than 10 > on entry but the whole of idle bhs are a bit of a bodge. > This is fixed by my series. > > If you are asking WHERE it gets set to -1, I don't claim > to be a glib expert but I believe it's the line > gint source_timeout = -1 > around line 2816 in glib/gmain.c > Thanks for the explanation. It seems *timeout is always set to -1 before calling GSource's prepare(), so "*timeout = qemu_soonest_timeout(*timeout, 10);" will always get *timeout = 10, so this call can be saved.
On 20 Aug 2013, at 08:19, Wenchao Xia wrote: > Thanks for the explanation. It seems *timeout is always set > to -1 before calling GSource's prepare(), so > "*timeout = qemu_soonest_timeout(*timeout, 10);" > will always get *timeout = 10, so this call can be saved. I believe that's incorrect too. glib *currently* calls the API that way, but there's nothing to say a future or past glub couldn't call each g_source with the same timeout pointer, with each g_source adjusting the timeout downwards. Or (for instance) call it with 0 for a non-blocking prepare. Therefore the implemented way is safer (only reducing the timeout).
于 2013-8-20 17:29, Alex Bligh 写道: > > On 20 Aug 2013, at 08:19, Wenchao Xia wrote: > >> Thanks for the explanation. It seems *timeout is always set >> to -1 before calling GSource's prepare(), so >> "*timeout = qemu_soonest_timeout(*timeout, 10);" >> will always get *timeout = 10, so this call can be saved. > > I believe that's incorrect too. glib *currently* calls > the API that way, but there's nothing to say a future > or past glub couldn't call each g_source with the same > timeout pointer, with each g_source adjusting the timeout > downwards. Or (for instance) call it with 0 for a So it is an undefined value, should avoid use it? For example, other gsource have minimum timeout of 5, and aio_ctx_prepare() is called with 0, then returned timeout is 0, but it is supposed to work with timeout 5 as the glib doc described: https://developer.gnome.org/glib/2.32/glib-The-Main-Event-Loop.html#GSourceFuncs That is my opinion, but I haven't read other project's code about prepare() usage, so can't be sure. Guess maintainer can give a conclusion. > non-blocking prepare. Therefore the implemented way > is safer (only reducing the timeout). >
On 20 Aug 2013, at 12:19, Wenchao Xia wrote:
> So it is an undefined value, should avoid use it?
It's not an undefined value. It's the value that the poll should
wait for subject to modification by the prepare call.
On Thu, Aug 15, 2013 at 09:34:21PM +0100, Alex Bligh wrote: > @@ -150,13 +150,14 @@ aio_ctx_prepare(GSource *source, gint *timeout) > { > AioContext *ctx = (AioContext *) source; > QEMUBH *bh; > + int deadline; > > for (bh = ctx->first_bh; bh; bh = bh->next) { > if (!bh->deleted && bh->scheduled) { > if (bh->idle) { > /* idle bottom halves will be polled at least > * every 10ms */ > - *timeout = 10; > + *timeout = qemu_soonest_timeout(*timeout, 10); > } else { > /* non-idle bottom halves will be executed > * immediately */ I agree with Wenchao: The docs explicitly say that .prepare() can set timeout to the maximum timeout value that is required. It then explains that the "actual timeout used" is the minimum of all timeout values - it's not our job to calculate the minimum, glib will do that after calling all .prepare() functions. I would drop this hunk. Stefan
Stefan, --On 21 August 2013 11:01:42 +0200 Stefan Hajnoczi <stefanha@gmail.com> wrote: > I agree with Wenchao: > > The docs explicitly say that .prepare() can set timeout to the maximum > timeout value that is required. It then explains that the "actual > timeout used" is the minimum of all timeout values - it's not our job to > calculate the minimum, glib will do that after calling all .prepare() > functions. > > I would drop this hunk. OK I will put that into v13. Anything else for v13 or are you ready to merge? I will rebase (again) on block-next obviously.
diff --git a/async.c b/async.c index 2b9ba9b..d8656cc 100644 --- a/async.c +++ b/async.c @@ -150,13 +150,14 @@ aio_ctx_prepare(GSource *source, gint *timeout) { AioContext *ctx = (AioContext *) source; QEMUBH *bh; + int deadline; for (bh = ctx->first_bh; bh; bh = bh->next) { if (!bh->deleted && bh->scheduled) { if (bh->idle) { /* idle bottom halves will be polled at least * every 10ms */ - *timeout = 10; + *timeout = qemu_soonest_timeout(*timeout, 10); } else { /* non-idle bottom halves will be executed * immediately */ @@ -166,6 +167,14 @@ aio_ctx_prepare(GSource *source, gint *timeout) } } + deadline = qemu_timeout_ns_to_ms(timerlistgroup_deadline_ns(&ctx->tlg)); + if (deadline == 0) { + *timeout = 0; + return true; + } else { + *timeout = qemu_soonest_timeout(*timeout, deadline); + } + return false; } @@ -180,7 +189,7 @@ aio_ctx_check(GSource *source) return true; } } - return aio_pending(ctx); + return aio_pending(ctx) || (timerlistgroup_deadline_ns(&ctx->tlg) == 0); } static gboolean
Calculate the timeout in aio_ctx_prepare taking into account the timers attached to the AioContext. Alter aio_ctx_check similarly. Signed-off-by: Alex Bligh <alex@alex.org.uk> --- async.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)