| Message ID | 20190525151241.5017-9-clg@kaod.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | None | expand |
Joel, Andrew, On 5/25/19 17:12, Cédric Le Goater wrote: > From: Joel Stanley <joel@jms.id.au> > > The Linux kernel driver was updated in commit 4451d3f59f2a > ("clocksource/drivers/fttmr010: Fix set_next_event handler) to fix an > issue observed on hardware: > > > RELOAD register is loaded into COUNT register when the aspeed timer > > is enabled, which means the next event may be delayed because timer > > interrupt won't be generated until <0xFFFFFFFF - current_count + > > cycles>. > > When running under Qemu, the system appeared "laggy". The guest is now > scheduling timer events too regularly, starving the host of CPU time. > > This patch modifies the timer model to attempt to schedule the timer > expiry as the guest requests, but if we have missed the deadline we > re interrupt and try again, which allows the guest to catch up. > > Provides expected behaviour with old and new guest code. > > Fixes: c04bd47db6b9 ("hw/timer: Add ASPEED timer device model") > Signed-off-by: Joel Stanley <joel@jms.id.au> > [clg: - merged a fix from Andrew Jeffery <andrew@aj.id.au> > "Fire interrupt on failure to meet deadline" > https://lists.ozlabs.org/pipermail/openbmc/2019-January/014641.html > - adapted commit log > - checkpatch fixes ] > Signed-off-by: Cédric Le Goater <clg@kaod.org> > --- > hw/timer/aspeed_timer.c | 59 ++++++++++++++++++++++------------------- > 1 file changed, 31 insertions(+), 28 deletions(-) > > diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c > index 5c786e512815..9ffd8e09f670 100644 > --- a/hw/timer/aspeed_timer.c > +++ b/hw/timer/aspeed_timer.c > @@ -109,37 +109,40 @@ static inline uint64_t calculate_time(struct AspeedTimer *t, uint32_t ticks) > > static uint64_t calculate_next(struct AspeedTimer *t) > { > - uint64_t next = 0; > - uint32_t rate = calculate_rate(t); > + uint64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); > + uint64_t next; > > - while (!next) { > - /* We don't know the relationship between the values in the match > - * registers, so sort using MAX/MIN/zero. We sort in that order as the > - * timer counts down to zero. */ > - uint64_t seq[] = { > - calculate_time(t, MAX(t->match[0], t->match[1])), > - calculate_time(t, MIN(t->match[0], t->match[1])), > - calculate_time(t, 0), > - }; > - uint64_t reload_ns; > - uint64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); > - > - if (now < seq[0]) { > - next = seq[0]; > - } else if (now < seq[1]) { > - next = seq[1]; > - } else if (now < seq[2]) { > - next = seq[2]; > - } else if (t->reload) { > - reload_ns = muldiv64(t->reload, NANOSECONDS_PER_SECOND, rate); > - t->start = now - ((now - t->start) % reload_ns); > - } else { > - /* no reload value, return 0 */ > - break; > - } > + /* > + * We don't know the relationship between the values in the match > + * registers, so sort using MAX/MIN/zero. We sort in that order as > + * the timer counts down to zero. > + */ > + > + next = calculate_time(t, MAX(t->match[0], t->match[1])); > + if (now < next) { > + return next; > + } > + > + next = calculate_time(t, MIN(t->match[0], t->match[1])); > + if (now < next) { > + return next; > + } > + > + next = calculate_time(t, 0); > + if (now < next) { > + return next; > + } > + > + /* We've missed all deadlines, fire interrupt and try again */ > + timer_del(&t->timer); > + > + if (timer_overflow_interrupt(t)) { > + t->level = !t->level; > + qemu_set_irq(t->irq, t->level); > } > > - return next; > + t->start = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); > + return calculate_time(t, MAX(MAX(t->match[0], t->match[1]), 0)); This MAX(MAX(x, y), 0) looks strange to me. Would you remember where it comes from ? Thanks, C. > } > > static void aspeed_timer_mod(AspeedTimer *t)
On Fri, 22 Sep 2023, at 22:51, Cédric Le Goater wrote: > Joel, Andrew, > > On 5/25/19 17:12, Cédric Le Goater wrote: >> From: Joel Stanley <joel@jms.id.au> >> >> The Linux kernel driver was updated in commit 4451d3f59f2a >> ("clocksource/drivers/fttmr010: Fix set_next_event handler) to fix an >> issue observed on hardware: >> >> > RELOAD register is loaded into COUNT register when the aspeed timer >> > is enabled, which means the next event may be delayed because timer >> > interrupt won't be generated until <0xFFFFFFFF - current_count + >> > cycles>. >> >> When running under Qemu, the system appeared "laggy". The guest is now >> scheduling timer events too regularly, starving the host of CPU time. >> >> This patch modifies the timer model to attempt to schedule the timer >> expiry as the guest requests, but if we have missed the deadline we >> re interrupt and try again, which allows the guest to catch up. >> >> Provides expected behaviour with old and new guest code. >> >> Fixes: c04bd47db6b9 ("hw/timer: Add ASPEED timer device model") >> Signed-off-by: Joel Stanley <joel@jms.id.au> >> [clg: - merged a fix from Andrew Jeffery <andrew@aj.id.au> >> "Fire interrupt on failure to meet deadline" >> https://lists.ozlabs.org/pipermail/openbmc/2019-January/014641.html >> - adapted commit log >> - checkpatch fixes ] >> Signed-off-by: Cédric Le Goater <clg@kaod.org> >> --- >> hw/timer/aspeed_timer.c | 59 ++++++++++++++++++++++------------------- >> 1 file changed, 31 insertions(+), 28 deletions(-) >> >> diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c >> index 5c786e512815..9ffd8e09f670 100644 >> --- a/hw/timer/aspeed_timer.c >> +++ b/hw/timer/aspeed_timer.c >> @@ -109,37 +109,40 @@ static inline uint64_t calculate_time(struct AspeedTimer *t, uint32_t ticks) >> >> static uint64_t calculate_next(struct AspeedTimer *t) >> { >> - uint64_t next = 0; >> - uint32_t rate = calculate_rate(t); >> + uint64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); >> + uint64_t next; >> >> - while (!next) { >> - /* We don't know the relationship between the values in the match >> - * registers, so sort using MAX/MIN/zero. We sort in that order as the >> - * timer counts down to zero. */ >> - uint64_t seq[] = { >> - calculate_time(t, MAX(t->match[0], t->match[1])), >> - calculate_time(t, MIN(t->match[0], t->match[1])), >> - calculate_time(t, 0), >> - }; >> - uint64_t reload_ns; >> - uint64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); >> - >> - if (now < seq[0]) { >> - next = seq[0]; >> - } else if (now < seq[1]) { >> - next = seq[1]; >> - } else if (now < seq[2]) { >> - next = seq[2]; >> - } else if (t->reload) { >> - reload_ns = muldiv64(t->reload, NANOSECONDS_PER_SECOND, rate); >> - t->start = now - ((now - t->start) % reload_ns); >> - } else { >> - /* no reload value, return 0 */ >> - break; >> - } >> + /* >> + * We don't know the relationship between the values in the match >> + * registers, so sort using MAX/MIN/zero. We sort in that order as >> + * the timer counts down to zero. >> + */ >> + >> + next = calculate_time(t, MAX(t->match[0], t->match[1])); >> + if (now < next) { >> + return next; >> + } >> + >> + next = calculate_time(t, MIN(t->match[0], t->match[1])); >> + if (now < next) { >> + return next; >> + } >> + >> + next = calculate_time(t, 0); >> + if (now < next) { >> + return next; >> + } >> + >> + /* We've missed all deadlines, fire interrupt and try again */ >> + timer_del(&t->timer); >> + >> + if (timer_overflow_interrupt(t)) { >> + t->level = !t->level; >> + qemu_set_irq(t->irq, t->level); >> } >> >> - return next; >> + t->start = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); >> + return calculate_time(t, MAX(MAX(t->match[0], t->match[1]), 0)); > > This MAX(MAX(x, y), 0) looks strange to me. Would you remember where it comes > from ? Thanks, The inner MAX() deals with the lack of ordering constraints between the match values. I think the outer MAX() is redundant. We should probably remove it. The match member is type uint32_t so it can't be negative. You did steal that from an RFC patch :D Andrew
On 9/25/23 09:54, Andrew Jeffery wrote: > > > On Fri, 22 Sep 2023, at 22:51, Cédric Le Goater wrote: >> Joel, Andrew, >> >> On 5/25/19 17:12, Cédric Le Goater wrote: >>> From: Joel Stanley <joel@jms.id.au> >>> >>> The Linux kernel driver was updated in commit 4451d3f59f2a >>> ("clocksource/drivers/fttmr010: Fix set_next_event handler) to fix an >>> issue observed on hardware: >>> >>> > RELOAD register is loaded into COUNT register when the aspeed timer >>> > is enabled, which means the next event may be delayed because timer >>> > interrupt won't be generated until <0xFFFFFFFF - current_count + >>> > cycles>. >>> >>> When running under Qemu, the system appeared "laggy". The guest is now >>> scheduling timer events too regularly, starving the host of CPU time. >>> >>> This patch modifies the timer model to attempt to schedule the timer >>> expiry as the guest requests, but if we have missed the deadline we >>> re interrupt and try again, which allows the guest to catch up. >>> >>> Provides expected behaviour with old and new guest code. >>> >>> Fixes: c04bd47db6b9 ("hw/timer: Add ASPEED timer device model") >>> Signed-off-by: Joel Stanley <joel@jms.id.au> >>> [clg: - merged a fix from Andrew Jeffery <andrew@aj.id.au> >>> "Fire interrupt on failure to meet deadline" >>> https://lists.ozlabs.org/pipermail/openbmc/2019-January/014641.html >>> - adapted commit log >>> - checkpatch fixes ] >>> Signed-off-by: Cédric Le Goater <clg@kaod.org> >>> --- >>> hw/timer/aspeed_timer.c | 59 ++++++++++++++++++++++------------------- >>> 1 file changed, 31 insertions(+), 28 deletions(-) >>> >>> diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c >>> index 5c786e512815..9ffd8e09f670 100644 >>> --- a/hw/timer/aspeed_timer.c >>> +++ b/hw/timer/aspeed_timer.c >>> @@ -109,37 +109,40 @@ static inline uint64_t calculate_time(struct AspeedTimer *t, uint32_t ticks) >>> >>> static uint64_t calculate_next(struct AspeedTimer *t) >>> { >>> - uint64_t next = 0; >>> - uint32_t rate = calculate_rate(t); >>> + uint64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); >>> + uint64_t next; >>> >>> - while (!next) { >>> - /* We don't know the relationship between the values in the match >>> - * registers, so sort using MAX/MIN/zero. We sort in that order as the >>> - * timer counts down to zero. */ >>> - uint64_t seq[] = { >>> - calculate_time(t, MAX(t->match[0], t->match[1])), >>> - calculate_time(t, MIN(t->match[0], t->match[1])), >>> - calculate_time(t, 0), >>> - }; >>> - uint64_t reload_ns; >>> - uint64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); >>> - >>> - if (now < seq[0]) { >>> - next = seq[0]; >>> - } else if (now < seq[1]) { >>> - next = seq[1]; >>> - } else if (now < seq[2]) { >>> - next = seq[2]; >>> - } else if (t->reload) { >>> - reload_ns = muldiv64(t->reload, NANOSECONDS_PER_SECOND, rate); >>> - t->start = now - ((now - t->start) % reload_ns); >>> - } else { >>> - /* no reload value, return 0 */ >>> - break; >>> - } >>> + /* >>> + * We don't know the relationship between the values in the match >>> + * registers, so sort using MAX/MIN/zero. We sort in that order as >>> + * the timer counts down to zero. >>> + */ >>> + >>> + next = calculate_time(t, MAX(t->match[0], t->match[1])); >>> + if (now < next) { >>> + return next; >>> + } >>> + >>> + next = calculate_time(t, MIN(t->match[0], t->match[1])); >>> + if (now < next) { >>> + return next; >>> + } >>> + >>> + next = calculate_time(t, 0); >>> + if (now < next) { >>> + return next; >>> + } >>> + >>> + /* We've missed all deadlines, fire interrupt and try again */ >>> + timer_del(&t->timer); >>> + >>> + if (timer_overflow_interrupt(t)) { >>> + t->level = !t->level; >>> + qemu_set_irq(t->irq, t->level); >>> } >>> >>> - return next; >>> + t->start = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); >>> + return calculate_time(t, MAX(MAX(t->match[0], t->match[1]), 0)); >> >> This MAX(MAX(x, y), 0) looks strange to me. Would you remember where it comes >> from ? Thanks, > > The inner MAX() deals with the lack of ordering constraints between the match values. I think the outer MAX() is redundant. We should probably remove it. The match member is type uint32_t so it can't be negative. You did steal that from an RFC patch :D I did ! Fixed there : https://patchwork.ozlabs.org/project/qemu-devel/patch/20230922155924.1172019-5-clg@kaod.org/ Cheers, C.
On Mon, 25 Sep 2023, at 18:50, Cédric Le Goater wrote: > On 9/25/23 09:54, Andrew Jeffery wrote: >> >> >> On Fri, 22 Sep 2023, at 22:51, Cédric Le Goater wrote: >>> Joel, Andrew, >>> >>> On 5/25/19 17:12, Cédric Le Goater wrote: >>>> From: Joel Stanley <joel@jms.id.au> >>>> >>>> The Linux kernel driver was updated in commit 4451d3f59f2a >>>> ("clocksource/drivers/fttmr010: Fix set_next_event handler) to fix an >>>> issue observed on hardware: >>>> >>>> > RELOAD register is loaded into COUNT register when the aspeed timer >>>> > is enabled, which means the next event may be delayed because timer >>>> > interrupt won't be generated until <0xFFFFFFFF - current_count + >>>> > cycles>. >>>> >>>> When running under Qemu, the system appeared "laggy". The guest is now >>>> scheduling timer events too regularly, starving the host of CPU time. >>>> >>>> This patch modifies the timer model to attempt to schedule the timer >>>> expiry as the guest requests, but if we have missed the deadline we >>>> re interrupt and try again, which allows the guest to catch up. >>>> >>>> Provides expected behaviour with old and new guest code. >>>> >>>> Fixes: c04bd47db6b9 ("hw/timer: Add ASPEED timer device model") >>>> Signed-off-by: Joel Stanley <joel@jms.id.au> >>>> [clg: - merged a fix from Andrew Jeffery <andrew@aj.id.au> >>>> "Fire interrupt on failure to meet deadline" >>>> https://lists.ozlabs.org/pipermail/openbmc/2019-January/014641.html >>>> - adapted commit log >>>> - checkpatch fixes ] >>>> Signed-off-by: Cédric Le Goater <clg@kaod.org> >>>> --- >>>> hw/timer/aspeed_timer.c | 59 ++++++++++++++++++++++------------------- >>>> 1 file changed, 31 insertions(+), 28 deletions(-) >>>> >>>> diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c >>>> index 5c786e512815..9ffd8e09f670 100644 >>>> --- a/hw/timer/aspeed_timer.c >>>> +++ b/hw/timer/aspeed_timer.c >>>> @@ -109,37 +109,40 @@ static inline uint64_t calculate_time(struct AspeedTimer *t, uint32_t ticks) >>>> >>>> static uint64_t calculate_next(struct AspeedTimer *t) >>>> { >>>> - uint64_t next = 0; >>>> - uint32_t rate = calculate_rate(t); >>>> + uint64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); >>>> + uint64_t next; >>>> >>>> - while (!next) { >>>> - /* We don't know the relationship between the values in the match >>>> - * registers, so sort using MAX/MIN/zero. We sort in that order as the >>>> - * timer counts down to zero. */ >>>> - uint64_t seq[] = { >>>> - calculate_time(t, MAX(t->match[0], t->match[1])), >>>> - calculate_time(t, MIN(t->match[0], t->match[1])), >>>> - calculate_time(t, 0), >>>> - }; >>>> - uint64_t reload_ns; >>>> - uint64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); >>>> - >>>> - if (now < seq[0]) { >>>> - next = seq[0]; >>>> - } else if (now < seq[1]) { >>>> - next = seq[1]; >>>> - } else if (now < seq[2]) { >>>> - next = seq[2]; >>>> - } else if (t->reload) { >>>> - reload_ns = muldiv64(t->reload, NANOSECONDS_PER_SECOND, rate); >>>> - t->start = now - ((now - t->start) % reload_ns); >>>> - } else { >>>> - /* no reload value, return 0 */ >>>> - break; >>>> - } >>>> + /* >>>> + * We don't know the relationship between the values in the match >>>> + * registers, so sort using MAX/MIN/zero. We sort in that order as >>>> + * the timer counts down to zero. >>>> + */ >>>> + >>>> + next = calculate_time(t, MAX(t->match[0], t->match[1])); >>>> + if (now < next) { >>>> + return next; >>>> + } >>>> + >>>> + next = calculate_time(t, MIN(t->match[0], t->match[1])); >>>> + if (now < next) { >>>> + return next; >>>> + } >>>> + >>>> + next = calculate_time(t, 0); >>>> + if (now < next) { >>>> + return next; >>>> + } >>>> + >>>> + /* We've missed all deadlines, fire interrupt and try again */ >>>> + timer_del(&t->timer); >>>> + >>>> + if (timer_overflow_interrupt(t)) { >>>> + t->level = !t->level; >>>> + qemu_set_irq(t->irq, t->level); >>>> } >>>> >>>> - return next; >>>> + t->start = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); >>>> + return calculate_time(t, MAX(MAX(t->match[0], t->match[1]), 0)); >>> >>> This MAX(MAX(x, y), 0) looks strange to me. Would you remember where it comes >>> from ? Thanks, >> >> The inner MAX() deals with the lack of ordering constraints between the match values. I think the outer MAX() is redundant. We should probably remove it. The match member is type uint32_t so it can't be negative. You did steal that from an RFC patch :D > > I did ! Fixed there : > > > https://patchwork.ozlabs.org/project/qemu-devel/patch/20230922155924.1172019-5-clg@kaod.org/ > Thanks. That one might be further down in my review queue 😅 Andrew
On Fri, 22 Sept 2023 at 13:21, Cédric Le Goater <clg@kaod.org> wrote: > > + t->start = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); > > + return calculate_time(t, MAX(MAX(t->match[0], t->match[1]), 0)); > > This MAX(MAX(x, y), 0) looks strange to me. Would you remember where it comes > from ? Thanks, That looks very strange. I think you've sorted it, so I wanted to bring up the MAX macros themselves. It's unfortunate that they create a non-unique local variable. Are we allowed to borrow the kernels macros? They have some infrastructure for creating a unique local variable name, as well as handling the const and non-const variants with the one macro. https://elixir.bootlin.com/linux/v6.5/source/include/linux/minmax.h Cheers, Joel
On 9/27/23 04:12, Joel Stanley wrote: > On Fri, 22 Sept 2023 at 13:21, Cédric Le Goater <clg@kaod.org> wrote: > >>> + t->start = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); >>> + return calculate_time(t, MAX(MAX(t->match[0], t->match[1]), 0)); >> >> This MAX(MAX(x, y), 0) looks strange to me. Would you remember where it comes >> from ? Thanks, > > That looks very strange. I think you've sorted it, so I wanted to > bring up the MAX macros themselves. It's unfortunate that they create > a non-unique local variable. Are we allowed to borrow the kernels > macros? They have some infrastructure for creating a unique local > variable name, as well as handling the const and non-const variants > with the one macro. > > https://elixir.bootlin.com/linux/v6.5/source/include/linux/minmax.h I believe that's what Markus does in its own way : https://lore.kernel.org/qemu-devel/20230921121312.1301864-8-armbru@redhat.com/ Thanks, C.
Cédric Le Goater <clg@kaod.org> writes: > On 9/27/23 04:12, Joel Stanley wrote: >> On Fri, 22 Sept 2023 at 13:21, Cédric Le Goater <clg@kaod.org> wrote: >> >>>> + t->start = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); >>>> + return calculate_time(t, MAX(MAX(t->match[0], t->match[1]), 0)); >>> >>> This MAX(MAX(x, y), 0) looks strange to me. Would you remember where it comes >>> from ? Thanks, >> That looks very strange. I think you've sorted it, so I wanted to >> bring up the MAX macros themselves. It's unfortunate that they create >> a non-unique local variable. Are we allowed to borrow the kernels >> macros? They have some infrastructure for creating a unique local >> variable name, as well as handling the const and non-const variants >> with the one macro. >> https://elixir.bootlin.com/linux/v6.5/source/include/linux/minmax.h > > I believe that's what Markus does in its own way : > > https://lore.kernel.org/qemu-devel/20230921121312.1301864-8-armbru@redhat.com/ I wasn't aware of the kernel's infrastructure. We can steal it if people think it provides additional value. Make your case :)
diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c index 5c786e512815..9ffd8e09f670 100644 --- a/hw/timer/aspeed_timer.c +++ b/hw/timer/aspeed_timer.c @@ -109,37 +109,40 @@ static inline uint64_t calculate_time(struct AspeedTimer *t, uint32_t ticks) static uint64_t calculate_next(struct AspeedTimer *t) { - uint64_t next = 0; - uint32_t rate = calculate_rate(t); + uint64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); + uint64_t next; - while (!next) { - /* We don't know the relationship between the values in the match - * registers, so sort using MAX/MIN/zero. We sort in that order as the - * timer counts down to zero. */ - uint64_t seq[] = { - calculate_time(t, MAX(t->match[0], t->match[1])), - calculate_time(t, MIN(t->match[0], t->match[1])), - calculate_time(t, 0), - }; - uint64_t reload_ns; - uint64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); - - if (now < seq[0]) { - next = seq[0]; - } else if (now < seq[1]) { - next = seq[1]; - } else if (now < seq[2]) { - next = seq[2]; - } else if (t->reload) { - reload_ns = muldiv64(t->reload, NANOSECONDS_PER_SECOND, rate); - t->start = now - ((now - t->start) % reload_ns); - } else { - /* no reload value, return 0 */ - break; - } + /* + * We don't know the relationship between the values in the match + * registers, so sort using MAX/MIN/zero. We sort in that order as + * the timer counts down to zero. + */ + + next = calculate_time(t, MAX(t->match[0], t->match[1])); + if (now < next) { + return next; + } + + next = calculate_time(t, MIN(t->match[0], t->match[1])); + if (now < next) { + return next; + } + + next = calculate_time(t, 0); + if (now < next) { + return next; + } + + /* We've missed all deadlines, fire interrupt and try again */ + timer_del(&t->timer); + + if (timer_overflow_interrupt(t)) { + t->level = !t->level; + qemu_set_irq(t->irq, t->level); } - return next; + t->start = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); + return calculate_time(t, MAX(MAX(t->match[0], t->match[1]), 0)); } static void aspeed_timer_mod(AspeedTimer *t)