From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59147) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1adkDp-0001cg-0y for qemu-devel@nongnu.org; Wed, 09 Mar 2016 14:56:42 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1adkDn-00010w-VD for qemu-devel@nongnu.org; Wed, 09 Mar 2016 14:56:40 -0500 References: <0eb543c0917bac416a18fc9daf97f5ba4e61ce1f.1454169735.git.digetx@gmail.com> From: Dmitry Osipenko Message-ID: <56E07FD4.70209@gmail.com> Date: Wed, 9 Mar 2016 22:56:04 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH v12 8/9] hw/ptimer: Perform delayed tick instead of immediate if delta = 0 List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Crosthwaite Cc: Peter Maydell , qemu-arm , QEMU Developers 09.03.2016 00:08, Peter Crosthwaite пишет: > On Sat, Jan 30, 2016 at 8:43 AM, Dmitry Osipenko wrote: >> It might be necessary by some emulated HW to perform the tick after one >> period if delta = 0. Given that it is much less churny to implement immediate >> tick by the ptimer user itself, let's make ptimer do the delayed tick. >> > > Isn't this related to previous patch? It is kind of a rounding problem > that will vary from timer to timer. Some timers may interpret the > "tick" as the wrap-around back to the load value (even if that is 0) > while others interpret the tick as the transition to zero (makes more > sense for a one-shot). It's hard to set a universal policy here. If by previous patch you are meaning "Fix counter - 1 ...", then no, it's a distinct issue/feature. I endeavored to add some sort of "flags" to ptimer in order to support various policies, but quickly realized that it doesn't really worth an effort since there are no users of those policies (currently "delta = 0" is a forbidden case) and ptimer VMSD version would be bumped. It's not possible (doesn't make sense) for ptimer user to implement policy provided by this patch by itself. Other policies could be easily supported in future if desired (or implemented by ptimer user). > > But is this a non-issue when we consider that event latency (usually > interrupt latency) is undefined anyway? > It's not an issue for a timer that has quite short period. Some of the arm_mptimer tests would fail (in icount mode) if that policy not implemented. BTW, I think "deferred" might be better than "delayed", however not sure :) > Regards, > Peter > >> Signed-off-by: Dmitry Osipenko >> --- >> hw/core/ptimer.c | 34 +++++++++++++++------------------- >> 1 file changed, 15 insertions(+), 19 deletions(-) >> >> diff --git a/hw/core/ptimer.c b/hw/core/ptimer.c >> index b2044fb..bcd090c 100644 >> --- a/hw/core/ptimer.c >> +++ b/hw/core/ptimer.c >> @@ -36,19 +36,7 @@ static void ptimer_reload(ptimer_state *s) >> { >> uint32_t period_frac = s->period_frac; >> uint64_t period = s->period; >> - >> - if (s->delta == 0) { >> - ptimer_trigger(s); >> - } >> - >> - if (s->delta == 0 && s->enabled == 1) { >> - s->delta = s->limit; >> - } >> - >> - if (s->delta == 0) { >> - ptimer_stop(s); >> - return; >> - } >> + uint64_t delta = MAX(1, s->delta); >> >> /* >> * Artificially limit timeout rate to something >> @@ -59,15 +47,15 @@ static void ptimer_reload(ptimer_state *s) >> * on the current generation of host machines. >> */ >> >> - if (s->enabled == 1 && (s->delta * period < 10000) && !use_icount) { >> - period = 10000 / s->delta; >> + if (s->enabled == 1 && (delta * period < 10000) && !use_icount) { >> + period = 10000 / delta; >> period_frac = 0; >> } >> >> s->last_event = s->next_event; >> - s->next_event = s->last_event + s->delta * period; >> + s->next_event = s->last_event + delta * period; >> if (period_frac) { >> - s->next_event += ((int64_t)period_frac * s->delta) >> 32; >> + s->next_event += ((int64_t)period_frac * delta) >> 32; >> } >> timer_mod(s->timer, s->next_event); >> } >> @@ -75,8 +63,16 @@ static void ptimer_reload(ptimer_state *s) >> static void ptimer_tick(void *opaque) >> { >> ptimer_state *s = (ptimer_state *)opaque; >> - s->delta = 0; >> - ptimer_reload(s); >> + >> + s->delta = (s->enabled == 1) ? s->limit : 0; >> + >> + if (s->delta == 0) { >> + s->enabled = 0; >> + } else { >> + ptimer_reload(s); >> + } >> + >> + ptimer_trigger(s); >> } >> >> uint64_t ptimer_get_count(ptimer_state *s) >> -- >> 2.7.0 >> -- Dmitry