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

[Andrew Cooper <andrew.cooper3@citrix.com>]

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