| Message ID | 0eb543c0917bac416a18fc9daf97f5ba4e61ce1f.1454169735.git.digetx@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, Jan 30, 2016 at 8:43 AM, Dmitry Osipenko <digetx@gmail.com> 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. But is this a non-issue when we consider that event latency (usually interrupt latency) is undefined anyway? Regards, Peter > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > --- > 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 >
09.03.2016 00:08, Peter Crosthwaite ?????: > On Sat, Jan 30, 2016 at 8:43 AM, Dmitry Osipenko <digetx@gmail.com> 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 <digetx@gmail.com> >> --- >> 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 >>
09.03.2016 00:08, Peter Crosthwaite ?????: > On Sat, Jan 30, 2016 at 8:43 AM, Dmitry Osipenko <digetx@gmail.com> 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 Ah, you probably meant "Legalize running with delta = load = 0 and abort on period = 0" patch. Yes, they could be squashed. I chose wrong policy at first time, further added MPtimer tests revealed it.
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)
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. Signed-off-by: Dmitry Osipenko <digetx@gmail.com> --- hw/core/ptimer.c | 34 +++++++++++++++------------------- 1 file changed, 15 insertions(+), 19 deletions(-)