[PATCHv5] x86/xen: allow privcmd hypercalls to be preempted

[PATCHv5] x86/xen: allow privcmd hypercalls to be preempted

[David Vrabel]

Hypercalls submitted by user space tools via the privcmd driver can
take a long time (potentially many 10s of seconds) if the hypercall
has many sub-operations.

A fully preemptible kernel may deschedule such as task in any upcall
called from a hypercall continuation.

However, in a kernel with voluntary or no preemption, hypercall
continuations in Xen allow event handlers to be run but the task
issuing the hypercall will not be descheduled until the hypercall is
complete and the ioctl returns to user space.  These long running
tasks may also trigger the kernel's soft lockup detection.

Add xen_preemptible_hcall_begin() and xen_preemptible_hcall_end() to
bracket hypercalls that may be preempted.  Use these in the privcmd
driver.

When returning from an upcall, call xen_maybe_preempt_hcall() which
adds a schedule point if if the current task was within a preemptible
hypercall.

Since preempt_schedule_irq() can move the task to a different CPU,
clear and set xen_in_preemptible_hcall around the call.

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
Changes in v5:
- Add xen_maybe_preempt_hcall() instead of copy-and-paste of assembly.

Changes in v4:
- Fix 32-bit build.

Changes in v3:
- Export xen_in_preemptible_hcall (to fix modular privcmd driver).

Changes in v2:
- Use per-cpu variable to mark preemptible regions
- Call preempt_schedule_irq() from the correct place in
  xen_hypervisor_callback
---
 arch/x86/kernel/entry_32.S |    3 +++
 arch/x86/kernel/entry_64.S |    3 +++
 drivers/xen/Makefile       |    2 +-
 drivers/xen/preempt.c      |   42 ++++++++++++++++++++++++++++++++++++++++++
 drivers/xen/privcmd.c      |    2 ++
 include/xen/xen-ops.h      |   26 ++++++++++++++++++++++++++
 6 files changed, 77 insertions(+), 1 deletion(-)
 create mode 100644 drivers/xen/preempt.c

diff --git a/arch/x86/kernel/entry_32.S b/arch/x86/kernel/entry_32.S
index 000d419..31e2d5b 100644
--- a/arch/x86/kernel/entry_32.S
+++ b/arch/x86/kernel/entry_32.S
@@ -982,6 +982,9 @@ ENTRY(xen_hypervisor_callback)
 ENTRY(xen_do_upcall)
 1:	mov %esp, %eax
 	call xen_evtchn_do_upcall
+#ifndef CONFIG_PREEMPT
+	call xen_maybe_preempt_hcall
+#endif
 	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..9069401 100644
--- a/arch/x86/kernel/entry_64.S
+++ b/arch/x86/kernel/entry_64.S
@@ -1198,6 +1198,9 @@ ENTRY(xen_do_hypervisor_callback)   # do_hypervisor_callback(struct *pt_regs)
 	popq %rsp
 	CFI_DEF_CFA_REGISTER rsp
 	decl PER_CPU_VAR(irq_count)
+#ifndef CONFIG_PREEMPT
+	call xen_maybe_preempt_hcall
+#endif
 	jmp  error_exit
 	CFI_ENDPROC
 END(xen_do_hypervisor_callback)
diff --git a/drivers/xen/Makefile b/drivers/xen/Makefile
index 2140398..2ccd359 100644
--- a/drivers/xen/Makefile
+++ b/drivers/xen/Makefile
@@ -2,7 +2,7 @@ ifeq ($(filter y, $(CONFIG_ARM) $(CONFIG_ARM64)),)
 obj-$(CONFIG_HOTPLUG_CPU)		+= cpu_hotplug.o
 endif
 obj-$(CONFIG_X86)			+= fallback.o
-obj-y	+= grant-table.o features.o balloon.o manage.o
+obj-y	+= grant-table.o features.o balloon.o manage.o preempt.o
 obj-y	+= events/
 obj-y	+= xenbus/
 
