On 10/22/2018 01:15 PM, Peter Zijlstra wrote: > Firstly, who come a patch that is grubbing around in kernel/locking/ has > an x86/hyperv subject and isn't Cc'ed to the locking maintainers? > > On Mon, Oct 22, 2018 at 12:31:45PM -0400, Waiman Long wrote: >> On 10/22/2018 03:32 AM, Juergen Gross wrote: >>> On 22/10/2018 03:53, Yi Sun wrote: >>>> On 18-10-19 16:20:52, Juergen Gross wrote: >>>>> On 19/10/2018 15:13, Yi Sun wrote: >>>> [...] >>>> >>>>>> diff --git a/kernel/locking/qspinlock_paravirt.h >>>>>> b/kernel/locking/qspinlock_paravirt.h >>>>>> index 0130e48..9e88c7e 100644 >>>>>> --- a/kernel/locking/qspinlock_paravirt.h >>>>>> +++ b/kernel/locking/qspinlock_paravirt.h >>>>>> @@ -7,6 +7,8 @@ >>>>>> #include <linux/bootmem.h> >>>>>> #include <linux/debug_locks.h> >>>>>> >>>>>> +#include <asm/mshyperv.h> >>>>>> + >>>>>> /* >>>>>> * Implement paravirt qspinlocks; the general idea is to halt the vcpus >>>>>> instead >>>>>> * of spinning them. >>>>>> @@ -305,6 +307,10 @@ static void pv_wait_node(struct mcs_spinlock *node, >>>>>> struct mcs_spinlock *prev) >>>>>> wait_early = true; >>>>>> break; >>>>>> } >>>>>> +#if defined(CONFIG_X86_64) && defined(CONFIG_PARAVIRT_SPINLOCKS) && >>>>>> IS_ENABLED(CONFIG_HYPERV) >>>>>> + if (!hv_notify_long_spin_wait(SPIN_THRESHOLD - >>>>>> loop)) >>>>>> + break; >>>>>> +#endif > Secondly; how come you thought that was acceptable in any way shape or > form? > >>> vcpu_is_preempted() is already part of this loop. And this is a paravirt >>> hook. Can't you make use of that? This might require adding another >>> parameter to this hook, but I'd prefer that over another pv-spinlock >>> hook. >> I agree with Juergen on that. I would suggest rename the >> vcpu_is_preempted hook into a more generic vcpu_stop_spinning, perhaps, >> so different hypervisors can act on the information accordingly. Adding >> an extra parameter is fine. > No; no extra parameters. vcpu_is_preempted() is a simple and intuitive > interface. Why would we want to make it complicated?
Hyperv seems to do it in a somewhat different way by looking at the spin counter and decide if it should continue. I don't know why they do that and what advantage it has. The current patch is definitely not OK. A revised patch that makes use of an existing paravirt hook will be more acceptable. Again, I would like to see some performance figure and why they do it this way to see if it is worthwhile to change the existing interface. Cheers, Longman
