| Message ID | 20190503111307.10716-1-nadav.amit@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] x86: Some cleanup of delay() and io_delay() | expand |
This one regards KVM-unit-tests, of course. > On May 3, 2019, at 4:13 AM, nadav.amit@gmail.com wrote: > > From: Nadav Amit <nadav.amit@gmail.com> > > 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(-) > > 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(); > -- > 2.17.1
On 05/03/2019 04:13 AM, nadav.amit@gmail.com wrote: > From: Nadav Amit <nadav.amit@gmail.com> > > 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(-) > > 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(); May be the commit header can be re-worded to something like the following in order to summarize your changes in a better way ? x86: Incorporate timestamp in delay() and call the latter in io_delay()
> On May 3, 2019, at 11:57 AM, Krish Sadhukhan <krish.sadhukhan@oracle.com> wrote: > > > > On 05/03/2019 04:13 AM, nadav.amit@gmail.com wrote: >> From: Nadav Amit <nadav.amit@gmail.com> >> >> 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(-) >> >> 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(); > > May be the commit header can be re-worded to something like the following in order to summarize your changes in a better way ? > > > x86: Incorporate timestamp in delay() and call the latter in io_delay() Sounds good. Let’s see if there is any additional feedback before I send v3 (and whether Paolo will just do the rewording when he applies the patch…)
On 03/05/19 13:13, nadav.amit@gmail.com wrote: > From: Nadav Amit <nadav.amit@gmail.com> > > 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> Queued; there is another io_delay instance in taskswitch2.c, so I removed it too. 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();