Messages in this thread | | | Date | Thu, 13 Sep 2018 13:24:07 +0200 (CEST) | From | Thomas Gleixner <> | Subject | Re: [PATCH v1 2/3] locking/pvqspinlock, hv: Enable PV qspinlock for Hyper-V |
| |
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
| |