diff --git a/drivers/xen/preempt.c b/drivers/xen/preempt.c
new file mode 100644
index 0000000..e2bcc01
--- /dev/null
+++ b/drivers/xen/preempt.c
@@ -0,0 +1,42 @@
+/*
+ * Preemptible hypercalls
+ *
+ * Copyright (C) 2014 Citrix Systems R&D ltd.
+ *
+ * This source code is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of the
+ * License, or (at your option) any later version.
+ */
+
+#include <xen/xen-ops.h>
+
+#ifndef CONFIG_PREEMPT
+
+/*
+ * Some hypercalls issued by the toolstack can take many 10s of
+ * seconds. Allow tasks running hypercalls via the privcmd driver to
+ * be voluntarily preempted even if full kernel preemption is
+ * disabled.
+ *
+ * Such preemptible hypercalls are bracketed by
+ * xen_preemptible_hcall_begin() and xen_preemptible_hcall_end()
+ * calls.
+ */
+
+DEFINE_PER_CPU(bool, xen_in_preemptible_hcall);
+EXPORT_SYMBOL_GPL(xen_in_preemptible_hcall);
+
+void xen_maybe_preempt_hcall(void)
+{
+	if (__this_cpu_read(xen_in_preemptible_hcall)) {
+		/*
+		 * Clear flag as we may be rescheduled on a different
+		 * cpu.
+		 */
+		__this_cpu_write(xen_in_preemptible_hcall, false);
+		_cond_resched();
+		__this_cpu_write(xen_in_preemptible_hcall, true);
+	}
+}
+#endif /* CONFIG_PREEMPT */
diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
index 569a13b..59ac71c 100644
--- a/drivers/xen/privcmd.c
+++ b/drivers/xen/privcmd.c
@@ -56,10 +56,12 @@ static long privcmd_ioctl_hypercall(void __user *udata)
 	if (copy_from_user(&hypercall, udata, sizeof(hypercall)))
 		return -EFAULT;
 
+	xen_preemptible_hcall_begin();
 	ret = privcmd_call(hypercall.op,
 			   hypercall.arg[0], hypercall.arg[1],
 			   hypercall.arg[2], hypercall.arg[3],
 			   hypercall.arg[4]);
+	xen_preemptible_hcall_end();
 
 	return ret;
 }
diff --git a/include/xen/xen-ops.h b/include/xen/xen-ops.h
index 7491ee5..8333821 100644
--- a/include/xen/xen-ops.h
+++ b/include/xen/xen-ops.h
@@ -46,4 +46,30 @@ static inline efi_system_table_t __init *xen_efi_probe(void)
 }
 #endif
 
+#ifdef CONFIG_PREEMPT
+
+static inline void xen_preemptible_hcall_begin(void)
+{
+}
+
+static inline void xen_preemptible_hcall_end(void)
+{
+}
+
+#else
+
+DECLARE_PER_CPU(bool, xen_in_preemptible_hcall);
+
+static inline void xen_preemptible_hcall_begin(void)
+{
+	__this_cpu_write(xen_in_preemptible_hcall, true);
+}
+
+static inline void xen_preemptible_hcall_end(void)
+{
+	__this_cpu_write(xen_in_preemptible_hcall, false);
+}
+
+#endif /* CONFIG_PREEMPT */
+
 #endif /* INCLUDE_XEN_OPS_H */
-- 
1.7.10.4

Re: [PATCHv5] x86/xen: allow privcmd hypercalls to be preempted

[Boris Ostrovsky]

On 02/05/2015 07:41 AM, David Vrabel wrote:
> Hypercalls submitted by user space tools via the privcmd driver can
> take a long time (potentially many 10s of seconds) if the hypercall
> has many sub-operations.
>
> A fully preemptible kernel may deschedule such as task in any upcall
> called from a hypercall continuation.
>
> However, in a kernel with voluntary or no preemption, hypercall
> continuations in Xen allow event handlers to be run but the task
> issuing the hypercall will not be descheduled until the hypercall is
> complete and the ioctl returns to user space.  These long running
> tasks may also trigger the kernel's soft lockup detection.
>
> Add xen_preemptible_hcall_begin() and xen_preemptible_hcall_end() to
> bracket hypercalls that may be preempted.  Use these in the privcmd
> driver.
>
> When returning from an upcall, call xen_maybe_preempt_hcall() which
> adds a schedule point if if the current task was within a preemptible
> hypercall.
>
> Since preempt_schedule_irq() can move the task to a different CPU,
> clear and set xen_in_preemptible_hcall around the call.
>
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>

Is this in place of Luis' earlier patches?

-boris

Re: [PATCHv5] x86/xen: allow privcmd hypercalls to be preempted

[David Vrabel]

