In order to better test hypervisor preemption paths, add an option to
artificially increase the number of preemptions.

While modifying xen-command-line.pandoc, escape some underscores, and
remove some trailing whitespace.

Signed-off-by: George Dunlap <george.dunlap@citrix.com>
---
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Jan Beulich <jbeulich@suse.com>
---
 docs/misc/xen-command-line.pandoc | 20 ++++++++++++++++++--
 xen/arch/x86/time.c               | 11 +++++++++++
 xen/include/xen/sched.h           | 10 +++++++++-
 3 files changed, 38 insertions(+), 3 deletions(-)

diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index 981a5e2381..1a9fda8627 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -636,13 +636,29 @@ Available alternatives, with their meaning, are:
 Specify the USB controller to use, either by instance number (when going
 over the PCI busses sequentially) or by PCI device (must be on segment 0).
 
-### debug_stack_lines
+### debug\_stack\_lines
 > `= <integer>`
 
 > Default: `20`
 
 Limits the number lines printed in Xen stack traces.
 
+### debug-synthetic-preemption
+> `= <integer>`
+
+> Default: `0`
+
+Artificially increases rate at which `hypercall_preempt_check()`
+returns `true`, for debugging purposes, to a rate of one in `N`. (The
+default, `0`, disables the feature.)
+
+When promoting pagetables, for instance, `hypercall_preempt_check()`
+is called before processing each PTE.  Since there are 512 PTEs per
+page, a value of `1024` should result in pagetable promotion being
+interrupted every other page on average.
+
+Only available in DEBUG builds.
+
 ### debugtrace
 > `= [cpu:]<size>`
 
@@ -1690,7 +1706,7 @@ The following resources are available:
     CDP, one COS will corespond two CBMs other than one with CAT, due to the
     sum of CBMs is fixed, that means actual `cos_max` in use will automatically
     reduce to half when CDP is enabled.
-	
+
 ### pv-linear-pt (x86)
 > `= <boolean>`
 
diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
index 64e471a39b..34302f81e7 100644
--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -43,6 +43,17 @@
 static char __initdata opt_clocksource[10];
 string_param("clocksource", opt_clocksource);
 
+#ifndef NDEBUG
+int debug_synthetic_preemption = 0;
+integer_param("debug-synthetic-preemption", debug_synthetic_preemption);
+
+bool synthetic_preemption_check(void) {
+    if ( debug_synthetic_preemption == 0 )
+        return false;
+    return !(rdtsc() % debug_synthetic_preemption);
+}
+#endif
+
 unsigned long __read_mostly cpu_khz;  /* CPU clock frequency in kHz. */
 DEFINE_SPINLOCK(rtc_lock);
 unsigned long pit0_ticks;
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 9f7bc69293..c0071eee04 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -748,6 +748,13 @@ static inline void hypercall_cancel_continuation(struct vcpu *v)
     v->hcall_preempted = false;
 }
 
+#ifndef NDEBUG
+bool synthetic_preemption_check(void);
+#define synthetic_preemption_check synthetic_preemption_check
+#else
+#define synthetic_preempiton_check() false
+#endif
+
 /*
  * For long-running operations that must be in hypercall context, check
  * if there is background work to be done that should interrupt this
@@ -755,7 +762,8 @@ static inline void hypercall_cancel_continuation(struct vcpu *v)
  */
 #define hypercall_preempt_check() (unlikely(    \
         softirq_pending(smp_processor_id()) |   \
