[PATCH v2] x86: Some cleanup of delay() and io_delay()

[PATCH v2] x86: Some cleanup of delay() and io_delay()

[nadav.amit]

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

Re: [PATCH v2] x86: Some cleanup of delay() and io_delay()

[Nadav Amit]

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

Re: [PATCH v2] x86: Some cleanup of delay() and io_delay()

[Krish Sadhukhan]

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()

Re: [PATCH v2] x86: Some cleanup of delay() and io_delay()

[Nadav Amit]

> 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…)

Re: [PATCH v2] x86: Some cleanup of delay() and io_delay()

[Paolo Bonzini]

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();
>