| Message ID | 20190314084235.9887-6-clg@kaod.org |
|---|---|
| State | New |
| Headers | show |
| Series | aspeed/timer: Fix slowdowns in recent Linux | expand |
Christian, Could you please provide a fix for this patch ? patchew complains, see attached log. Thanks, C. On 3/14/19 9:42 AM, Cédric Le Goater wrote: > From: Christian Svensson <bluecmd@google.com> > > If the host decrements the counter register that results in a negative > delta. This is then passed to muldiv64 which only handles unsigned > numbers resulting in bogus results. > > This fix ensures the data being operated on is signed before it is > ultimately casted to the final unsigned value. > > Test case: kexec a kernel using aspeed_timer and it will freeze on the > second bootup when the kernel initializes the timer. With this patch > that no longer happens and the timer appears to run OK. > > Signed-off-by: Christian Svensson <bluecmd@google.com> > [clg: - checkpatch fixes ] > Signed-off-by: Cédric Le Goater <clg@kaod.org> > --- > hw/timer/aspeed_timer.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c > index 9988b8fbbf17..0b16eac8970c 100644 > --- a/hw/timer/aspeed_timer.c > +++ b/hw/timer/aspeed_timer.c > @@ -275,7 +275,8 @@ static void aspeed_timer_set_value(AspeedTimerCtrlState *s, int timer, int reg, > int64_t delta = (int64_t) value - (int64_t) calculate_ticks(t, now); > uint32_t rate = calculate_rate(t); > > - t->start += muldiv64(delta, NANOSECONDS_PER_SECOND, rate); > + t->start = (int64_t)t->start + > + ((__int128_t)delta * NANOSECONDS_PER_SECOND / rate); > aspeed_timer_mod(t); > } > break; > Patchew URL: https://patchew.org/QEMU/20190314084235.9887-1-clg@kaod.org/ Hi, This series failed the docker-mingw@fedora build test. Please find the testing commands and their output below. If you have Docker installed, you can probably reproduce it locally. === TEST SCRIPT BEGIN === #!/bin/bash time make docker-test-mingw@fedora SHOW_ENV=1 J=14 NETWORK=1 === TEST SCRIPT END === CC migration/xbzrle.o CC migration/postcopy-ram.o /tmp/qemu-test/src/hw/timer/aspeed_timer.c: In function 'aspeed_timer_set_value': /tmp/qemu-test/src/hw/timer/aspeed_timer.c:279:19: error: '__int128_t' undeclared (first use in this function); did you mean '__int128'? ((__int128_t)delta * NANOSECONDS_PER_SECOND / rate); ^~~~~~~~~~ __int128 /tmp/qemu-test/src/hw/timer/aspeed_timer.c:279:19: note: each undeclared identifier is reported only once for each function it appears in /tmp/qemu-test/src/hw/timer/aspeed_timer.c:279:30: error: expected ')' before 'delta' ((__int128_t)delta * NANOSECONDS_PER_SECOND / rate); ~ ^~~~~ ) /tmp/qemu-test/src/hw/timer/aspeed_timer.c:276:22: error: unused variable 'rate' [-Werror=unused-variable] uint32_t rate = calculate_rate(t); ^~~~ /tmp/qemu-test/src/hw/timer/aspeed_timer.c:275:21: error: unused variable 'delta' [-Werror=unused-variable] int64_t delta = (int64_t) value - (int64_t) calculate_ticks(t, now); ^~~~~ cc1: all warnings being treated as errors The full log is available at http://patchew.org/logs/20190314084235.9887-1-clg@kaod.org/testing.docker-mingw@fedora/?type=message. --- Email generated automatically by Patchew [http://patchew.org/]. Please send your feedback to patchew-devel@redhat.com
Hi all, I have a new patch but I'm not sure how you want me to post it. Should I do a "PATCH v2" with a single patch and this thread as the thread ID? Thanks, - Chris On Thu, Mar 14, 2019 at 10:05 AM Cédric Le Goater <clg@kaod.org> wrote: > Christian, > > Could you please provide a fix for this patch ? patchew complains, see > attached log. > > Thanks, > > C. > > On 3/14/19 9:42 AM, Cédric Le Goater wrote: > > From: Christian Svensson <bluecmd@google.com> > > > > If the host decrements the counter register that results in a negative > > delta. This is then passed to muldiv64 which only handles unsigned > > numbers resulting in bogus results. > > > > This fix ensures the data being operated on is signed before it is > > ultimately casted to the final unsigned value. > > > > Test case: kexec a kernel using aspeed_timer and it will freeze on the > > second bootup when the kernel initializes the timer. With this patch > > that no longer happens and the timer appears to run OK. > > > > Signed-off-by: Christian Svensson <bluecmd@google.com> > > [clg: - checkpatch fixes ] > > Signed-off-by: Cédric Le Goater <clg@kaod.org> > > --- > > hw/timer/aspeed_timer.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c > > index 9988b8fbbf17..0b16eac8970c 100644 > > --- a/hw/timer/aspeed_timer.c > > +++ b/hw/timer/aspeed_timer.c > > @@ -275,7 +275,8 @@ static void > aspeed_timer_set_value(AspeedTimerCtrlState *s, int timer, int reg, > > int64_t delta = (int64_t) value - (int64_t) > calculate_ticks(t, now); > > uint32_t rate = calculate_rate(t); > > > > - t->start += muldiv64(delta, NANOSECONDS_PER_SECOND, rate); > > + t->start = (int64_t)t->start + > > + ((__int128_t)delta * NANOSECONDS_PER_SECOND / rate); > > aspeed_timer_mod(t); > > } > > break; > > > >
On 3/14/19 11:47 AM, Christian Svensson wrote: > Hi all, > > I have a new patch but I'm not sure how you want me to post it. > Should I do a "PATCH v2" with a single patch and this thread as the thread ID? I would wait for feedback from Peter first. Thanks, C. > Thanks, > - Chris > > > On Thu, Mar 14, 2019 at 10:05 AM Cédric Le Goater <clg@kaod.org <mailto:clg@kaod.org>> wrote: > > Christian, > > Could you please provide a fix for this patch ? patchew complains, see > attached log. > > Thanks, > > C. > > On 3/14/19 9:42 AM, Cédric Le Goater wrote: > > From: Christian Svensson <bluecmd@google.com <mailto:bluecmd@google.com>> > > > > If the host decrements the counter register that results in a negative > > delta. This is then passed to muldiv64 which only handles unsigned > > numbers resulting in bogus results. > > > > This fix ensures the data being operated on is signed before it is > > ultimately casted to the final unsigned value. > > > > Test case: kexec a kernel using aspeed_timer and it will freeze on the > > second bootup when the kernel initializes the timer. With this patch > > that no longer happens and the timer appears to run OK. > > > > Signed-off-by: Christian Svensson <bluecmd@google.com <mailto:bluecmd@google.com>> > > [clg: - checkpatch fixes ] > > Signed-off-by: Cédric Le Goater <clg@kaod.org <mailto:clg@kaod.org>> > > --- > > hw/timer/aspeed_timer.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c > > index 9988b8fbbf17..0b16eac8970c 100644 > > --- a/hw/timer/aspeed_timer.c > > +++ b/hw/timer/aspeed_timer.c > > @@ -275,7 +275,8 @@ static void aspeed_timer_set_value(AspeedTimerCtrlState *s, int timer, int reg, > > int64_t delta = (int64_t) value - (int64_t) calculate_ticks(t, now); > > uint32_t rate = calculate_rate(t); > > > > - t->start += muldiv64(delta, NANOSECONDS_PER_SECOND, rate); > > + t->start = (int64_t)t->start + > > + ((__int128_t)delta * NANOSECONDS_PER_SECOND / rate); > > aspeed_timer_mod(t); > > } > > break; > > >
On Thu, 14 Mar 2019 at 08:43, Cédric Le Goater <clg@kaod.org> wrote: > > From: Christian Svensson <bluecmd@google.com> > > If the host decrements the counter register that results in a negative > delta. This is then passed to muldiv64 which only handles unsigned > numbers resulting in bogus results. > > This fix ensures the data being operated on is signed before it is > ultimately casted to the final unsigned value. > > Test case: kexec a kernel using aspeed_timer and it will freeze on the > second bootup when the kernel initializes the timer. With this patch > that no longer happens and the timer appears to run OK. > > Signed-off-by: Christian Svensson <bluecmd@google.com> > [clg: - checkpatch fixes ] > Signed-off-by: Cédric Le Goater <clg@kaod.org> > --- > hw/timer/aspeed_timer.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c > index 9988b8fbbf17..0b16eac8970c 100644 > --- a/hw/timer/aspeed_timer.c > +++ b/hw/timer/aspeed_timer.c > @@ -275,7 +275,8 @@ static void aspeed_timer_set_value(AspeedTimerCtrlState *s, int timer, int reg, > int64_t delta = (int64_t) value - (int64_t) calculate_ticks(t, now); > uint32_t rate = calculate_rate(t); > > - t->start += muldiv64(delta, NANOSECONDS_PER_SECOND, rate); > + t->start = (int64_t)t->start + > + ((__int128_t)delta * NANOSECONDS_PER_SECOND / rate); > aspeed_timer_mod(t); You can't guarantee that __int128_t exists. If you must use it then you need an alternate code path for the ifndef CONFIG_INT128 case (which is going to be complicated enough that this should really be abstracted away into a signed version of muldiv64().) But overall I'm a little sceptical that the aspeed timer is really a special case that needs a signed version of this when no other timer in the system does... thanks -- PMM
Thanks for the feedback, On Thu, Mar 14, 2019, 11:57 Peter Maydell <peter.maydell@linaro.org> wrote: > But overall I'm a little sceptical that the aspeed timer is > really a special case that needs a signed version of this > when no other timer in the system does... I agree, and the v2 of the patch doesn't require it. Happy to submit it when you folks think it's a good time to merge it. If you're curious you can get a sneak peek of the patch in https://github.com/u-root/u-bmc/issues/134. Regards,
diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c index 9988b8fbbf17..0b16eac8970c 100644 --- a/hw/timer/aspeed_timer.c +++ b/hw/timer/aspeed_timer.c @@ -275,7 +275,8 @@ static void aspeed_timer_set_value(AspeedTimerCtrlState *s, int timer, int reg, int64_t delta = (int64_t) value - (int64_t) calculate_ticks(t, now); uint32_t rate = calculate_rate(t); - t->start += muldiv64(delta, NANOSECONDS_PER_SECOND, rate); + t->start = (int64_t)t->start + + ((__int128_t)delta * NANOSECONDS_PER_SECOND / rate); aspeed_timer_mod(t); } break;