-        local_events_need_delivery()            \
+        local_events_need_delivery() |          \
+        synthetic_preemption_check()            \
     ))
 
 /*
-- 
2.24.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

On 23/12/2019 16:43, George Dunlap wrote:
> In order to better test hypervisor preemption paths, add an option to
> artificially increase the number of preemptions.
>
> While modifying xen-command-line.pandoc, escape some underscores

This is pandoc, not markdown, and underscores like these are one of the
differences.  I specifically took these underscores out so the HTML
anchor links work correctly.

> , and
> remove some trailing whitespace.
>
> Signed-off-by: George Dunlap <george.dunlap@citrix.com>
> ---
> CC: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Jan Beulich <jbeulich@suse.com>
> ---
>  docs/misc/xen-command-line.pandoc | 20 ++++++++++++++++++--
>  xen/arch/x86/time.c               | 11 +++++++++++
>  xen/include/xen/sched.h           | 10 +++++++++-
>  3 files changed, 38 insertions(+), 3 deletions(-)
>
> diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
> index 981a5e2381..1a9fda8627 100644
> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -636,13 +636,29 @@ Available alternatives, with their meaning, are:
>  Specify the USB controller to use, either by instance number (when going
>  over the PCI busses sequentially) or by PCI device (must be on segment 0).
>  
> -### debug_stack_lines
> +### debug\_stack\_lines
>  > `= <integer>`
>  
>  > Default: `20`
>  
>  Limits the number lines printed in Xen stack traces.
>  
> +### debug-synthetic-preemption
> +> `= <integer>`
> +
> +> Default: `0`
> +
> +Artificially increases rate at which `hypercall_preempt_check()`
> +returns `true`, for debugging purposes, to a rate of one in `N`. (The
> +default, `0`, disables the feature.)
> +
> +When promoting pagetables, for instance, `hypercall_preempt_check()`
> +is called before processing each PTE.  Since there are 512 PTEs per
> +page, a value of `1024` should result in pagetable promotion being
> +interrupted every other page on average.
> +
> +Only available in DEBUG builds.

Please, not like this.  I learnt the hard way with hvm_fep that it is
important to have functionality like this in release builds as well.

Give it a Kconfig option, and default it to DEBUG.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Hi George,

I was expecting a bigger list of CC here. What did you use to compute it?

On Mon, 23 Dec 2019, 17:45 George Dunlap, <george.dunlap@citrix.com> wrote:

> In order to better test hypervisor preemption paths, add an option to
> artificially increase the number of preemptions.
>
> While modifying xen-command-line.pandoc, escape some underscores, and
> remove some trailing whitespace.
>
> Signed-off-by: George Dunlap <george.dunlap@citrix.com>
> ---
> CC: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Jan Beulich <jbeulich@suse.com>
> ---
>  docs/misc/xen-command-line.pandoc | 20 ++++++++++++++++++--
>  xen/arch/x86/time.c               | 11 +++++++++++
>  xen/include/xen/sched.h           | 10 +++++++++-
>  3 files changed, 38 insertions(+), 3 deletions(-)
>
> diff --git a/docs/misc/xen-command-line.pandoc
> b/docs/misc/xen-command-line.pandoc
> index 981a5e2381..1a9fda8627 100644
> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -636,13 +636,29 @@ Available alternatives, with their meaning, are:
>  Specify the USB controller to use, either by instance number (when going
>  over the PCI busses sequentially) or by PCI device (must be on segment 0).
>
> -### debug_stack_lines
> +### debug\_stack\_lines
>  > `= <integer>`
>
>  > Default: `20`
>
>  Limits the number lines printed in Xen stack traces.
>
> +### debug-synthetic-preemption
> +> `= <integer>`
> +
> +> Default: `0`
> +
> +Artificially increases rate at which `hypercall_preempt_check()`
> +returns `true`, for debugging purposes, to a rate of one in `N`. (The
> +default, `0`, disables the feature.)
> +
> +When promoting pagetables, for instance, `hypercall_preempt_check()`
> +is called before processing each PTE.  Since there are 512 PTEs per
> +page, a value of `1024` should result in pagetable promotion being
> +interrupted every other page on average.
> +
> +Only available in DEBUG builds.
> +
>  ### debugtrace
>  > `= [cpu:]<size>`
>
> @@ -1690,7 +1706,7 @@ The following resources are available:
>      CDP, one COS will corespond two CBMs other than one with CAT, due to
> the
>      sum of CBMs is fixed, that means actual `cos_max` in use will
> automatically
>      reduce to half when CDP is enabled.
> -
> +
>  ### pv-linear-pt (x86)
>  > `= <boolean>`
>
> diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
> index 64e471a39b..34302f81e7 100644
> --- a/xen/arch/x86/time.c
> +++ b/xen/arch/x86/time.c
> @@ -43,6 +43,17 @@
>  static char __initdata opt_clocksource[10];
>  string_param("clocksource", opt_clocksource);
>
> +#ifndef NDEBUG
> +int debug_synthetic_preemption = 0;
> +integer_param("debug-synthetic-preemption", debug_synthetic_preemption);
> +
> +bool synthetic_preemption_check(void) {
> +    if ( debug_synthetic_preemption == 0 )
> +        return false;
> +    return !(rdtsc() % debug_synthetic_preemption);
> +}
> +#endif

+
>  unsigned long __read_mostly cpu_khz;  /* CPU clock frequency in kHz. */
>  DEFINE_SPINLOCK(rtc_lock);
>  unsigned long pit0_ticks;
> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> index 9f7bc69293..c0071eee04 100644
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -748,6 +748,13 @@ static inline void
> hypercall_cancel_continuation(struct vcpu *v)
>      v->hcall_preempted = false;
>  }
>
> +#ifndef NDEBUG
> +bool synthetic_preemption_check(void);
> +#define synthetic_preemption_check synthetic_preemption_check


