From: Thomas Gleixner <tglx@linutronix.de>
To: Yi Sun <yi.y.sun@linux.intel.com>
Cc: linux-kernel@vger.kernel.org, x86@kernel.org,
chao.p.peng@intel.com, chao.gao@intel.com,
isaku.yamahata@intel.com, michael.h.kelly@microsoft.com,
tianyu.lan@microsoft.com, "K. Y. Srinivasan" <kys@microsoft.com>,
Haiyang Zhang <haiyangz@microsoft.com>,
Stephen Hemminger <sthemmin@microsoft.com>
Subject: Re: [PATCH v1 2/3] locking/pvqspinlock, hv: Enable PV qspinlock for Hyper-V
Date: Thu, 13 Sep 2018 13:24:07 +0200 (CEST) [thread overview]
Message-ID: <alpine.DEB.2.21.1809131307110.1473@nanos.tec.linutronix.de> (raw)
In-Reply-To: <1536830005-37260-3-git-send-email-yi.y.sun@linux.intel.com>
On Thu, 13 Sep 2018, Yi Sun wrote:
> +++ b/arch/x86/hyperv/hv_spinlock.c
> @@ -0,0 +1,99 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +/*
> + * Hyper-V specific spinlock code.
> + *
> + * Copyright (C) 2018, Intel, Inc.
> + *
> + * Author : Yi Sun <yi.y.sun@linux.intel.com>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published
> + * by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE, GOOD TITLE or
> + * NON INFRINGEMENT. See the GNU General Public License for more
> + * details.
Please remove the boilerplate. The SPDX identifier is enough. If in doubt
talk to Intel legal.
> +static bool hv_pvspin = true;
__initdata please.
> +static u32 spin_wait_info = 0;
No need for 0 initialization.
> +
> +static void hv_notify_long_spin_wait(void)
> +{
> + u64 input = spin_wait_info | 0x00000000ffffffff;
What? The result is always 0x00000000ffffffff .....
> + spin_wait_info++;
> + hv_do_fast_hypercall8(HVCALL_NOTIFY_LONG_SPIN_WAIT, input);
> +}
> +
> +static void hv_qlock_kick(int cpu)
> +{
> + spin_wait_info--;
Looking at the above this is completely pointless and I have no idea what
that variable is supposed to do.
> + apic->send_IPI(cpu, X86_PLATFORM_IPI_VECTOR);
> +}
> +
> +/*
> + * Halt the current CPU & release it back to the host
> + */
> +static void hv_qlock_wait(u8 *byte, u8 val)
> +{
> + unsigned long msr_val;
> +
> + if (READ_ONCE(*byte) != val)
> + return;
> +
> + hv_notify_long_spin_wait();
> +
> + rdmsrl(HV_X64_MSR_GUEST_IDLE, msr_val);
Magic rdmsrl(). That wants a comment what this is for.
> +/*
> + * Hyper-V does not support this so far.
> + */
> +bool hv_vcpu_is_preempted(int vcpu)
static ?
> +{
> + return false;
> +}
> +PV_CALLEE_SAVE_REGS_THUNK(hv_vcpu_is_preempted);
> +
> +void __init hv_init_spinlocks(void)
> +{
> + if (!hv_pvspin ||
> + !apic ||
> + !(ms_hyperv.hints & HV_X64_CLUSTER_IPI_RECOMMENDED) ||
> + !(ms_hyperv.features & HV_X64_MSR_GUEST_IDLE_AVAILABLE)) {
> + pr_warn("hv: PV spinlocks disabled\n");
Why does this need to be pr_warn? This is purely informational. Also please
use pr_fmt instead of the 'hv:' prefix.
> +static __init int hv_parse_nopvspin(char *arg)
> +{
> + hv_pvspin = false;
> + return 0;
> +}
> +early_param("hv_nopvspin", hv_parse_nopvspin);
That lacks Documentation of the command line parameter. Wants to be in the
same patch.
Thanks,
tglx
next prev parent reply other threads:[~2018-09-13 11:24 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-09-13 9:13 [PATCH v1 0/3] Enable PV qspinlock for Hyper-V Yi Sun
2018-09-13 9:13 ` [PATCH v1 1/3] X86/Hyper-V: Add Guest IDLE MSR support Yi Sun
2018-09-13 9:13 ` [PATCH v1 2/3] locking/pvqspinlock, hv: Enable PV qspinlock for Hyper-V Yi Sun
2018-09-13 11:06 ` Thomas Gleixner
2018-09-14 8:53 ` Yi Sun
2018-09-13 11:24 ` Thomas Gleixner [this message]
2018-09-14 9:04 ` Yi Sun
2018-09-14 10:42 ` Thomas Gleixner
2018-09-17 12:54 ` Yi Sun
2018-09-20 1:56 ` Yi Sun
2018-09-13 16:16 ` kbuild test robot
2018-09-13 16:42 ` kbuild test robot
2018-09-13 9:13 ` [PATCH v1 3/3] hv: add description for hv_nopvspin Yi Sun
2018-09-15 23:43 ` [PATCH v1 0/3] Enable PV qspinlock for Hyper-V Michael Kelley (EOSG)
2018-09-17 14:37 ` 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=alpine.DEB.2.21.1809131307110.1473@nanos.tec.linutronix.de \
--to=tglx@linutronix.de \
--cc=chao.gao@intel.com \
--cc=chao.p.peng@intel.com \
--cc=haiyangz@microsoft.com \
--cc=isaku.yamahata@intel.com \
--cc=kys@microsoft.com \
--cc=linux-kernel@vger.kernel.org \
--cc=michael.h.kelly@microsoft.com \
--cc=sthemmin@microsoft.com \
--cc=tianyu.lan@microsoft.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).