All of lore.kernel.org
 help / color / mirror / Atom feed
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



  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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.