Why do you need this define?

+#else
> +#define synthetic_preempiton_check() false
>

Typo in the name. Also, it seems like this wasn't tested on Arm and, AFAICT
break because the function would not be definr in debug build.

But, I am not sure why the implementation needs to be arch specific when
get_cycles() could do the job.

+#endif
> +
>  /*
>   * For long-running operations that must be in hypercall context, check
>   * if there is background work to be done that should interrupt this
> @@ -755,7 +762,8 @@ static inline void
> hypercall_cancel_continuation(struct vcpu *v)
>   */
>  #define hypercall_preempt_check() (unlikely(    \
>          softirq_pending(smp_processor_id()) |   \
> -        local_events_need_delivery()            \
> +        local_events_need_delivery() |          \
> +        synthetic_preemption_check()            \


The function you return bool, so shouldn't it be ||?

Cheers,

     ))
>
>  /*
> --
> 2.24.0
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

On 23.12.2019 17:43, George Dunlap wrote:
> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -636,13 +636,29 @@ Available alternatives, with their meaning, are:
>  Specify the USB controller to use, either by instance number (when going
>  over the PCI busses sequentially) or by PCI device (must be on segment 0).
>  
> -### debug_stack_lines
> +### debug\_stack\_lines
>  > `= <integer>`
>  
>  > Default: `20`
>  
>  Limits the number lines printed in Xen stack traces.
>  
> +### debug-synthetic-preemption
> +> `= <integer>`
> +
> +> Default: `0`
> +
> +Artificially increases rate at which `hypercall_preempt_check()`
> +returns `true`, for debugging purposes, to a rate of one in `N`. (The
> +default, `0`, disables the feature.)
> +
> +When promoting pagetables, for instance, `hypercall_preempt_check()`
> +is called before processing each PTE.  Since there are 512 PTEs per
> +page, a value of `1024` should result in pagetable promotion being
> +interrupted every other page on average.

If this is meant to be x86 only, then text here should state this.
Otherwise I think it would help if the example described in the
last sentence would mention its x86 relationship.

> --- a/xen/arch/x86/time.c
> +++ b/xen/arch/x86/time.c
> @@ -43,6 +43,17 @@
>  static char __initdata opt_clocksource[10];
>  string_param("clocksource", opt_clocksource);
>  
> +#ifndef NDEBUG
> +int debug_synthetic_preemption = 0;

static unsigned int __read_mostly?

> +integer_param("debug-synthetic-preemption", debug_synthetic_preemption);

Perhaps allow changing at runtime?

> +bool synthetic_preemption_check(void) {

Brace placement.

> +    if ( debug_synthetic_preemption == 0 )
> +        return false;
> +    return !(rdtsc() % debug_synthetic_preemption);

Please consistently use either ! or "== 0".

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel