[RFC v3 0/2] x86/xen: add xen hypercall preemption

[RFC v3 0/2] x86/xen: add xen hypercall preemption

[Luis R. Rodriguez]

From: "Luis R. Rodriguez" <mcgrof@suse.com>

After my last respin Andy provided some ideas as how to skip
IRQ context hacks for preemption, this v3 spin addresses that
and a bit more.

This is based on both Andrew Cooper's and David Vrabel's work,
further modified based on ideas by Andy Lutomirski to avoid
having to deal with preemption on IRQ context. Ian had originally
suggested to avoid the pt_regs stuff by using a CPU variable but
based on Andy's observations it is difficult to prove we will
avoid recursing or bad nesting when dealing with preemption out
of IRQ context. This is specially true given that after a hypercall
gets preempted the hypercall may end up another CPU.

This uses NOKPROBE_SYMBOL and notrace since based on Andy's advice
I am not confident that tracing and kprobes are safe to use in what
might be an extended RCU quiescent state (i.e. where we're outside
irq_enter and irq_exit).

I've tested this on 64-bit, some testing on 32-bit would be
appreciated.

Luis R. Rodriguez (2):
  x86/xen: add xen_is_preemptible_hypercall()
  x86/xen: allow privcmd hypercalls to be preempted

 arch/x86/include/asm/xen/hypercall.h | 20 ++++++++++++++++++++
 arch/x86/kernel/entry_32.S           |  2 ++
 arch/x86/kernel/entry_64.S           |  2 ++
 arch/x86/xen/enlighten.c             |  7 +++++++
 arch/x86/xen/xen-head.S              | 18 +++++++++++++++++-
 drivers/xen/events/events_base.c     | 13 +++++++++++++
 include/xen/events.h                 |  1 +
 7 files changed, 62 insertions(+), 1 deletion(-)

-- 
2.1.1

[RFC v3 1/2] x86/xen: add xen_is_preemptible_hypercall()

[Luis R. Rodriguez]

From: "Luis R. Rodriguez" <mcgrof@suse.com>

On kernels with voluntary or no preemption we can run
into situations where a hypercall issued through userspace
will linger around as it addresses sub-operatiosn in kernel
context (multicalls). Such operations can trigger soft lockup
detection.

We want to address a way to let the kernel voluntarily preempt
such calls even on non preempt kernels, to address this we first
need to distinguish which hypercalls fall under this category.
This implements xen_is_preemptible_hypercall() which lets us do
just that by adding a secondary hypercall page, calls made via
the new page may be preempted.

Andrew had originally submitted a version of this work [0].

[0] http://lists.xen.org/archives/html/xen-devel/2014-02/msg01056.html

Based on original work by: Andrew Cooper <andrew.cooper3@citrix.com>

Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Borislav Petkov <bp@suse.de>
Cc: David Vrabel <david.vrabel@citrix.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: x86@kernel.org
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Cc: Jan Beulich <JBeulich@suse.com>
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
---
 arch/x86/include/asm/xen/hypercall.h | 20 ++++++++++++++++++++
 arch/x86/xen/enlighten.c             |  7 +++++++
 arch/x86/xen/xen-head.S              | 18 +++++++++++++++++-
 3 files changed, 44 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/xen/hypercall.h b/arch/x86/include/asm/xen/hypercall.h
index ca08a27..221008e 100644
--- a/arch/x86/include/asm/xen/hypercall.h
+++ b/arch/x86/include/asm/xen/hypercall.h
@@ -84,6 +84,22 @@
 
 extern struct { char _entry[32]; } hypercall_page[];
 
+#ifndef CONFIG_PREEMPT
+extern struct { char _entry[32]; } preemptible_hypercall_page[];
+
+static inline bool xen_is_preemptible_hypercall(struct pt_regs *regs)
+{
+	return !user_mode_vm(regs) &&
+		regs->ip >= (unsigned long)preemptible_hypercall_page &&
+		regs->ip < (unsigned long)preemptible_hypercall_page + PAGE_SIZE;
+}
+#else
+static inline bool xen_is_preemptible_hypercall(struct pt_regs *regs)
+{
+	return false;
+}
+#endif
+
 #define __HYPERCALL		"call hypercall_page+%c[offset]"
 #define __HYPERCALL_ENTRY(x)						\
 	[offset] "i" (__HYPERVISOR_##x * sizeof(hypercall_page[0]))
@@ -215,7 +231,11 @@ privcmd_call(unsigned call,
 
 	asm volatile("call *%[call]"
 		     : __HYPERCALL_5PARAM
+#ifndef CONFIG_PREEMPT
+		     : [call] "a" (&preemptible_hypercall_page[call])
+#else
 		     : [call] "a" (&hypercall_page[call])
+#endif
 		     : __HYPERCALL_CLOBBER5);
 
 	return (long)__res;
diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
index 6bf3a13..9c01b48 100644
--- a/arch/x86/xen/enlighten.c
+++ b/arch/x86/xen/enlighten.c
@@ -84,6 +84,9 @@
 #include "multicalls.h"
 
 EXPORT_SYMBOL_GPL(hypercall_page);
+#ifndef CONFIG_PREEMPT
+EXPORT_SYMBOL_GPL(preemptible_hypercall_page);
+#endif
 
 /*
  * Pointer to the xen_vcpu_info structure or
@@ -1531,6 +1534,10 @@ asmlinkage __visible void __init xen_start_kernel(void)
 #endif
 	xen_setup_machphys_mapping();
 
+#ifndef CONFIG_PREEMPT
+	copy_page(preemptible_hypercall_page, hypercall_page);
+#endif
+
 	/* Install Xen paravirt ops */
 	pv_info = xen_info;
 	pv_init_ops = xen_init_ops;
diff --git a/arch/x86/xen/xen-head.S b/arch/x86/xen/xen-head.S
index 674b2225..6e6a9517 100644
--- a/arch/x86/xen/xen-head.S
+++ b/arch/x86/xen/xen-head.S
@@ -85,9 +85,18 @@ ENTRY(xen_pvh_early_cpu_init)
 .pushsection .text
 	.balign PAGE_SIZE
 ENTRY(hypercall_page)
+
+#ifdef CONFIG_PREEMPT
+#  define PREEMPT_HYPERCALL_ENTRY(x)
+#else
+#  define PREEMPT_HYPERCALL_ENTRY(x) \
+       .global xen_hypercall_##x ## _p ASM_NL \
+       .set preemptible_xen_hypercall_##x, xen_hypercall_##x + PAGE_SIZE ASM_NL
+#endif
 #define NEXT_HYPERCALL(x) \
 	ENTRY(xen_hypercall_##x) \
-	.skip 32
+	.skip 32 ASM_NL \
+	PREEMPT_HYPERCALL_ENTRY(x)
 
 NEXT_HYPERCALL(set_trap_table)
 NEXT_HYPERCALL(mmu_update)
@@ -138,6 +147,13 @@ NEXT_HYPERCALL(arch_4)
 NEXT_HYPERCALL(arch_5)
 NEXT_HYPERCALL(arch_6)
 	.balign PAGE_SIZE
+
+#ifndef CONFIG_PREEMPT
+ENTRY(preemptible_hypercall_page)
+	.skip PAGE_SIZE
+#endif /* CONFIG_PREEMPT */
+
+#undef NEXT_HYPERCALL
 .popsection
 
 	ELFNOTE(Xen, XEN_ELFNOTE_GUEST_OS,       .asciz "linux")
-- 
2.1.1

Re: [RFC v3 1/2] x86/xen: add xen_is_preemptible_hypercall()

[Andy Lutomirski]

On Wed, Jan 21, 2015 at 6:17 PM, Luis R. Rodriguez
<mcgrof@do-not-panic.com> wrote:
> From: "Luis R. Rodriguez" <mcgrof@suse.com>
>
> On kernels with voluntary or no preemption we can run
> into situations where a hypercall issued through userspace
> will linger around as it addresses sub-operatiosn in kernel
> context (multicalls). Such operations can trigger soft lockup
> detection.
>
> We want to address a way to let the kernel voluntarily preempt
> such calls even on non preempt kernels, to address this we first
> need to distinguish which hypercalls fall under this category.
> This implements xen_is_preemptible_hypercall() which lets us do
> just that by adding a secondary hypercall page, calls made via
> the new page may be preempted.
>
> Andrew had originally submitted a version of this work [0].
>
> [0] http://lists.xen.org/archives/html/xen-devel/2014-02/msg01056.html
>
> Based on original work by: Andrew Cooper <andrew.cooper3@citrix.com>
>
> Cc: Andy Lutomirski <luto@amacapital.net>
> Cc: Borislav Petkov <bp@suse.de>
> Cc: David Vrabel <david.vrabel@citrix.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: x86@kernel.org
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> Cc: Jan Beulich <JBeulich@suse.com>
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
> ---
>  arch/x86/include/asm/xen/hypercall.h | 20 ++++++++++++++++++++
>  arch/x86/xen/enlighten.c             |  7 +++++++
>  arch/x86/xen/xen-head.S              | 18 +++++++++++++++++-
>  3 files changed, 44 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/include/asm/xen/hypercall.h b/arch/x86/include/asm/xen/hypercall.h
> index ca08a27..221008e 100644
> --- a/arch/x86/include/asm/xen/hypercall.h
> +++ b/arch/x86/include/asm/xen/hypercall.h
> @@ -84,6 +84,22 @@
>
>  extern struct { char _entry[32]; } hypercall_page[];
>
> +#ifndef CONFIG_PREEMPT
> +extern struct { char _entry[32]; } preemptible_hypercall_page[];

A comment somewhere explaining why only non-preemptible kernels have
preemptible hypercalls might be friendly to some future reader. :)

> +
> +static inline bool xen_is_preemptible_hypercall(struct pt_regs *regs)
> +{
> +       return !user_mode_vm(regs) &&
> +               regs->ip >= (unsigned long)preemptible_hypercall_page &&
> +               regs->ip < (unsigned long)preemptible_hypercall_page + PAGE_SIZE;
> +}

This makes it seem like the page is indeed one page long, but I don't
see what actually allocates a whole page for it.  What am I missing?

--Andy

Re: [RFC v3 1/2] x86/xen: add xen_is_preemptible_hypercall()

[Luis R. Rodriguez]

On Wed, Jan 21, 2015 at 07:07:36PM -0800, Andy Lutomirski wrote:
> On Wed, Jan 21, 2015 at 6:17 PM, Luis R. Rodriguez
> <mcgrof@do-not-panic.com> wrote:
> > From: "Luis R. Rodriguez" <mcgrof@suse.com>
> >
> > On kernels with voluntary or no preemption we can run
> > into situations where a hypercall issued through userspace
> > will linger around as it addresses sub-operatiosn in kernel
> > context (multicalls). Such operations can trigger soft lockup
> > detection.
> >
> > We want to address a way to let the kernel voluntarily preempt
> > such calls even on non preempt kernels, to address this we first
> > need to distinguish which hypercalls fall under this category.
> > This implements xen_is_preemptible_hypercall() which lets us do
> > just that by adding a secondary hypercall page, calls made via
> > the new page may be preempted.
> >
> > Andrew had originally submitted a version of this work [0].
> >
> > [0] http://lists.xen.org/archives/html/xen-devel/2014-02/msg01056.html
> >
> > Based on original work by: Andrew Cooper <andrew.cooper3@citrix.com>
> >
> > Cc: Andy Lutomirski <luto@amacapital.net>
> > Cc: Borislav Petkov <bp@suse.de>
> > Cc: David Vrabel <david.vrabel@citrix.com>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Ingo Molnar <mingo@redhat.com>
> > Cc: "H. Peter Anvin" <hpa@zytor.com>
> > Cc: x86@kernel.org
> > Cc: Steven Rostedt <rostedt@goodmis.org>
> > Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> > Cc: Jan Beulich <JBeulich@suse.com>
> > Cc: linux-kernel@vger.kernel.org
> > Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
> > ---
> >  arch/x86/include/asm/xen/hypercall.h | 20 ++++++++++++++++++++
> >  arch/x86/xen/enlighten.c             |  7 +++++++
> >  arch/x86/xen/xen-head.S              | 18 +++++++++++++++++-
> >  3 files changed, 44 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/x86/include/asm/xen/hypercall.h b/arch/x86/include/asm/xen/hypercall.h
> > index ca08a27..221008e 100644
> > --- a/arch/x86/include/asm/xen/hypercall.h
> > +++ b/arch/x86/include/asm/xen/hypercall.h
> > @@ -84,6 +84,22 @@
> >
> >  extern struct { char _entry[32]; } hypercall_page[];
> >
> > +#ifndef CONFIG_PREEMPT
> > +extern struct { char _entry[32]; } preemptible_hypercall_page[];
> 
> A comment somewhere explaining why only non-preemptible kernels have
> preemptible hypercalls might be friendly to some future reader. :)

Good idea, since this section is arch specific, I'll instead add a blurb
explaining this on the upcall.

> > +
> > +static inline bool xen_is_preemptible_hypercall(struct pt_regs *regs)
> > +{
> > +       return !user_mode_vm(regs) &&
> > +               regs->ip >= (unsigned long)preemptible_hypercall_page &&
> > +               regs->ip < (unsigned long)preemptible_hypercall_page + PAGE_SIZE;
> > +}
> 
> This makes it seem like the page is indeed one page long, but I don't
> see what actually allocates a whole page for it.  What am I missing?

arch/x86/xen/xen-head.S

.pushsection .text                                                              
        .balign PAGE_SIZE                                                       
ENTRY(hypercall_page) 

#ifndef CONFIG_PREEMPT                                                          
ENTRY(preemptible_hypercall_page)                                               
        .skip PAGE_SIZE                                                         
#endif /* CONFIG_PREEMPT */  

Does that suffice to be sure?

 Luis

Re: [RFC v3 1/2] x86/xen: add xen_is_preemptible_hypercall()

[Andy Lutomirski]

On Thu, Jan 22, 2015 at 11:30 AM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
> On Wed, Jan 21, 2015 at 07:07:36PM -0800, Andy Lutomirski wrote:
>> On Wed, Jan 21, 2015 at 6:17 PM, Luis R. Rodriguez
>> <mcgrof@do-not-panic.com> wrote:
>> > From: "Luis R. Rodriguez" <mcgrof@suse.com>
>> >
>> > On kernels with voluntary or no preemption we can run
>> > into situations where a hypercall issued through userspace
>> > will linger around as it addresses sub-operatiosn in kernel
>> > context (multicalls). Such operations can trigger soft lockup
>> > detection.
>> >
>> > We want to address a way to let the kernel voluntarily preempt
>> > such calls even on non preempt kernels, to address this we first
>> > need to distinguish which hypercalls fall under this category.
>> > This implements xen_is_preemptible_hypercall() which lets us do
>> > just that by adding a secondary hypercall page, calls made via
>> > the new page may be preempted.
>> >
>> > Andrew had originally submitted a version of this work [0].
>> >
>> > [0] http://lists.xen.org/archives/html/xen-devel/2014-02/msg01056.html
>> >
>> > Based on original work by: Andrew Cooper <andrew.cooper3@citrix.com>
>> >
>> > Cc: Andy Lutomirski <luto@amacapital.net>
>> > Cc: Borislav Petkov <bp@suse.de>
>> > Cc: David Vrabel <david.vrabel@citrix.com>
>> > Cc: Thomas Gleixner <tglx@linutronix.de>
>> > Cc: Ingo Molnar <mingo@redhat.com>
>> > Cc: "H. Peter Anvin" <hpa@zytor.com>
>> > Cc: x86@kernel.org
>> > Cc: Steven Rostedt <rostedt@goodmis.org>
>> > Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
>> > Cc: Jan Beulich <JBeulich@suse.com>
>> > Cc: linux-kernel@vger.kernel.org
>> > Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
>> > ---
>> >  arch/x86/include/asm/xen/hypercall.h | 20 ++++++++++++++++++++
>> >  arch/x86/xen/enlighten.c             |  7 +++++++
>> >  arch/x86/xen/xen-head.S              | 18 +++++++++++++++++-
>> >  3 files changed, 44 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/arch/x86/include/asm/xen/hypercall.h b/arch/x86/include/asm/xen/hypercall.h
>> > index ca08a27..221008e 100644
>> > --- a/arch/x86/include/asm/xen/hypercall.h
>> > +++ b/arch/x86/include/asm/xen/hypercall.h
>> > @@ -84,6 +84,22 @@
>> >
>> >  extern struct { char _entry[32]; } hypercall_page[];
>> >
>> > +#ifndef CONFIG_PREEMPT
>> > +extern struct { char _entry[32]; } preemptible_hypercall_page[];
>>
>> A comment somewhere explaining why only non-preemptible kernels have
>> preemptible hypercalls might be friendly to some future reader. :)
>
> Good idea, since this section is arch specific, I'll instead add a blurb
> explaining this on the upcall.
>
>> > +
>> > +static inline bool xen_is_preemptible_hypercall(struct pt_regs *regs)
>> > +{
>> > +       return !user_mode_vm(regs) &&
>> > +               regs->ip >= (unsigned long)preemptible_hypercall_page &&
>> > +               regs->ip < (unsigned long)preemptible_hypercall_page + PAGE_SIZE;
>> > +}
>>
>> This makes it seem like the page is indeed one page long, but I don't
>> see what actually allocates a whole page for it.  What am I missing?
>
> arch/x86/xen/xen-head.S
>
> .pushsection .text
>         .balign PAGE_SIZE
> ENTRY(hypercall_page)
>
> #ifndef CONFIG_PREEMPT
> ENTRY(preemptible_hypercall_page)
>         .skip PAGE_SIZE
> #endif /* CONFIG_PREEMPT */
>
> Does that suffice to be sure?

This looks like hypercall_page and preemptible_hypercall_page will
both be page-aligned but will be the same page.  Should there be
another .skip PAGE_SIZE in there?

--Andy

>
>  Luis



-- 
Andy Lutomirski
AMA Capital Management, LLC

Re: [RFC v3 1/2] x86/xen: add xen_is_preemptible_hypercall()

[Luis R. Rodriguez]

On Thu, Jan 22, 2015 at 12:01:50PM -0800, Andy Lutomirski wrote:
> On Thu, Jan 22, 2015 at 11:30 AM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
> > On Wed, Jan 21, 2015 at 07:07:36PM -0800, Andy Lutomirski wrote:
> >> On Wed, Jan 21, 2015 at 6:17 PM, Luis R. Rodriguez
> >> <mcgrof@do-not-panic.com> wrote:
> >> > From: "Luis R. Rodriguez" <mcgrof@suse.com>
> >> >
> >> > On kernels with voluntary or no preemption we can run
> >> > into situations where a hypercall issued through userspace
> >> > will linger around as it addresses sub-operatiosn in kernel
> >> > context (multicalls). Such operations can trigger soft lockup
> >> > detection.
> >> >
> >> > We want to address a way to let the kernel voluntarily preempt
> >> > such calls even on non preempt kernels, to address this we first
> >> > need to distinguish which hypercalls fall under this category.
> >> > This implements xen_is_preemptible_hypercall() which lets us do
> >> > just that by adding a secondary hypercall page, calls made via
> >> > the new page may be preempted.
> >> >
> >> > Andrew had originally submitted a version of this work [0].
> >> >
> >> > [0] http://lists.xen.org/archives/html/xen-devel/2014-02/msg01056.html
> >> >
> >> > Based on original work by: Andrew Cooper <andrew.cooper3@citrix.com>
> >> >
> >> > Cc: Andy Lutomirski <luto@amacapital.net>
> >> > Cc: Borislav Petkov <bp@suse.de>
> >> > Cc: David Vrabel <david.vrabel@citrix.com>
> >> > Cc: Thomas Gleixner <tglx@linutronix.de>
> >> > Cc: Ingo Molnar <mingo@redhat.com>
> >> > Cc: "H. Peter Anvin" <hpa@zytor.com>
> >> > Cc: x86@kernel.org
> >> > Cc: Steven Rostedt <rostedt@goodmis.org>
> >> > Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> >> > Cc: Jan Beulich <JBeulich@suse.com>
> >> > Cc: linux-kernel@vger.kernel.org
> >> > Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
> >> > ---
> >> >  arch/x86/include/asm/xen/hypercall.h | 20 ++++++++++++++++++++
> >> >  arch/x86/xen/enlighten.c             |  7 +++++++
> >> >  arch/x86/xen/xen-head.S              | 18 +++++++++++++++++-
> >> >  3 files changed, 44 insertions(+), 1 deletion(-)
> >> >
> >> > diff --git a/arch/x86/include/asm/xen/hypercall.h b/arch/x86/include/asm/xen/hypercall.h
> >> > index ca08a27..221008e 100644
> >> > --- a/arch/x86/include/asm/xen/hypercall.h
> >> > +++ b/arch/x86/include/asm/xen/hypercall.h
> >> > @@ -84,6 +84,22 @@
> >> >
> >> >  extern struct { char _entry[32]; } hypercall_page[];
> >> >
> >> > +#ifndef CONFIG_PREEMPT
> >> > +extern struct { char _entry[32]; } preemptible_hypercall_page[];
> >>
> >> A comment somewhere explaining why only non-preemptible kernels have
> >> preemptible hypercalls might be friendly to some future reader. :)
> >
> > Good idea, since this section is arch specific, I'll instead add a blurb
> > explaining this on the upcall.
> >
> >> > +
> >> > +static inline bool xen_is_preemptible_hypercall(struct pt_regs *regs)
> >> > +{
> >> > +       return !user_mode_vm(regs) &&
> >> > +               regs->ip >= (unsigned long)preemptible_hypercall_page &&
> >> > +               regs->ip < (unsigned long)preemptible_hypercall_page + PAGE_SIZE;
> >> > +}
> >>
> >> This makes it seem like the page is indeed one page long, but I don't
> >> see what actually allocates a whole page for it.  What am I missing?
> >
> > arch/x86/xen/xen-head.S
> >
> > .pushsection .text
> >         .balign PAGE_SIZE
> > ENTRY(hypercall_page)
> >
> > #ifndef CONFIG_PREEMPT
> > ENTRY(preemptible_hypercall_page)
> >         .skip PAGE_SIZE
> > #endif /* CONFIG_PREEMPT */
> >
> > Does that suffice to be sure?
> 
> This looks like hypercall_page and preemptible_hypercall_page will
> both be page-aligned but will be the same page.  Should there be
> another .skip PAGE_SIZE in there?

I think the trick here was since hypercall_page is already aligned,
and we are just allocation PAGE_SIZE we are essentially pegging
preemptible_hypercall_page right after hypercall_page.

Andrew, David, can you confirm?

  Luis

Re: [RFC v3 1/2] x86/xen: add xen_is_preemptible_hypercall()

[Andrew Cooper]

On 22/01/2015 21:09, Luis R. Rodriguez wrote:
> On Thu, Jan 22, 2015 at 12:01:50PM -0800, Andy Lutomirski wrote:
>> On Thu, Jan 22, 2015 at 11:30 AM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
>>> On Wed, Jan 21, 2015 at 07:07:36PM -0800, Andy Lutomirski wrote:
>>>> On Wed, Jan 21, 2015 at 6:17 PM, Luis R. Rodriguez
>>>> <mcgrof@do-not-panic.com> wrote:
>>>>> From: "Luis R. Rodriguez" <mcgrof@suse.com>
>>>>>
>>>>> On kernels with voluntary or no preemption we can run
>>>>> into situations where a hypercall issued through userspace
>>>>> will linger around as it addresses sub-operatiosn in kernel
>>>>> context (multicalls). Such operations can trigger soft lockup
>>>>> detection.
>>>>>
>>>>> We want to address a way to let the kernel voluntarily preempt
>>>>> such calls even on non preempt kernels, to address this we first
>>>>> need to distinguish which hypercalls fall under this category.
>>>>> This implements xen_is_preemptible_hypercall() which lets us do
>>>>> just that by adding a secondary hypercall page, calls made via
>>>>> the new page may be preempted.
>>>>>
>>>>> Andrew had originally submitted a version of this work [0].
>>>>>
>>>>> [0] http://lists.xen.org/archives/html/xen-devel/2014-02/msg01056.html
>>>>>
>>>>> Based on original work by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>>>
>>>>> Cc: Andy Lutomirski <luto@amacapital.net>
>>>>> Cc: Borislav Petkov <bp@suse.de>
>>>>> Cc: David Vrabel <david.vrabel@citrix.com>
>>>>> Cc: Thomas Gleixner <tglx@linutronix.de>
>>>>> Cc: Ingo Molnar <mingo@redhat.com>
>>>>> Cc: "H. Peter Anvin" <hpa@zytor.com>
>>>>> Cc: x86@kernel.org
>>>>> Cc: Steven Rostedt <rostedt@goodmis.org>
>>>>> Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
>>>>> Cc: Jan Beulich <JBeulich@suse.com>
>>>>> Cc: linux-kernel@vger.kernel.org
>>>>> Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
>>>>> ---
>>>>>  arch/x86/include/asm/xen/hypercall.h | 20 ++++++++++++++++++++
>>>>>  arch/x86/xen/enlighten.c             |  7 +++++++
>>>>>  arch/x86/xen/xen-head.S              | 18 +++++++++++++++++-
>>>>>  3 files changed, 44 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/arch/x86/include/asm/xen/hypercall.h b/arch/x86/include/asm/xen/hypercall.h
>>>>> index ca08a27..221008e 100644
>>>>> --- a/arch/x86/include/asm/xen/hypercall.h
>>>>> +++ b/arch/x86/include/asm/xen/hypercall.h
>>>>> @@ -84,6 +84,22 @@
>>>>>
>>>>>  extern struct { char _entry[32]; } hypercall_page[];
>>>>>
>>>>> +#ifndef CONFIG_PREEMPT
>>>>> +extern struct { char _entry[32]; } preemptible_hypercall_page[];
>>>> A comment somewhere explaining why only non-preemptible kernels have
>>>> preemptible hypercalls might be friendly to some future reader. :)
>>> Good idea, since this section is arch specific, I'll instead add a blurb
>>> explaining this on the upcall.
>>>
>>>>> +
>>>>> +static inline bool xen_is_preemptible_hypercall(struct pt_regs *regs)
>>>>> +{
>>>>> +       return !user_mode_vm(regs) &&
>>>>> +               regs->ip >= (unsigned long)preemptible_hypercall_page &&
>>>>> +               regs->ip < (unsigned long)preemptible_hypercall_page + PAGE_SIZE;
>>>>> +}
>>>> This makes it seem like the page is indeed one page long, but I don't
>>>> see what actually allocates a whole page for it.  What am I missing?
>>> arch/x86/xen/xen-head.S
>>>
>>> .pushsection .text
>>>         .balign PAGE_SIZE
>>> ENTRY(hypercall_page)
>>>
>>> #ifndef CONFIG_PREEMPT
>>> ENTRY(preemptible_hypercall_page)
>>>         .skip PAGE_SIZE
>>> #endif /* CONFIG_PREEMPT */
>>>
>>> Does that suffice to be sure?
>> This looks like hypercall_page and preemptible_hypercall_page will
>> both be page-aligned but will be the same page.  Should there be
>> another .skip PAGE_SIZE in there?
> I think the trick here was since hypercall_page is already aligned,
> and we are just allocation PAGE_SIZE we are essentially pegging
> preemptible_hypercall_page right after hypercall_page.
>
> Andrew, David, can you confirm?

Your version is different to my original one (observe the lack of
NEXT_HYPERCALL()s), and I would agree that it would appear as if in your
version, hypercall_page and preemptible_hypercall_page are symbols with
the same address.

nm should give you a quick confirmation one way or another.

~Andrew

Re: [RFC v3 1/2] x86/xen: add xen_is_preemptible_hypercall()

[Luis R. Rodriguez]

On Thu, Jan 22, 2015 at 09:44:12PM +0000, Andrew Cooper wrote:
> On 22/01/2015 21:09, Luis R. Rodriguez wrote:
> > On Thu, Jan 22, 2015 at 12:01:50PM -0800, Andy Lutomirski wrote:
> >> On Thu, Jan 22, 2015 at 11:30 AM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
> >>> On Wed, Jan 21, 2015 at 07:07:36PM -0800, Andy Lutomirski wrote:
> >>>> On Wed, Jan 21, 2015 at 6:17 PM, Luis R. Rodriguez
> >>>> <mcgrof@do-not-panic.com> wrote:
> >>>>> From: "Luis R. Rodriguez" <mcgrof@suse.com>
> >>>>>
> >>>>> On kernels with voluntary or no preemption we can run
> >>>>> into situations where a hypercall issued through userspace
> >>>>> will linger around as it addresses sub-operatiosn in kernel
> >>>>> context (multicalls). Such operations can trigger soft lockup
> >>>>> detection.
> >>>>>
> >>>>> We want to address a way to let the kernel voluntarily preempt
> >>>>> such calls even on non preempt kernels, to address this we first
> >>>>> need to distinguish which hypercalls fall under this category.
> >>>>> This implements xen_is_preemptible_hypercall() which lets us do
> >>>>> just that by adding a secondary hypercall page, calls made via
> >>>>> the new page may be preempted.
> >>>>>
> >>>>> Andrew had originally submitted a version of this work [0].
> >>>>>
> >>>>> [0] http://lists.xen.org/archives/html/xen-devel/2014-02/msg01056.html
> >>>>>
> >>>>> Based on original work by: Andrew Cooper <andrew.cooper3@citrix.com>
> >>>>>
> >>>>> Cc: Andy Lutomirski <luto@amacapital.net>
> >>>>> Cc: Borislav Petkov <bp@suse.de>
> >>>>> Cc: David Vrabel <david.vrabel@citrix.com>
> >>>>> Cc: Thomas Gleixner <tglx@linutronix.de>
> >>>>> Cc: Ingo Molnar <mingo@redhat.com>
> >>>>> Cc: "H. Peter Anvin" <hpa@zytor.com>
> >>>>> Cc: x86@kernel.org
> >>>>> Cc: Steven Rostedt <rostedt@goodmis.org>
> >>>>> Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> >>>>> Cc: Jan Beulich <JBeulich@suse.com>
> >>>>> Cc: linux-kernel@vger.kernel.org
> >>>>> Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
> >>>>> ---
> >>>>>  arch/x86/include/asm/xen/hypercall.h | 20 ++++++++++++++++++++
> >>>>>  arch/x86/xen/enlighten.c             |  7 +++++++
> >>>>>  arch/x86/xen/xen-head.S              | 18 +++++++++++++++++-
> >>>>>  3 files changed, 44 insertions(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/arch/x86/include/asm/xen/hypercall.h b/arch/x86/include/asm/xen/hypercall.h
> >>>>> index ca08a27..221008e 100644
> >>>>> --- a/arch/x86/include/asm/xen/hypercall.h
> >>>>> +++ b/arch/x86/include/asm/xen/hypercall.h
> >>>>> @@ -84,6 +84,22 @@
> >>>>>
> >>>>>  extern struct { char _entry[32]; } hypercall_page[];
> >>>>>
> >>>>> +#ifndef CONFIG_PREEMPT
> >>>>> +extern struct { char _entry[32]; } preemptible_hypercall_page[];
> >>>> A comment somewhere explaining why only non-preemptible kernels have
> >>>> preemptible hypercalls might be friendly to some future reader. :)
> >>> Good idea, since this section is arch specific, I'll instead add a blurb
> >>> explaining this on the upcall.
> >>>
> >>>>> +
> >>>>> +static inline bool xen_is_preemptible_hypercall(struct pt_regs *regs)
> >>>>> +{
> >>>>> +       return !user_mode_vm(regs) &&
> >>>>> +               regs->ip >= (unsigned long)preemptible_hypercall_page &&
> >>>>> +               regs->ip < (unsigned long)preemptible_hypercall_page + PAGE_SIZE;
> >>>>> +}
> >>>> This makes it seem like the page is indeed one page long, but I don't
> >>>> see what actually allocates a whole page for it.  What am I missing?
> >>> arch/x86/xen/xen-head.S
> >>>
> >>> .pushsection .text
> >>>         .balign PAGE_SIZE
> >>> ENTRY(hypercall_page)
> >>>
> >>> #ifndef CONFIG_PREEMPT
> >>> ENTRY(preemptible_hypercall_page)
> >>>         .skip PAGE_SIZE
> >>> #endif /* CONFIG_PREEMPT */
> >>>
> >>> Does that suffice to be sure?
> >> This looks like hypercall_page and preemptible_hypercall_page will
> >> both be page-aligned but will be the same page.  Should there be
> >> another .skip PAGE_SIZE in there?
> > I think the trick here was since hypercall_page is already aligned,
> > and we are just allocation PAGE_SIZE we are essentially pegging
> > preemptible_hypercall_page right after hypercall_page.
> >
> > Andrew, David, can you confirm?
> 
> Your version is different to my original one (observe the lack of
> NEXT_HYPERCALL()s),

