> On May 3, 2019, at 11:57 AM, Krish Sadhukhan <krish.sadhukhan@xxxxxxxxxx> wrote:
>
>
>
> On 05/03/2019 04:13 AM, nadav.amit@xxxxxxxxx wrote:
>> From: Nadav Amit <nadav.amit@xxxxxxxxx>
>>
>> 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@xxxxxxxxxx>
>> Signed-off-by: Nadav Amit <nadav.amit@xxxxxxxxx>
>> ---
>> 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…)