On 05/02/15 15:37, Boris Ostrovsky wrote:
> On 02/05/2015 07:41 AM, David Vrabel wrote:
>> Hypercalls submitted by user space tools via the privcmd driver can
>> take a long time (potentially many 10s of seconds) if the hypercall
>> has many sub-operations.
>>
>> A fully preemptible kernel may deschedule such as task in any upcall
>> called from a hypercall continuation.
>>
>> However, in a kernel with voluntary or no preemption, hypercall
>> continuations in Xen allow event handlers to be run but the task
>> issuing the hypercall will not be descheduled until the hypercall is
>> complete and the ioctl returns to user space.  These long running
>> tasks may also trigger the kernel's soft lockup detection.
>>
>> Add xen_preemptible_hcall_begin() and xen_preemptible_hcall_end() to
>> bracket hypercalls that may be preempted.  Use these in the privcmd
>> driver.
>>
>> When returning from an upcall, call xen_maybe_preempt_hcall() which
>> adds a schedule point if if the current task was within a preemptible
>> hypercall.
>>
>> Since preempt_schedule_irq() can move the task to a different CPU,
>> clear and set xen_in_preemptible_hcall around the call.
>>
>> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> 
> Is this in place of Luis' earlier patches?

Yes.

David

Re: [PATCHv5] x86/xen: allow privcmd hypercalls to be preempted

[Boris Ostrovsky]

On 02/05/2015 07:41 AM, David Vrabel wrote:
> +
> +void xen_maybe_preempt_hcall(void)
> +{
> +	if (__this_cpu_read(xen_in_preemptible_hcall)) {

Can you check should_resched() here?

-boris

> +		/*
> +		 * Clear flag as we may be rescheduled on a different
> +		 * cpu.
> +		 */
> +		__this_cpu_write(xen_in_preemptible_hcall, false);
> +		_cond_resched();
> +		__this_cpu_write(xen_in_preemptible_hcall, true);
> +	}
> +}
> +#endif /* CONFIG_PREEMPT */
>

Re: [PATCHv5] x86/xen: allow privcmd hypercalls to be preempted

[David Vrabel]