I don't get what is missing they should be pretty identical. It had:

--- a/arch/x86/xen/xen-head.S
+++ b/arch/x86/xen/xen-head.S
@@ -85,9 +85,18 @@ ENTRY(xen_pvh_early_cpu_init)
 .pushsection .text
        .balign PAGE_SIZE
 ENTRY(hypercall_page)
+
+#ifdef CONFIG_PREEMPT
+#  define PREEMPT_HYPERCALL_ENTRY(x)
+#else
+#  define PREEMPT_HYPERCALL_ENTRY(x) \
+       .global xen_hypercall_##x ## _p ASM_NL \
+       .set preemptible_xen_hypercall_##x, xen_hypercall_##x + PAGE_SIZE ASM_NL
+#endif
 #define NEXT_HYPERCALL(x) \
        ENTRY(xen_hypercall_##x) \
-       .skip 32
+       .skip 32 ASM_NL \
+       PREEMPT_HYPERCALL_ENTRY(x)

Is that what you mean?

> and I would agree that it would appear as if in your
> version, hypercall_page and preemptible_hypercall_page are symbols with
> the same address.
> 
> nm should give you a quick confirmation one way or another.

symtab:

mcgrof@ergon ~/linux (git::hypercall-preemption-v3)$ grep hypercall_page /boot/System.map-3.19.0-rc4-00040-g9c6fb2a 
ffffffff81001000 T hypercall_page
ffffffff81002000 T preemptible_hypercall_page

This is one page apart.

ffffffff81ae7b50 R __ksymtab_hypercall_page
ffffffff81aea610 R __ksymtab_preemptible_hypercall_page
ffffffff81af86c0 r __kcrctab_hypercall_page
ffffffff81af9c20 r __kcrctab_preemptible_hypercall_page
ffffffff81afbaa8 r __kstrtab_preemptible_hypercall_page
ffffffff81afbac3 r __kstrtab_hypercall_page

  Luis

[RFC v3 2/2] x86/xen: allow privcmd hypercalls to be preempted

[Luis R. Rodriguez]

From: "Luis R. Rodriguez" <mcgrof@suse.com>

Xen has support for splitting heavy work work into a series
of hypercalls, called multicalls, and preempting them through
what Xen calls continuation [0]. Despite this though without
CONFIG_PREEMPT preemption won't happen, without preemption
a system can become pretty useless on heavy handed hypercalls.
Such is the case for example when creating a > 50 GiB HVM guest,
we can get softlockups [1] with:.

kernel: [  802.084335] BUG: soft lockup - CPU#1 stuck for 22s! [xend:31351]

The softlock up triggers on the TASK_UNINTERRUPTIBLE hanger check
(default 120 seconds), on the Xen side in this particular case
this happens when the following Xen hypervisor code is used:

xc_domain_set_pod_target() -->
  do_memory_op() -->
    arch_memory_op() -->
      p2m_pod_set_mem_target()
	-- long delay (real or emulated) --

This happens on arch_memory_op() on the XENMEM_set_pod_target memory
op even though arch_memory_op() can handle continuation via
hypercall_create_continuation() for example.

Machines over 50 GiB of memory are on high demand and hard to come
by so to help replicate this sort of issue long delays on select
hypercalls have been emulated in order to be able to test this on
smaller machines [2].

On one hand this issue can be considered as expected given that
CONFIG_PREEMPT=n is used however we have forced voluntary preemption
precedent practices in the kernel even for CONFIG_PREEMPT=n through
the usage of cond_resched() sprinkled in many places. To address
this issue with Xen hypercalls though we need to find a way to aid
to the schedular in the middle of hypercalls. We are motivated to
address this issue on CONFIG_PREEMPT=n as otherwise the system becomes
rather unresponsive for long periods of time; in the worst case, at least
only currently by emulating long delays on select io disk bound
hypercalls, this can lead to filesystem corruption if the delay happens
for example on SCHEDOP_remote_shutdown (when we call 'xl <domain> shutdown').

We can address this problem by trying to check if we should schedule
on the xen timer in the middle of a hypercall on the return from the
timer interrupt. We want to be careful to not always force voluntary
preemption though so to do this we only selectively enable preemption
on very specific xen hypercalls.

This enables hypercall preemption by selectively forcing checks for
voluntary preempting only on ioctl initiated private hypercalls
where we know some folks have run into reported issues [1].

[0] http://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=42217cbc5b3e84b8c145d8cfb62dd5de0134b9e8;hp=3a0b9c57d5c9e82c55dd967c84dd06cb43c49ee9
[1] https://bugzilla.novell.com/show_bug.cgi?id=861093
[2] http://ftp.suse.com/pub/people/mcgrof/xen/emulate-long-xen-hypercalls.patch

Based on original work by: David Vrabel <david.vrabel@citrix.com>
Suggested-by: Andy Lutomirski <luto@amacapital.net>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Borislav Petkov <bp@suse.de>
Cc: David Vrabel <david.vrabel@citrix.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: x86@kernel.org
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Cc: Jan Beulich <JBeulich@suse.com>
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
---
 arch/x86/kernel/entry_32.S       |  2 ++
 arch/x86/kernel/entry_64.S       |  2 ++
 drivers/xen/events/events_base.c | 13 +++++++++++++
 include/xen/events.h             |  1 +
 4 files changed, 18 insertions(+)

diff --git a/arch/x86/kernel/entry_32.S b/arch/x86/kernel/entry_32.S
index 000d419..b4b1f42 100644
--- a/arch/x86/kernel/entry_32.S
+++ b/arch/x86/kernel/entry_32.S
@@ -982,6 +982,8 @@ ENTRY(xen_hypervisor_callback)
 ENTRY(xen_do_upcall)
 1:	mov %esp, %eax
 	call xen_evtchn_do_upcall
+	movl %esp,%eax
+	call xen_end_upcall
 	jmp  ret_from_intr
 	CFI_ENDPROC
 ENDPROC(xen_hypervisor_callback)
diff --git a/arch/x86/kernel/entry_64.S b/arch/x86/kernel/entry_64.S
index 9ebaf63..ee28733 100644
--- a/arch/x86/kernel/entry_64.S
+++ b/arch/x86/kernel/entry_64.S
@@ -1198,6 +1198,8 @@ ENTRY(xen_do_hypervisor_callback)   # do_hypervisor_callback(struct *pt_regs)
 	popq %rsp
 	CFI_DEF_CFA_REGISTER rsp
 	decl PER_CPU_VAR(irq_count)
+	movq %rsp, %rdi  /* pass pt_regs as first argument */
+	call xen_end_upcall
 	jmp  error_exit
 	CFI_ENDPROC
 END(xen_do_hypervisor_callback)
diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c
index b4bca2d..23c526b 100644
--- a/drivers/xen/events/events_base.c
+++ b/drivers/xen/events/events_base.c
@@ -32,6 +32,8 @@
 #include <linux/slab.h>
 #include <linux/irqnr.h>
 #include <linux/pci.h>
+#include <linux/sched.h>
+#include <linux/kprobes.h>
 
 #ifdef CONFIG_X86
 #include <asm/desc.h>
@@ -1243,6 +1245,17 @@ void xen_evtchn_do_upcall(struct pt_regs *regs)
 	set_irq_regs(old_regs);
 }
 
