| Message ID | 20190504223618.26742-1-nadav.amit@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [kvm-unit-tests,v3] x86: Incorporate timestamp in delay() and call the latter in io_delay() | expand |
On 05/05/19 00:36, Nadav Amit wrote: > There is no guarantee that a self-IPI would be delivered immediately. > In eventinj, io_delay() is called after self-IPI is generated but does > nothing. > > In general, there is mess in regard to delay() and io_delay(). There are > two definitions of delay() and they do not really look on the timestamp > counter and instead count invocations of "pause" (or even "nop"), which > might be different on different CPUs/setups, for example due to > different pause-loop-exiting configurations. > > To address these issues change io_delay() to really do a delay, based on > timestamp counter, and move common functions into delay.[hc]. > > Cc: Krish Sadhukhan <krish.sadhukhan@oracle.com> > Signed-off-by: Nadav Amit <nadav.amit@gmail.com> I had already included the fixed commit message in v2, thanks anyway for sending this one. :) Paolo > --- > lib/x86/delay.c | 9 ++++++--- > lib/x86/delay.h | 7 +++++++ > x86/eventinj.c | 5 +---- > x86/ioapic.c | 8 +------- > 4 files changed, 15 insertions(+), 14 deletions(-) > > diff --git a/lib/x86/delay.c b/lib/x86/delay.c > index 595ad24..e7d2717 100644 > --- a/lib/x86/delay.c > +++ b/lib/x86/delay.c > @@ -1,8 +1,11 @@ > #include "delay.h" > +#include "processor.h" > > void delay(u64 count) > { > - while (count--) > - asm volatile("pause"); > -} > + u64 start = rdtsc(); > > + do { > + pause(); > + } while (rdtsc() - start < count); > +} > diff --git a/lib/x86/delay.h b/lib/x86/delay.h > index a9bf894..a51eb34 100644 > --- a/lib/x86/delay.h > +++ b/lib/x86/delay.h > @@ -3,6 +3,13 @@ > > #include "libcflat.h" > > +#define IPI_DELAY 1000000 > + > void delay(u64 count); > > +static inline void io_delay(void) > +{ > + delay(IPI_DELAY); > +} > + > #endif > diff --git a/x86/eventinj.c b/x86/eventinj.c > index d2dfc40..901b9db 100644 > --- a/x86/eventinj.c > +++ b/x86/eventinj.c > @@ -7,6 +7,7 @@ > #include "apic-defs.h" > #include "vmalloc.h" > #include "alloc_page.h" > +#include "delay.h" > > #ifdef __x86_64__ > # define R "r" > @@ -16,10 +17,6 @@ > > void do_pf_tss(void); > > -static inline void io_delay(void) > -{ > -} > - > static void apic_self_ipi(u8 v) > { > apic_icr_write(APIC_DEST_SELF | APIC_DEST_PHYSICAL | APIC_DM_FIXED | > diff --git a/x86/ioapic.c b/x86/ioapic.c > index 2ac4ac6..c32dabd 100644 > --- a/x86/ioapic.c > +++ b/x86/ioapic.c > @@ -4,6 +4,7 @@ > #include "smp.h" > #include "desc.h" > #include "isr.h" > +#include "delay.h" > > static void toggle_irq_line(unsigned line) > { > @@ -165,13 +166,6 @@ static void test_ioapic_level_tmr(bool expected_tmr_before) > expected_tmr_before ? "true" : "false"); > } > > -#define IPI_DELAY 1000000 > - > -static void delay(int count) > -{ > - while(count--) asm(""); > -} > - > static void toggle_irq_line_0x0e(void *data) > { > irq_disable(); >
diff --git a/lib/x86/delay.c b/lib/x86/delay.c index 595ad24..e7d2717 100644 --- a/lib/x86/delay.c +++ b/lib/x86/delay.c @@ -1,8 +1,11 @@ #include "delay.h" +#include "processor.h" void delay(u64 count) { - while (count--) - asm volatile("pause"); -} + u64 start = rdtsc(); + do { + pause(); + } while (rdtsc() - start < count); +} diff --git a/lib/x86/delay.h b/lib/x86/delay.h index a9bf894..a51eb34 100644 --- a/lib/x86/delay.h +++ b/lib/x86/delay.h @@ -3,6 +3,13 @@ #include "libcflat.h" +#define IPI_DELAY 1000000 + void delay(u64 count); +static inline void io_delay(void) +{ + delay(IPI_DELAY); +} + #endif diff --git a/x86/eventinj.c b/x86/eventinj.c index d2dfc40..901b9db 100644 --- a/x86/eventinj.c +++ b/x86/eventinj.c @@ -7,6 +7,7 @@ #include "apic-defs.h" #include "vmalloc.h" #include "alloc_page.h" +#include "delay.h" #ifdef __x86_64__ # define R "r" @@ -16,10 +17,6 @@ void do_pf_tss(void); -static inline void io_delay(void) -{ -} - static void apic_self_ipi(u8 v) { apic_icr_write(APIC_DEST_SELF | APIC_DEST_PHYSICAL | APIC_DM_FIXED | diff --git a/x86/ioapic.c b/x86/ioapic.c index 2ac4ac6..c32dabd 100644 --- a/x86/ioapic.c +++ b/x86/ioapic.c @@ -4,6 +4,7 @@ #include "smp.h" #include "desc.h" #include "isr.h" +#include "delay.h" static void toggle_irq_line(unsigned line) { @@ -165,13 +166,6 @@ static void test_ioapic_level_tmr(bool expected_tmr_before) expected_tmr_before ? "true" : "false"); } -#define IPI_DELAY 1000000 - -static void delay(int count) -{ - while(count--) asm(""); -} - static void toggle_irq_line_0x0e(void *data) { irq_disable();
There is no guarantee that a self-IPI would be delivered immediately. In eventinj, io_delay() is called after self-IPI is generated but does nothing. In general, there is mess in regard to delay() and io_delay(). There are two definitions of delay() and they do not really look on the timestamp counter and instead count invocations of "pause" (or even "nop"), which might be different on different CPUs/setups, for example due to different pause-loop-exiting configurations. To address these issues change io_delay() to really do a delay, based on timestamp counter, and move common functions into delay.[hc]. Cc: Krish Sadhukhan <krish.sadhukhan@oracle.com> Signed-off-by: Nadav Amit <nadav.amit@gmail.com> --- lib/x86/delay.c | 9 ++++++--- lib/x86/delay.h | 7 +++++++ x86/eventinj.c | 5 +---- x86/ioapic.c | 8 +------- 4 files changed, 15 insertions(+), 14 deletions(-)