On 05/02/15 16:11, Boris Ostrovsky wrote:
> On 02/05/2015 07:41 AM, David Vrabel wrote:
>> +
>> +void xen_maybe_preempt_hcall(void)
>> +{
>> +    if (__this_cpu_read(xen_in_preemptible_hcall)) {
> 
> Can you check should_resched() here?

_cond_resched() already does this.

David

Re: [PATCHv5] x86/xen: allow privcmd hypercalls to be preempted

[Boris Ostrovsky]

On 02/05/2015 11:14 AM, David Vrabel wrote:
> On 05/02/15 16:11, Boris Ostrovsky wrote:
>> On 02/05/2015 07:41 AM, David Vrabel wrote:
>>> +
>>> +void xen_maybe_preempt_hcall(void)
>>> +{
>>> +    if (__this_cpu_read(xen_in_preemptible_hcall)) {
>> Can you check should_resched() here?
> _cond_resched() already does this.
>

Right, you'd be checking it twice, but since in most cases you won't 
need to reschedule you will then avoid making unnecessary call (and 
per-cpu updates).

-boris

Re: [PATCHv5] x86/xen: allow privcmd hypercalls to be preempted

[Luis R. Rodriguez]

On Thu, Feb 05, 2015 at 12:41:17PM +0000, David Vrabel wrote:
> --- a/arch/x86/kernel/entry_32.S
> +++ b/arch/x86/kernel/entry_32.S
> @@ -982,6 +982,9 @@ ENTRY(xen_hypervisor_callback)
>  ENTRY(xen_do_upcall)
>  1:	mov %esp, %eax
>  	call xen_evtchn_do_upcall
> +#ifndef CONFIG_PREEMPT
> +	call xen_maybe_preempt_hcall
> +#endif
>  	jmp  ret_from_intr
>  	CFI_ENDPROC
>  ENDPROC(xen_hypervisor_callback)

Oh good, did you get to test 32-bit? I was still trying to get
a good recent Xen build to test the domU suggestion you had.

> diff --git a/arch/x86/kernel/entry_64.S b/arch/x86/kernel/entry_64.S
> index 9ebaf63..9069401 100644
> --- a/arch/x86/kernel/entry_64.S
> +++ b/arch/x86/kernel/entry_64.S
> @@ -1198,6 +1198,9 @@ ENTRY(xen_do_hypervisor_callback)   # do_hypervisor_callback(struct *pt_regs)
>  	popq %rsp
>  	CFI_DEF_CFA_REGISTER rsp
>  	decl PER_CPU_VAR(irq_count)
> +#ifndef CONFIG_PREEMPT
> +	call xen_maybe_preempt_hcall
> +#endif
>  	jmp  error_exit
>  	CFI_ENDPROC
>  END(xen_do_hypervisor_callback)

> diff --git a/drivers/xen/preempt.c b/drivers/xen/preempt.c
> new file mode 100644
> index 0000000..e2bcc01
> --- /dev/null
> +++ b/drivers/xen/preempt.c

<-- ... -->

> +DEFINE_PER_CPU(bool, xen_in_preemptible_hcall);
> +EXPORT_SYMBOL_GPL(xen_in_preemptible_hcall);
> +
> +void xen_maybe_preempt_hcall(void)
> +{
> +	if (__this_cpu_read(xen_in_preemptible_hcall)) {
> +		/*
> +		 * Clear flag as we may be rescheduled on a different
> +		 * cpu.
> +		 */
> +		__this_cpu_write(xen_in_preemptible_hcall, false);

This read might be on CPU 3.

> +		_cond_resched();
> +		__this_cpu_write(xen_in_preemptible_hcall, true);

This CPU wright might happen on CPU 1 and as such it would
write true to another CPU. I had tested this approach and
it was my original approach and while it sort of works its
obviously broken due to the above. After considering
Andy's original request to use pt_regs instead it seemed a lot
safer than trying to fix this approach, so that is what I
went with.

  Luis

Re: [PATCHv5] x86/xen: allow privcmd hypercalls to be preempted

[Andrew Cooper]

On 05/02/15 18:14, Luis R. Rodriguez wrote:
> On Thu, Feb 05, 2015 at 12:41:17PM +0000, David Vrabel wrote:
>> --- a/arch/x86/kernel/entry_32.S
>> +++ b/arch/x86/kernel/entry_32.S
>> @@ -982,6 +982,9 @@ ENTRY(xen_hypervisor_callback)
>>  ENTRY(xen_do_upcall)
>>  1:	mov %esp, %eax
>>  	call xen_evtchn_do_upcall
>> +#ifndef CONFIG_PREEMPT
>> +	call xen_maybe_preempt_hcall
>> +#endif
>>  	jmp  ret_from_intr
>>  	CFI_ENDPROC
>>  ENDPROC(xen_hypervisor_callback)
> Oh good, did you get to test 32-bit? I was still trying to get
> a good recent Xen build to test the domU suggestion you had.
>
>> diff --git a/arch/x86/kernel/entry_64.S b/arch/x86/kernel/entry_64.S
>> index 9ebaf63..9069401 100644
>> --- a/arch/x86/kernel/entry_64.S
>> +++ b/arch/x86/kernel/entry_64.S
>> @@ -1198,6 +1198,9 @@ ENTRY(xen_do_hypervisor_callback)   # do_hypervisor_callback(struct *pt_regs)
>>  	popq %rsp
>>  	CFI_DEF_CFA_REGISTER rsp
>>  	decl PER_CPU_VAR(irq_count)
>> +#ifndef CONFIG_PREEMPT
>> +	call xen_maybe_preempt_hcall
>> +#endif
>>  	jmp  error_exit
>>  	CFI_ENDPROC
>>  END(xen_do_hypervisor_callback)
>> diff --git a/drivers/xen/preempt.c b/drivers/xen/preempt.c
>> new file mode 100644
>> index 0000000..e2bcc01
>> --- /dev/null
>> +++ b/drivers/xen/preempt.c
> <-- ... -->
>
>> +DEFINE_PER_CPU(bool, xen_in_preemptible_hcall);
>> +EXPORT_SYMBOL_GPL(xen_in_preemptible_hcall);
>> +
>> +void xen_maybe_preempt_hcall(void)
>> +{
>> +	if (__this_cpu_read(xen_in_preemptible_hcall)) {
>> +		/*
>> +		 * Clear flag as we may be rescheduled on a different
>> +		 * cpu.
>> +		 */
>> +		__this_cpu_write(xen_in_preemptible_hcall, false);
> This read might be on CPU 3.
>
>> +		_cond_resched();
>> +		__this_cpu_write(xen_in_preemptible_hcall, true);
> This CPU wright might happen on CPU 1 and as such it would
> write true to another CPU.

That is the entire point, because the hypercall is now being continued
on CPU1 rather than CPU3.  The "preemptibleness" follows the task which
initiated the hypercall, not the cpu which it happened to start
executing on.

~Andrew

>  I had tested this approach and
> it was my original approach and while it sort of works its
> obviously broken due to the above. After considering
> Andy's original request to use pt_regs instead it seemed a lot
> safer than trying to fix this approach, so that is what I
> went with.
>
>   Luis
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCHv5] x86/xen: allow privcmd hypercalls to be preempted

[Luis R. Rodriguez]

On Thu, Feb 5, 2015 at 10:23 AM, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
> On 05/02/15 18:14, Luis R. Rodriguez wrote:
>> On Thu, Feb 05, 2015 at 12:41:17PM +0000, David Vrabel wrote:
>>> diff --git a/drivers/xen/preempt.c b/drivers/xen/preempt.c
>>> new file mode 100644
>>> index 0000000..e2bcc01
>>> --- /dev/null
>>> +++ b/drivers/xen/preempt.c
>> <-- ... -->
>>
>>> +DEFINE_PER_CPU(bool, xen_in_preemptible_hcall);
>>> +EXPORT_SYMBOL_GPL(xen_in_preemptible_hcall);
>>> +
>>> +void xen_maybe_preempt_hcall(void)
>>> +{
>>> +    if (__this_cpu_read(xen_in_preemptible_hcall)) {
>>> +            /*
>>> +             * Clear flag as we may be rescheduled on a different
>>> +             * cpu.
>>> +             */
>>> +            __this_cpu_write(xen_in_preemptible_hcall, false);
>> This read might be on CPU 3.
>>
>>> +            _cond_resched();
>>> +            __this_cpu_write(xen_in_preemptible_hcall, true);
>> This CPU wright might happen on CPU 1 and as such it would
>> write true to another CPU.
>
> That is the entire point, because the hypercall is now being continued
> on CPU1 rather than CPU3.  The "preemptibleness" follows the task which
> initiated the hypercall, not the cpu which it happened to start
> executing on.

Ah yes, indeed. Glad we could ditch pt_regs!

 Luis

Re: [PATCHv5] x86/xen: allow privcmd hypercalls to be preempted

[Andy Lutomirski]

On Thu, Feb 5, 2015 at 4:41 AM, David Vrabel <david.vrabel@citrix.com> wrote:
> Hypercalls submitted by user space tools via the privcmd driver can
> take a long time (potentially many 10s of seconds) if the hypercall
> has many sub-operations.
>

> +
> +void xen_maybe_preempt_hcall(void)

I think this should be asmlinkage __visible.  Other than that, this
looks good to me.  I like your solution to the tracking problem.

> +{
> +       if (__this_cpu_read(xen_in_preemptible_hcall)) {

Hmm.  That being said, is there any issue with nested interrupts?
Suppose you get an interrupt that re-enables IRQs in the middle (I
think we do this on occasion).  Then you get another interrupt that
reschedules illegally in the middle of the first one.

Something like if (__this_cpu_read(xen_in_preemptible_hcall) &&
!in_interrupt()) { along with a comment might do the trick.

--Andy

Re: [PATCHv5] x86/xen: allow privcmd hypercalls to be preempted

[David Vrabel]

On 06/02/15 00:50, Andy Lutomirski wrote:
> On Thu, Feb 5, 2015 at 4:41 AM, David Vrabel <david.vrabel@citrix.com> wrote:
>> Hypercalls submitted by user space tools via the privcmd driver can
>> take a long time (potentially many 10s of seconds) if the hypercall
>> has many sub-operations.
>>
> 
>> +
>> +void xen_maybe_preempt_hcall(void)
> 
> I think this should be asmlinkage __visible.  Other than that, this
> looks good to me.  I like your solution to the tracking problem.
> 
>> +{
>> +       if (__this_cpu_read(xen_in_preemptible_hcall)) {
> 
> Hmm.  That being said, is there any issue with nested interrupts?

The should_resched() check in _cond_resched() already handles this
because irq_enter() sets the preempt count as non-zero.

FWIW, XenServer has been using something like this in production for
about a year without any problems.

David