+notrace void xen_end_upcall(struct pt_regs *regs)
+{
+	if (!xen_is_preemptible_hypercall(regs) ||
+	    __this_cpu_read(xed_nesting_count))
+		return;
+
+	if (_cond_resched())
+		printk(KERN_DEBUG "xen hypercall preempted\n");
+}
+NOKPROBE_SYMBOL(xen_end_upcall);
+
 void xen_hvm_evtchn_do_upcall(void)
 {
 	__xen_evtchn_do_upcall();
diff --git a/include/xen/events.h b/include/xen/events.h
index 5321cd9..f08df87 100644
--- a/include/xen/events.h
+++ b/include/xen/events.h
@@ -95,6 +95,7 @@ void xen_hvm_callback_vector(void);
 extern int xen_have_vector_callback;
 int xen_set_callback_via(uint64_t via);
 void xen_evtchn_do_upcall(struct pt_regs *regs);
+void xen_end_upcall(struct pt_regs *regs);
 void xen_hvm_evtchn_do_upcall(void);
 
 /* Bind a pirq for a physical interrupt to an irq. */
-- 
2.1.1

Re: [RFC v3 2/2] x86/xen: allow privcmd hypercalls to be preempted

[Andy Lutomirski]

On Wed, Jan 21, 2015 at 6:17 PM, Luis R. Rodriguez
<mcgrof@do-not-panic.com> wrote:
> From: "Luis R. Rodriguez" <mcgrof@suse.com>
>
> Xen has support for splitting heavy work work into a series
> of hypercalls, called multicalls, and preempting them through
> what Xen calls continuation [0]. Despite this though without
> CONFIG_PREEMPT preemption won't happen, without preemption
> a system can become pretty useless on heavy handed hypercalls.
> Such is the case for example when creating a > 50 GiB HVM guest,
> we can get softlockups [1] with:.
>
> kernel: [  802.084335] BUG: soft lockup - CPU#1 stuck for 22s! [xend:31351]
>
> The softlock up triggers on the TASK_UNINTERRUPTIBLE hanger check
> (default 120 seconds), on the Xen side in this particular case
> this happens when the following Xen hypervisor code is used:
>
> xc_domain_set_pod_target() -->
>   do_memory_op() -->
>     arch_memory_op() -->
>       p2m_pod_set_mem_target()
>         -- long delay (real or emulated) --
>
> This happens on arch_memory_op() on the XENMEM_set_pod_target memory
> op even though arch_memory_op() can handle continuation via
> hypercall_create_continuation() for example.
>
> Machines over 50 GiB of memory are on high demand and hard to come
> by so to help replicate this sort of issue long delays on select
> hypercalls have been emulated in order to be able to test this on
> smaller machines [2].
>
> On one hand this issue can be considered as expected given that
> CONFIG_PREEMPT=n is used however we have forced voluntary preemption
> precedent practices in the kernel even for CONFIG_PREEMPT=n through
> the usage of cond_resched() sprinkled in many places. To address
> this issue with Xen hypercalls though we need to find a way to aid
> to the schedular in the middle of hypercalls. We are motivated to
> address this issue on CONFIG_PREEMPT=n as otherwise the system becomes
> rather unresponsive for long periods of time; in the worst case, at least
> only currently by emulating long delays on select io disk bound
> hypercalls, this can lead to filesystem corruption if the delay happens
> for example on SCHEDOP_remote_shutdown (when we call 'xl <domain> shutdown').
>
> We can address this problem by trying to check if we should schedule
> on the xen timer in the middle of a hypercall on the return from the
> timer interrupt. We want to be careful to not always force voluntary
> preemption though so to do this we only selectively enable preemption
> on very specific xen hypercalls.
>
> This enables hypercall preemption by selectively forcing checks for
> voluntary preempting only on ioctl initiated private hypercalls
> where we know some folks have run into reported issues [1].
>
> [0] http://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=42217cbc5b3e84b8c145d8cfb62dd5de0134b9e8;hp=3a0b9c57d5c9e82c55dd967c84dd06cb43c49ee9
> [1] https://bugzilla.novell.com/show_bug.cgi?id=861093
> [2] http://ftp.suse.com/pub/people/mcgrof/xen/emulate-long-xen-hypercalls.patch
>
> Based on original work by: David Vrabel <david.vrabel@citrix.com>
> Suggested-by: Andy Lutomirski <luto@amacapital.net>
> Cc: Andy Lutomirski <luto@amacapital.net>
> Cc: Borislav Petkov <bp@suse.de>
> Cc: David Vrabel <david.vrabel@citrix.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: x86@kernel.org
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> Cc: Jan Beulich <JBeulich@suse.com>
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
> ---
>  arch/x86/kernel/entry_32.S       |  2 ++
>  arch/x86/kernel/entry_64.S       |  2 ++
>  drivers/xen/events/events_base.c | 13 +++++++++++++
>  include/xen/events.h             |  1 +
>  4 files changed, 18 insertions(+)
>
> diff --git a/arch/x86/kernel/entry_32.S b/arch/x86/kernel/entry_32.S
> index 000d419..b4b1f42 100644
> --- a/arch/x86/kernel/entry_32.S
> +++ b/arch/x86/kernel/entry_32.S
> @@ -982,6 +982,8 @@ ENTRY(xen_hypervisor_callback)
>  ENTRY(xen_do_upcall)
>  1:     mov %esp, %eax
>         call xen_evtchn_do_upcall
> +       movl %esp,%eax
> +       call xen_end_upcall
>         jmp  ret_from_intr
>         CFI_ENDPROC
>  ENDPROC(xen_hypervisor_callback)
> diff --git a/arch/x86/kernel/entry_64.S b/arch/x86/kernel/entry_64.S
> index 9ebaf63..ee28733 100644
> --- a/arch/x86/kernel/entry_64.S
> +++ b/arch/x86/kernel/entry_64.S
> @@ -1198,6 +1198,8 @@ ENTRY(xen_do_hypervisor_callback)   # do_hypervisor_callback(struct *pt_regs)
>         popq %rsp
>         CFI_DEF_CFA_REGISTER rsp
>         decl PER_CPU_VAR(irq_count)
> +       movq %rsp, %rdi  /* pass pt_regs as first argument */
> +       call xen_end_upcall
>         jmp  error_exit
>         CFI_ENDPROC
>  END(xen_do_hypervisor_callback)
> diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c
> index b4bca2d..23c526b 100644
> --- a/drivers/xen/events/events_base.c
> +++ b/drivers/xen/events/events_base.c
> @@ -32,6 +32,8 @@
>  #include <linux/slab.h>
>  #include <linux/irqnr.h>
>  #include <linux/pci.h>
> +#include <linux/sched.h>
> +#include <linux/kprobes.h>
>
>  #ifdef CONFIG_X86
>  #include <asm/desc.h>
> @@ -1243,6 +1245,17 @@ void xen_evtchn_do_upcall(struct pt_regs *regs)
>         set_irq_regs(old_regs);
>  }
>
> +notrace void xen_end_upcall(struct pt_regs *regs)
> +{
> +       if (!xen_is_preemptible_hypercall(regs) ||
> +           __this_cpu_read(xed_nesting_count))
> +               return;

What's xed_nesting_count?

> +
> +       if (_cond_resched())
> +               printk(KERN_DEBUG "xen hypercall preempted\n");

Did you mean to leave this in?  If so, should it be pr_debug?

--Andy

Re: [Xen-devel] [RFC v3 2/2] x86/xen: allow privcmd hypercalls to be preempted

[David Vrabel]

On 22/01/15 03:18, Andy Lutomirski wrote:
>> --- a/drivers/xen/events/events_base.c
>> +++ b/drivers/xen/events/events_base.c
>> @@ -32,6 +32,8 @@
>>  #include <linux/slab.h>
>>  #include <linux/irqnr.h>
>>  #include <linux/pci.h>
>> +#include <linux/sched.h>
>> +#include <linux/kprobes.h>
>>
>>  #ifdef CONFIG_X86
>>  #include <asm/desc.h>
>> @@ -1243,6 +1245,17 @@ void xen_evtchn_do_upcall(struct pt_regs *regs)
>>         set_irq_regs(old_regs);
>>  }
>>
>> +notrace void xen_end_upcall(struct pt_regs *regs)
>> +{
>> +       if (!xen_is_preemptible_hypercall(regs) ||
>> +           __this_cpu_read(xed_nesting_count))
>> +               return;
> 
> What's xed_nesting_count?

It used to prevent nested upcalls when a hypercall called from an upcall
triggers another upcall.

There's no way a such a nested hypercall can be preemptible so the check
cfor xed_nesting_count an be removed from here.

David

Re: [Xen-devel] [RFC v3 2/2] x86/xen: allow privcmd hypercalls to be preempted

[Luis R. Rodriguez]

On Thu, Jan 22, 2015 at 12:55:17PM +0000, David Vrabel wrote:
> On 22/01/15 03:18, Andy Lutomirski wrote:
> >> --- a/drivers/xen/events/events_base.c
> >> +++ b/drivers/xen/events/events_base.c
> >> @@ -32,6 +32,8 @@
> >>  #include <linux/slab.h>
> >>  #include <linux/irqnr.h>
> >>  #include <linux/pci.h>
> >> +#include <linux/sched.h>
> >> +#include <linux/kprobes.h>
> >>
> >>  #ifdef CONFIG_X86
> >>  #include <asm/desc.h>
> >> @@ -1243,6 +1245,17 @@ void xen_evtchn_do_upcall(struct pt_regs *regs)
> >>         set_irq_regs(old_regs);
> >>  }
> >>
> >> +notrace void xen_end_upcall(struct pt_regs *regs)
> >> +{
> >> +       if (!xen_is_preemptible_hypercall(regs) ||
> >> +           __this_cpu_read(xed_nesting_count))
> >> +               return;
> > 
> > What's xed_nesting_count?
> 
> It used to prevent nested upcalls when a hypercall called from an upcall
> triggers another upcall.
> 
> There's no way a such a nested hypercall can be preemptible so the check
> for xed_nesting_count an be removed from here.

Removed.

  Luis

Re: [RFC v3 2/2] x86/xen: allow privcmd hypercalls to be preempted

[Luis R. Rodriguez]

On Wed, Jan 21, 2015 at 07:18:46PM -0800, Andy Lutomirski wrote:
> On Wed, Jan 21, 2015 at 6:17 PM, Luis R. Rodriguez
> <mcgrof@do-not-panic.com> wrote:
> > From: "Luis R. Rodriguez" <mcgrof@suse.com>
> >
> > Xen has support for splitting heavy work work into a series
> > of hypercalls, called multicalls, and preempting them through
> > what Xen calls continuation [0]. Despite this though without
> > CONFIG_PREEMPT preemption won't happen, without preemption
> > a system can become pretty useless on heavy handed hypercalls.
> > Such is the case for example when creating a > 50 GiB HVM guest,
> > we can get softlockups [1] with:.
> >
> > kernel: [  802.084335] BUG: soft lockup - CPU#1 stuck for 22s! [xend:31351]
> >
> > The softlock up triggers on the TASK_UNINTERRUPTIBLE hanger check
> > (default 120 seconds), on the Xen side in this particular case
> > this happens when the following Xen hypervisor code is used:
> >
> > xc_domain_set_pod_target() -->
> >   do_memory_op() -->
> >     arch_memory_op() -->
> >       p2m_pod_set_mem_target()
> >         -- long delay (real or emulated) --
> >
> > This happens on arch_memory_op() on the XENMEM_set_pod_target memory
> > op even though arch_memory_op() can handle continuation via
> > hypercall_create_continuation() for example.
> >
> > Machines over 50 GiB of memory are on high demand and hard to come
> > by so to help replicate this sort of issue long delays on select
> > hypercalls have been emulated in order to be able to test this on
> > smaller machines [2].
> >
> > On one hand this issue can be considered as expected given that
> > CONFIG_PREEMPT=n is used however we have forced voluntary preemption
> > precedent practices in the kernel even for CONFIG_PREEMPT=n through
> > the usage of cond_resched() sprinkled in many places. To address
> > this issue with Xen hypercalls though we need to find a way to aid
> > to the schedular in the middle of hypercalls. We are motivated to
> > address this issue on CONFIG_PREEMPT=n as otherwise the system becomes
> > rather unresponsive for long periods of time; in the worst case, at least
> > only currently by emulating long delays on select io disk bound
> > hypercalls, this can lead to filesystem corruption if the delay happens
> > for example on SCHEDOP_remote_shutdown (when we call 'xl <domain> shutdown').
> >
> > We can address this problem by trying to check if we should schedule
> > on the xen timer in the middle of a hypercall on the return from the
> > timer interrupt. We want to be careful to not always force voluntary
> > preemption though so to do this we only selectively enable preemption
> > on very specific xen hypercalls.
> >
> > This enables hypercall preemption by selectively forcing checks for
> > voluntary preempting only on ioctl initiated private hypercalls
> > where we know some folks have run into reported issues [1].
> >
> > [0] http://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=42217cbc5b3e84b8c145d8cfb62dd5de0134b9e8;hp=3a0b9c57d5c9e82c55dd967c84dd06cb43c49ee9
> > [1] https://bugzilla.novell.com/show_bug.cgi?id=861093
> > [2] http://ftp.suse.com/pub/people/mcgrof/xen/emulate-long-xen-hypercalls.patch
> >
> > Based on original work by: David Vrabel <david.vrabel@citrix.com>
> > Suggested-by: Andy Lutomirski <luto@amacapital.net>
> > Cc: Andy Lutomirski <luto@amacapital.net>
> > Cc: Borislav Petkov <bp@suse.de>
> > Cc: David Vrabel <david.vrabel@citrix.com>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Ingo Molnar <mingo@redhat.com>
> > Cc: "H. Peter Anvin" <hpa@zytor.com>
> > Cc: x86@kernel.org
> > Cc: Steven Rostedt <rostedt@goodmis.org>
> > Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> > Cc: Jan Beulich <JBeulich@suse.com>
> > Cc: linux-kernel@vger.kernel.org
> > Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
> > ---
> >  arch/x86/kernel/entry_32.S       |  2 ++
> >  arch/x86/kernel/entry_64.S       |  2 ++
> >  drivers/xen/events/events_base.c | 13 +++++++++++++
> >  include/xen/events.h             |  1 +
> >  4 files changed, 18 insertions(+)
> >
> > diff --git a/arch/x86/kernel/entry_32.S b/arch/x86/kernel/entry_32.S
> > index 000d419..b4b1f42 100644
> > --- a/arch/x86/kernel/entry_32.S
> > +++ b/arch/x86/kernel/entry_32.S
> > @@ -982,6 +982,8 @@ ENTRY(xen_hypervisor_callback)
> >  ENTRY(xen_do_upcall)
> >  1:     mov %esp, %eax
> >         call xen_evtchn_do_upcall
> > +       movl %esp,%eax
> > +       call xen_end_upcall
> >         jmp  ret_from_intr
> >         CFI_ENDPROC
> >  ENDPROC(xen_hypervisor_callback)
> > diff --git a/arch/x86/kernel/entry_64.S b/arch/x86/kernel/entry_64.S
> > index 9ebaf63..ee28733 100644
> > --- a/arch/x86/kernel/entry_64.S
> > +++ b/arch/x86/kernel/entry_64.S
> > @@ -1198,6 +1198,8 @@ ENTRY(xen_do_hypervisor_callback)   # do_hypervisor_callback(struct *pt_regs)
> >         popq %rsp
> >         CFI_DEF_CFA_REGISTER rsp
> >         decl PER_CPU_VAR(irq_count)
> > +       movq %rsp, %rdi  /* pass pt_regs as first argument */
> > +       call xen_end_upcall
> >         jmp  error_exit
> >         CFI_ENDPROC
> >  END(xen_do_hypervisor_callback)
> > diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c
> > index b4bca2d..23c526b 100644
> > --- a/drivers/xen/events/events_base.c
> > +++ b/drivers/xen/events/events_base.c
> > @@ -32,6 +32,8 @@
> >  #include <linux/slab.h>
> >  #include <linux/irqnr.h>
> >  #include <linux/pci.h>
> > +#include <linux/sched.h>
> > +#include <linux/kprobes.h>
> >
> >  #ifdef CONFIG_X86
> >  #include <asm/desc.h>
> > @@ -1243,6 +1245,17 @@ void xen_evtchn_do_upcall(struct pt_regs *regs)
> >         set_irq_regs(old_regs);
> >  }
> >
> > +notrace void xen_end_upcall(struct pt_regs *regs)
> > +{
> > +       if (!xen_is_preemptible_hypercall(regs) ||
> > +           __this_cpu_read(xed_nesting_count))
> > +               return;
> 
> What's xed_nesting_count?

I'll nuke its use here as per David.

> > +
> > +       if (_cond_resched())
> > +               printk(KERN_DEBUG "xen hypercall preempted\n");
> 
> Did you mean to leave this in?  If so, should it be pr_debug?

Nuking as well.

 Luis

Re: [Xen-devel] [RFC v3 2/2] x86/xen: allow privcmd hypercalls to be preempted

[Andrew Cooper]

On 22/01/15 02:17, Luis R. Rodriguez wrote:
> --- a/drivers/xen/events/events_base.c
> +++ b/drivers/xen/events/events_base.c
> @@ -32,6 +32,8 @@
>  #include <linux/slab.h>
>  #include <linux/irqnr.h>
>  #include <linux/pci.h>
> +#include <linux/sched.h>
> +#include <linux/kprobes.h>
>  
>  #ifdef CONFIG_X86
>  #include <asm/desc.h>
> @@ -1243,6 +1245,17 @@ void xen_evtchn_do_upcall(struct pt_regs *regs)
>  	set_irq_regs(old_regs);
>  }
>  
> +notrace void xen_end_upcall(struct pt_regs *regs)
> +{
> +	if (!xen_is_preemptible_hypercall(regs) ||
> +	    __this_cpu_read(xed_nesting_count))
> +		return;
> +
> +	if (_cond_resched())
> +		printk(KERN_DEBUG "xen hypercall preempted\n");

I wouldn't even put this at debug level.  On a large server with plenty
of domains being created/migrated/destroyed, it is quite likely that a
toolstack task might get preempted in this way.

I don't believe the message is of any practical use.

~Andrew

Re: [Xen-devel] [RFC v3 2/2] x86/xen: allow privcmd hypercalls to be preempted

[Steven Rostedt]

On Thu, 22 Jan 2015 11:50:10 +0000
Andrew Cooper <andrew.cooper3@citrix.com> wrote:

> On 22/01/15 02:17, Luis R. Rodriguez wrote:
> > --- a/drivers/xen/events/events_base.c
> > +++ b/drivers/xen/events/events_base.c
> > @@ -32,6 +32,8 @@
> >  #include <linux/slab.h>
> >  #include <linux/irqnr.h>
> >  #include <linux/pci.h>
> > +#include <linux/sched.h>
> > +#include <linux/kprobes.h>
> >  
> >  #ifdef CONFIG_X86
> >  #include <asm/desc.h>
> > @@ -1243,6 +1245,17 @@ void xen_evtchn_do_upcall(struct pt_regs
> > *regs) set_irq_regs(old_regs);
> >  }
> >  
> > +notrace void xen_end_upcall(struct pt_regs *regs)
> > +{
> > +	if (!xen_is_preemptible_hypercall(regs) ||
> > +	    __this_cpu_read(xed_nesting_count))
> > +		return;
> > +
> > +	if (_cond_resched())
> > +		printk(KERN_DEBUG "xen hypercall preempted\n");
> 
> I wouldn't even put this at debug level.  On a large server with
> plenty of domains being created/migrated/destroyed, it is quite
> likely that a toolstack task might get preempted in this way.
> 
> I don't believe the message is of any practical use.
> 

Why not make this a tracepoint? Then you can enable it only when you
want to. As tracepoints are also hooks, you could add you own code that
hooks to it and does a printk as well. The advantage of doing it via a
tracepoint is that you can turn it on and off regardless of what the
loglevel is set at.

That is, if there is any practical use for that message. Tracing just
sched_switch will give you the same info.

-- Steve

Re: [Xen-devel] [RFC v3 2/2] x86/xen: allow privcmd hypercalls to be preempted

[Luis R. Rodriguez]

On Thu, Jan 22, 2015 at 08:56:49AM -0500, Steven Rostedt wrote:
> On Thu, 22 Jan 2015 11:50:10 +0000
> Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> 
> > On 22/01/15 02:17, Luis R. Rodriguez wrote:
> > > --- a/drivers/xen/events/events_base.c
> > > +++ b/drivers/xen/events/events_base.c
> > > @@ -32,6 +32,8 @@
> > >  #include <linux/slab.h>
> > >  #include <linux/irqnr.h>
> > >  #include <linux/pci.h>
> > > +#include <linux/sched.h>
> > > +#include <linux/kprobes.h>
> > >  
> > >  #ifdef CONFIG_X86
> > >  #include <asm/desc.h>
> > > @@ -1243,6 +1245,17 @@ void xen_evtchn_do_upcall(struct pt_regs
> > > *regs) set_irq_regs(old_regs);
> > >  }
> > >  
> > > +notrace void xen_end_upcall(struct pt_regs *regs)
> > > +{
> > > +	if (!xen_is_preemptible_hypercall(regs) ||
> > > +	    __this_cpu_read(xed_nesting_count))
> > > +		return;
> > > +
> > > +	if (_cond_resched())
> > > +		printk(KERN_DEBUG "xen hypercall preempted\n");
> > 
> > I wouldn't even put this at debug level.  On a large server with
> > plenty of domains being created/migrated/destroyed, it is quite
> > likely that a toolstack task might get preempted in this way.
> > 
> > I don't believe the message is of any practical use.
> > 
> 
> Why not make this a tracepoint? Then you can enable it only when you
> want to. As tracepoints are also hooks, you could add you own code that
> hooks to it and does a printk as well. The advantage of doing it via a
> tracepoint is that you can turn it on and off regardless of what the
> loglevel is set at.

This uses NOKPROBE_SYMBOL and notrace since based on Andy's advice
we are not confident that tracing and kprobes are safe to use in what
might be an extended RCU quiescent state (i.e. where we're outside
irq_enter and irq_exit).

> That is, if there is any practical use for that message. Tracing just
> sched_switch will give you the same info.

IMHO it may be more useful if we knew exactly what hypercalls were
being preempted but perhaps all that can be left as a secondary
exercise and for now I'll just nuke the print.

  Luis

Re: [Xen-devel] [RFC v3 2/2] x86/xen: allow privcmd hypercalls to be preempted

[Steven Rostedt]

[ Added Paul McKenney ]

On Thu, 22 Jan 2015 19:39:13 +0100
"Luis R. Rodriguez" <mcgrof@suse.com> wrote:

> > Why not make this a tracepoint? Then you can enable it only when you
> > want to. As tracepoints are also hooks, you could add you own code that
> > hooks to it and does a printk as well. The advantage of doing it via a
> > tracepoint is that you can turn it on and off regardless of what the
> > loglevel is set at.
> 
> This uses NOKPROBE_SYMBOL and notrace since based on Andy's advice
> we are not confident that tracing and kprobes are safe to use in what
> might be an extended RCU quiescent state (i.e. where we're outside
> irq_enter and irq_exit).

We have trace_*_rcuidle() for such cases.

That is, you create the tracepoint just the same, and instead of having
trace_foo(), if you are in a known area that is outside of rcu viewing,
you use trace_foo_rcuidle() and it will tell RCU "hey, there's something
here that may need RCU, so look at me!"

Also, please remove the "notrace", because function tracing goes an
extra step to not require RCU being visible. The only thing you get
with notrace is not being able to trace an otherwise traceable function.

-- Steve

> 
> > That is, if there is any practical use for that message. Tracing just
> > sched_switch will give you the same info.
> 
> IMHO it may be more useful if we knew exactly what hypercalls were
> being preempted but perhaps all that can be left as a secondary
> exercise and for now I'll just nuke the print.

Re: [Xen-devel] [RFC v3 2/2] x86/xen: allow privcmd hypercalls to be preempted

[Andy Lutomirski]

On Thu, Jan 22, 2015 at 12:16 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
>
> [ Added Paul McKenney ]
>
> On Thu, 22 Jan 2015 19:39:13 +0100
> "Luis R. Rodriguez" <mcgrof@suse.com> wrote:
>
>> > Why not make this a tracepoint? Then you can enable it only when you
>> > want to. As tracepoints are also hooks, you could add you own code that
>> > hooks to it and does a printk as well. The advantage of doing it via a
>> > tracepoint is that you can turn it on and off regardless of what the
>> > loglevel is set at.
>>
>> This uses NOKPROBE_SYMBOL and notrace since based on Andy's advice
>> we are not confident that tracing and kprobes are safe to use in what
>> might be an extended RCU quiescent state (i.e. where we're outside
>> irq_enter and irq_exit).
>
> We have trace_*_rcuidle() for such cases.
>
> That is, you create the tracepoint just the same, and instead of having
> trace_foo(), if you are in a known area that is outside of rcu viewing,
> you use trace_foo_rcuidle() and it will tell RCU "hey, there's something
> here that may need RCU, so look at me!"
>
> Also, please remove the "notrace", because function tracing goes an
> extra step to not require RCU being visible. The only thing you get
> with notrace is not being able to trace an otherwise traceable function.
>

Is this also true for kprobes?  And can kprobes nest inside function
tracing hooks?

The other issue, above and beyond RCU, is that we can't let kprobes
run on the int3 stack.  If Xen upcalls can happen when interrupts are
off, then we may need this protection to prevent that type of
recursion.  (This will be much less scary in 3.20, because userspace
int3 instructions will no longer execute on the int3 stack.)

--Andy

> -- Steve
>
>>
>> > That is, if there is any practical use for that message. Tracing just
>> > sched_switch will give you the same info.
>>
>> IMHO it may be more useful if we knew exactly what hypercalls were
>> being preempted but perhaps all that can be left as a secondary
>> exercise and for now I'll just nuke the print.



-- 
Andy Lutomirski
AMA Capital Management, LLC

Re: [Xen-devel] [RFC v3 2/2] x86/xen: allow privcmd hypercalls to be preempted

[Steven Rostedt]

On Thu, 22 Jan 2015 12:24:47 -0800
Andy Lutomirski <luto@amacapital.net> wrote:

> > Also, please remove the "notrace", because function tracing goes an
> > extra step to not require RCU being visible. The only thing you get
> > with notrace is not being able to trace an otherwise traceable function.
> >
> 
> Is this also true for kprobes?  And can kprobes nest inside function
> tracing hooks?

No, kprobes are a bit more fragile than function tracing or tracepoints.

And nothing should nest inside a function hook (except for interrupts,
they are fine).

> 
> The other issue, above and beyond RCU, is that we can't let kprobes
> run on the int3 stack.  If Xen upcalls can happen when interrupts are
> off, then we may need this protection to prevent that type of
> recursion.  (This will be much less scary in 3.20, because userspace
> int3 instructions will no longer execute on the int3 stack.)

Does this execute between the start of the int3 interrupt handler and
the call of do_int3()?

-- Steve

Re: [Xen-devel] [RFC v3 2/2] x86/xen: allow privcmd hypercalls to be preempted

[Andy Lutomirski]

On Thu, Jan 22, 2015 at 12:37 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Thu, 22 Jan 2015 12:24:47 -0800
> Andy Lutomirski <luto@amacapital.net> wrote:
>
>> > Also, please remove the "notrace", because function tracing goes an
>> > extra step to not require RCU being visible. The only thing you get
>> > with notrace is not being able to trace an otherwise traceable function.
>> >
>>
>> Is this also true for kprobes?  And can kprobes nest inside function
>> tracing hooks?
>
> No, kprobes are a bit more fragile than function tracing or tracepoints.
>
> And nothing should nest inside a function hook (except for interrupts,
> they are fine).
>

But kprobes do nest inside interrupts, right?

>>
>> The other issue, above and beyond RCU, is that we can't let kprobes
>> run on the int3 stack.  If Xen upcalls can happen when interrupts are
>> off, then we may need this protection to prevent that type of
>> recursion.  (This will be much less scary in 3.20, because userspace
>> int3 instructions will no longer execute on the int3 stack.)
>
> Does this execute between the start of the int3 interrupt handler and
> the call of do_int3()?

I doubt it.

The thing I worry about is that, if do_int3 nests inside itself by any
means (e.g. int3 sends a signal, scheduling for whatever reason
(really shouldn't happen, but I haven't looked that hard)), then we're
completely hosed -- the inner int3 will overwrite the outer int3's
stack frame.  Since I have no idea what Xen upcalls do, I don't know
whether they can fire inside do_int3.

--Andy

Re: [Xen-devel] [RFC v3 2/2] x86/xen: allow privcmd hypercalls to be preempted

[Steven Rostedt]

On Thu, 22 Jan 2015 12:58:00 -0800
Andy Lutomirski <luto@amacapital.net> wrote:

> On Thu, Jan 22, 2015 at 12:37 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> > On Thu, 22 Jan 2015 12:24:47 -0800
> > Andy Lutomirski <luto@amacapital.net> wrote:
> >
> >> > Also, please remove the "notrace", because function tracing goes an
> >> > extra step to not require RCU being visible. The only thing you get
> >> > with notrace is not being able to trace an otherwise traceable function.
> >> >
> >>
> >> Is this also true for kprobes?  And can kprobes nest inside function
> >> tracing hooks?
> >
> > No, kprobes are a bit more fragile than function tracing or tracepoints.
> >
> > And nothing should nest inside a function hook (except for interrupts,
> > they are fine).
> >
> 
> But kprobes do nest inside interrupts, right?

A kprobe being called while a function trace is happening is fine, but
you should not have the kprobe set directly inside the function trace
callback code. Because that means a kprobe could happen anywhere
function tracing is happening (for instance, in NMI context).

> 
> >>
> >> The other issue, above and beyond RCU, is that we can't let kprobes
> >> run on the int3 stack.  If Xen upcalls can happen when interrupts are
> >> off, then we may need this protection to prevent that type of
> >> recursion.  (This will be much less scary in 3.20, because userspace
> >> int3 instructions will no longer execute on the int3 stack.)
> >
> > Does this execute between the start of the int3 interrupt handler and
> > the call of do_int3()?
> 
> I doubt it.
> 
> The thing I worry about is that, if do_int3 nests inside itself by any
> means (e.g. int3 sends a signal, scheduling for whatever reason
> (really shouldn't happen, but I haven't looked that hard)), then we're
> completely hosed -- the inner int3 will overwrite the outer int3's
> stack frame.  Since I have no idea what Xen upcalls do, I don't know
> whether they can fire inside do_int3.

I thought there's logic in the do_int3 handler (in the assembly code)
that can handle nested int3s.

I'm not sure what xen does though.

-- Steve

Re: [Xen-devel] [RFC v3 2/2] x86/xen: allow privcmd hypercalls to be preempted

[Andy Lutomirski]

On Thu, Jan 22, 2015 at 1:16 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Thu, 22 Jan 2015 12:58:00 -0800
> Andy Lutomirski <luto@amacapital.net> wrote:
>
>> On Thu, Jan 22, 2015 at 12:37 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
>> > On Thu, 22 Jan 2015 12:24:47 -0800
>> > Andy Lutomirski <luto@amacapital.net> wrote:
>>
>> >>
>> >> The other issue, above and beyond RCU, is that we can't let kprobes
>> >> run on the int3 stack.  If Xen upcalls can happen when interrupts are
>> >> off, then we may need this protection to prevent that type of
>> >> recursion.  (This will be much less scary in 3.20, because userspace
>> >> int3 instructions will no longer execute on the int3 stack.)
>> >
>> > Does this execute between the start of the int3 interrupt handler and
>> > the call of do_int3()?
>>
>> I doubt it.
>>
>> The thing I worry about is that, if do_int3 nests inside itself by any
>> means (e.g. int3 sends a signal, scheduling for whatever reason
>> (really shouldn't happen, but I haven't looked that hard)), then we're
>> completely hosed -- the inner int3 will overwrite the outer int3's
>> stack frame.  Since I have no idea what Xen upcalls do, I don't know
>> whether they can fire inside do_int3.
>
> I thought there's logic in the do_int3 handler (in the assembly code)
> that can handle nested int3s.

Nope :(

In 3.20, there's likely to be logic that can handle a single level of
nesting as long as the outer one came from user space.

--Andy

Re: [Xen-devel] [RFC v3 2/2] x86/xen: allow privcmd hypercalls to be preempted

[Andrew Cooper]

On 22/01/2015 20:58, Andy Lutomirski wrote:
> On Thu, Jan 22, 2015 at 12:37 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
>> On Thu, 22 Jan 2015 12:24:47 -0800
>> Andy Lutomirski <luto@amacapital.net> wrote:
>>
>>>> Also, please remove the "notrace", because function tracing goes an
>>>> extra step to not require RCU being visible. The only thing you get
>>>> with notrace is not being able to trace an otherwise traceable function.
>>>>
>>> Is this also true for kprobes?  And can kprobes nest inside function
>>> tracing hooks?
>> No, kprobes are a bit more fragile than function tracing or tracepoints.
>>
>> And nothing should nest inside a function hook (except for interrupts,
>> they are fine).
>>
> But kprobes do nest inside interrupts, right?
>
>>> The other issue, above and beyond RCU, is that we can't let kprobes
>>> run on the int3 stack.  If Xen upcalls can happen when interrupts are
>>> off, then we may need this protection to prevent that type of
>>> recursion.  (This will be much less scary in 3.20, because userspace
>>> int3 instructions will no longer execute on the int3 stack.)
>> Does this execute between the start of the int3 interrupt handler and
>> the call of do_int3()?
> I doubt it.
>
> The thing I worry about is that, if do_int3 nests inside itself by any
> means (e.g. int3 sends a signal, scheduling for whatever reason
> (really shouldn't happen, but I haven't looked that hard)), then we're
> completely hosed -- the inner int3 will overwrite the outer int3's
> stack frame.  Since I have no idea what Xen upcalls do, I don't know
> whether they can fire inside do_int3.

The upcall is the "you have a virtual interrupt pending" signal and
should behave exactly like an external interrupt.  The exception frame
will appear to have interrupted the correct vcpu context, despite actual
trip via Xen.

Exceptions are handled as per native, with the xen_write_idt_entry()
PVOP taking care of registering the entry point with Xen, rather than
filling in a real IDT entry.

~Andrew

Re: [Xen-devel] [RFC v3 2/2] x86/xen: allow privcmd hypercalls to be preempted

[Paul E. McKenney]

On Thu, Jan 22, 2015 at 03:16:57PM -0500, Steven Rostedt wrote:
> 
> [ Added Paul McKenney ]
> 
> On Thu, 22 Jan 2015 19:39:13 +0100
> "Luis R. Rodriguez" <mcgrof@suse.com> wrote:
> 
> > > Why not make this a tracepoint? Then you can enable it only when you
> > > want to. As tracepoints are also hooks, you could add you own code that
> > > hooks to it and does a printk as well. The advantage of doing it via a
> > > tracepoint is that you can turn it on and off regardless of what the
> > > loglevel is set at.
> > 
> > This uses NOKPROBE_SYMBOL and notrace since based on Andy's advice
> > we are not confident that tracing and kprobes are safe to use in what
> > might be an extended RCU quiescent state (i.e. where we're outside
> > irq_enter and irq_exit).
> 
> We have trace_*_rcuidle() for such cases.
> 
> That is, you create the tracepoint just the same, and instead of having
> trace_foo(), if you are in a known area that is outside of rcu viewing,
> you use trace_foo_rcuidle() and it will tell RCU "hey, there's something
> here that may need RCU, so look at me!"

What Steve said!

Also, there is an rcu_is_watching() API member that can tell you
whether or not RCU is paying attention at a given point.  Or test with
CONFIG_PROVE_RCU, in which case lockdep will yell at you if you should
have used the _rcuidle() form of the tracing hooks.  ;-)

							Thanx, Paul

> Also, please remove the "notrace", because function tracing goes an
> extra step to not require RCU being visible. The only thing you get
> with notrace is not being able to trace an otherwise traceable function.
> 
> -- Steve
> 
> > 
> > > That is, if there is any practical use for that message. Tracing just
> > > sched_switch will give you the same info.
> > 
> > IMHO it may be more useful if we knew exactly what hypercalls were
> > being preempted but perhaps all that can be left as a secondary
> > exercise and for now I'll just nuke the print.
> 

Re: [Xen-devel] [RFC v3 2/2] x86/xen: allow privcmd hypercalls to be preempted

[Luis R. Rodriguez]

On Thu, Jan 22, 2015 at 11:50:10AM +0000, Andrew Cooper wrote:
> On 22/01/15 02:17, Luis R. Rodriguez wrote:
> > --- a/drivers/xen/events/events_base.c
> > +++ b/drivers/xen/events/events_base.c
> > @@ -32,6 +32,8 @@
> >  #include <linux/slab.h>
> >  #include <linux/irqnr.h>
> >  #include <linux/pci.h>
> > +#include <linux/sched.h>
> > +#include <linux/kprobes.h>
> >  
> >  #ifdef CONFIG_X86
> >  #include <asm/desc.h>
> > @@ -1243,6 +1245,17 @@ void xen_evtchn_do_upcall(struct pt_regs *regs)
> >  	set_irq_regs(old_regs);
> >  }
> >  
> > +notrace void xen_end_upcall(struct pt_regs *regs)
> > +{
> > +	if (!xen_is_preemptible_hypercall(regs) ||
> > +	    __this_cpu_read(xed_nesting_count))
> > +		return;
> > +
> > +	if (_cond_resched())
> > +		printk(KERN_DEBUG "xen hypercall preempted\n");
> 
> I wouldn't even put this at debug level.  On a large server with plenty
> of domains being created/migrated/destroyed, it is quite likely that a
> toolstack task might get preempted in this way.
> 
> I don't believe the message is of any practical use.

I'll just nuke it then.

 Luis

Re: [Xen-devel] [RFC v3 2/2] x86/xen: allow privcmd hypercalls to be preempted

[Julien Grall]

Hi Luis,

On 22/01/15 02:17, Luis R. Rodriguez wrote:
> diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c
> index b4bca2d..23c526b 100644
> --- a/drivers/xen/events/events_base.c
> +++ b/drivers/xen/events/events_base.c
> @@ -32,6 +32,8 @@
>  #include <linux/slab.h>
>  #include <linux/irqnr.h>
>  #include <linux/pci.h>
> +#include <linux/sched.h>
> +#include <linux/kprobes.h>
>  
>  #ifdef CONFIG_X86
>  #include <asm/desc.h>
> @@ -1243,6 +1245,17 @@ void xen_evtchn_do_upcall(struct pt_regs *regs)
>  	set_irq_regs(old_regs);
>  }
>  
> +notrace void xen_end_upcall(struct pt_regs *regs)
> +{
> +	if (!xen_is_preemptible_hypercall(regs) ||

I don't see any definition of xen_is_preemptible_hypercall for ARM32/ARM64.

As this function is called from the generic code, you have at least to
stub this function for those architectures.

Regards,

-- 
Julien Grall

Re: [Xen-devel] [RFC v3 2/2] x86/xen: allow privcmd hypercalls to be preempted

[Luis R. Rodriguez]

On Thu, Jan 22, 2015 at 01:10:49PM +0000, Julien Grall wrote:
> Hi Luis,
> 
> On 22/01/15 02:17, Luis R. Rodriguez wrote:
> > diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c
> > index b4bca2d..23c526b 100644
> > --- a/drivers/xen/events/events_base.c
> > +++ b/drivers/xen/events/events_base.c
> > @@ -32,6 +32,8 @@
> >  #include <linux/slab.h>
> >  #include <linux/irqnr.h>
> >  #include <linux/pci.h>
> > +#include <linux/sched.h>
> > +#include <linux/kprobes.h>
> >  
> >  #ifdef CONFIG_X86
> >  #include <asm/desc.h>
> > @@ -1243,6 +1245,17 @@ void xen_evtchn_do_upcall(struct pt_regs *regs)
> >  	set_irq_regs(old_regs);
> >  }
> >  
> > +notrace void xen_end_upcall(struct pt_regs *regs)
> > +{
> > +	if (!xen_is_preemptible_hypercall(regs) ||
> 
> I don't see any definition of xen_is_preemptible_hypercall for ARM32/ARM64.
> 
> As this function is called from the generic code, you have at least to
> stub this function for those architectures.

Will add as:

diff --git a/arch/arm/include/asm/xen/hypercall.h b/arch/arm/include/asm/xen/hypercall.h
index 712b50e..4fc8395 100644
--- a/arch/arm/include/asm/xen/hypercall.h
+++ b/arch/arm/include/asm/xen/hypercall.h
@@ -74,4 +74,9 @@ MULTI_mmu_update(struct multicall_entry *mcl, struct mmu_update *req,
 	BUG();
 }
 
+static inline bool xen_is_preemptible_hypercall(struct pt_regs *regs)
+{
+	return false;
+}
+
 #endif /* _ASM_ARM_XEN_HYPERCALL_H */

This will cover both arm and arm64 as arm64 includes the arm header.

  Luis

Re: [Xen-devel] [RFC v3 2/2] x86/xen: allow privcmd hypercalls to be preempted

[Julien Grall]

On 22/01/15 18:56, Luis R. Rodriguez wrote:
> On Thu, Jan 22, 2015 at 01:10:49PM +0000, Julien Grall wrote:
>> Hi Luis,
>>
>> On 22/01/15 02:17, Luis R. Rodriguez wrote:
>>> diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c
>>> index b4bca2d..23c526b 100644
>>> --- a/drivers/xen/events/events_base.c
>>> +++ b/drivers/xen/events/events_base.c
>>> @@ -32,6 +32,8 @@
>>>  #include <linux/slab.h>
>>>  #include <linux/irqnr.h>
>>>  #include <linux/pci.h>
>>> +#include <linux/sched.h>
>>> +#include <linux/kprobes.h>
>>>  
>>>  #ifdef CONFIG_X86
>>>  #include <asm/desc.h>
>>> @@ -1243,6 +1245,17 @@ void xen_evtchn_do_upcall(struct pt_regs *regs)
>>>  	set_irq_regs(old_regs);
>>>  }
>>>  
>>> +notrace void xen_end_upcall(struct pt_regs *regs)
>>> +{
>>> +	if (!xen_is_preemptible_hypercall(regs) ||
>>
>> I don't see any definition of xen_is_preemptible_hypercall for ARM32/ARM64.
>>
>> As this function is called from the generic code, you have at least to
>> stub this function for those architectures.
> 
> Will add as:
> 
> diff --git a/arch/arm/include/asm/xen/hypercall.h b/arch/arm/include/asm/xen/hypercall.h
> index 712b50e..4fc8395 100644
> --- a/arch/arm/include/asm/xen/hypercall.h
> +++ b/arch/arm/include/asm/xen/hypercall.h
> @@ -74,4 +74,9 @@ MULTI_mmu_update(struct multicall_entry *mcl, struct mmu_update *req,
>  	BUG();
>  }
>  
> +static inline bool xen_is_preemptible_hypercall(struct pt_regs *regs)
> +{
> +	return false;
> +}
> +
>  #endif /* _ASM_ARM_XEN_HYPERCALL_H */
> 
> This will cover both arm and arm64 as arm64 includes the arm header.

I'm fine with this solution.

Regards,

-- 
Julien Grall