From: Waiman Long <longman@redhat.com>
To: Peter Zijlstra <peterz@infradead.org>
Cc: Juergen Gross <jgross@suse.com>,
Yi Sun <yi.y.sun@linux.intel.com>,
linux-kernel@vger.kernel.org, x86@kernel.org, tglx@linutronix.de,
chao.p.peng@intel.com, chao.gao@intel.com,
isaku.yamahata@intel.com, michael.h.kelley@microsoft.com,
tianyu.lan@microsoft.com, "K. Y. Srinivasan" <kys@microsoft.com>,
Haiyang Zhang <haiyangz@microsoft.com>,
Stephen Hemminger <sthemmin@microsoft.com>,
"mingo@redhat.com" <mingo@redhat.com>,
Will Deacon <will.deacon@arm.com>
Subject: Re: [PATCH v1 2/2] x86/hyperv: make HvNotifyLongSpinWait hypercall
Date: Mon, 22 Oct 2018 13:27:27 -0400 [thread overview]
Message-ID: <6d8095c0-af95-5967-3ca5-2ceeb74233ea@redhat.com> (raw)
In-Reply-To: <20181022171516.GH3117@worktop.programming.kicks-ass.net>
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
next prev parent reply other threads:[~2018-10-22 17:27 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-10-19 13:13 [PATCH v1 0/2] Enable HvNotifyLongSpinWait for Hyper-V Yi Sun
2018-10-19 13:13 ` [PATCH v1 1/2] x86/hyperv: get spinlock retry number on Hyper-V Yi Sun
2018-10-19 13:13 ` [PATCH v1 2/2] x86/hyperv: make HvNotifyLongSpinWait hypercall Yi Sun
2018-10-19 14:20 ` Juergen Gross
2018-10-22 1:53 ` Yi Sun
2018-10-22 7:32 ` Juergen Gross
2018-10-22 16:31 ` Waiman Long
2018-10-22 17:15 ` Peter Zijlstra
2018-10-22 17:27 ` Waiman Long [this message]
2018-10-22 17:31 ` Peter Zijlstra
2018-10-22 18:01 ` Waiman Long
2018-10-23 2:57 ` Yi Sun
2018-10-23 8:51 ` Peter Zijlstra
2018-10-23 9:33 ` Yi Sun
2018-10-31 1:54 ` Yi Sun
2018-10-31 14:10 ` Peter Zijlstra
2018-10-31 15:07 ` Waiman Long
2018-10-31 17:15 ` Peter Zijlstra
2018-11-01 3:20 ` Yi Sun
2018-11-01 8:59 ` Peter Zijlstra
2018-11-01 12:59 ` Waiman Long
2018-11-05 6:54 ` Yi Sun
2018-10-24 16:53 ` Michael Kelley
2018-10-25 2:23 ` Yi Sun
2018-10-31 2:06 ` Yi Sun
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=6d8095c0-af95-5967-3ca5-2ceeb74233ea@redhat.com \
--to=longman@redhat.com \
--cc=chao.gao@intel.com \
--cc=chao.p.peng@intel.com \
--cc=haiyangz@microsoft.com \
--cc=isaku.yamahata@intel.com \
--cc=jgross@suse.com \
--cc=kys@microsoft.com \
--cc=linux-kernel@vger.kernel.org \
--cc=michael.h.kelley@microsoft.com \
--cc=mingo@redhat.com \
--cc=peterz@infradead.org \
--cc=sthemmin@microsoft.com \
--cc=tglx@linutronix.de \
--cc=tianyu.lan@microsoft.com \
--cc=will.deacon@arm.com \
--cc=x86@kernel.org \
--cc=yi.y.sun@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).