| Message ID | 1509569934-4919-3-git-send-email-longman@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 11/01/2017 04:58 PM, Waiman Long wrote: > +/* TODO: To be removed in a future kernel version */ > static __init int xen_parse_nopvspin(char *arg) > { > - xen_pvspin = false; > + pr_warn("xen_nopvspin is deprecated, replace it with \"pvlock_type=queued\"!\n"); > + if (!pv_spinlock_type) > + pv_spinlock_type = locktype_queued; Since we currently end up using unfair locks and because you are deprecating xen_nopvspin I wonder whether it would be better to set this to locktype_unfair so that current behavior doesn't change. (Sorry, I haven't responded to your earlier message before you posted this). Juergen? I am also not sure I agree with making pv_spinlock an enum *and* a bitmask at the same time. I understand that it makes checks easier but I think not assuming a value or a pattern would be better, especially since none of the uses is on a critical path. (For example, !pv_spinlock_type is the same as locktype_auto, which is defined but never used) -boris > return 0; > } > early_param("xen_nopvspin", xen_parse_nopvspin); > -
On 11/01/2017 06:01 PM, Boris Ostrovsky wrote: > On 11/01/2017 04:58 PM, Waiman Long wrote: >> +/* TODO: To be removed in a future kernel version */ >> static __init int xen_parse_nopvspin(char *arg) >> { >> - xen_pvspin = false; >> + pr_warn("xen_nopvspin is deprecated, replace it with \"pvlock_type=queued\"!\n"); >> + if (!pv_spinlock_type) >> + pv_spinlock_type = locktype_queued; > Since we currently end up using unfair locks and because you are > deprecating xen_nopvspin I wonder whether it would be better to set this > to locktype_unfair so that current behavior doesn't change. (Sorry, I > haven't responded to your earlier message before you posted this). Juergen? I think the latest patch from Juergen in tip is to use native qspinlock when xen_nopvspin is specified. Right? That is why I made the current choice. I can certainly change to unfair if it is what you guys want. > I am also not sure I agree with making pv_spinlock an enum *and* a > bitmask at the same time. I understand that it makes checks easier but I > think not assuming a value or a pattern would be better, especially > since none of the uses is on a critical path. > > (For example, !pv_spinlock_type is the same as locktype_auto, which is > defined but never used) OK, I will take out the enum and make explicit use of locktype_auto. Cheers, Longman
On 02/11/17 14:25, Waiman Long wrote: > On 11/01/2017 06:01 PM, Boris Ostrovsky wrote: >> On 11/01/2017 04:58 PM, Waiman Long wrote: >>> +/* TODO: To be removed in a future kernel version */ >>> static __init int xen_parse_nopvspin(char *arg) >>> { >>> - xen_pvspin = false; >>> + pr_warn("xen_nopvspin is deprecated, replace it with \"pvlock_type=queued\"!\n"); >>> + if (!pv_spinlock_type) >>> + pv_spinlock_type = locktype_queued; >> Since we currently end up using unfair locks and because you are >> deprecating xen_nopvspin I wonder whether it would be better to set this >> to locktype_unfair so that current behavior doesn't change. (Sorry, I >> haven't responded to your earlier message before you posted this). Juergen? > > I think the latest patch from Juergen in tip is to use native qspinlock > when xen_nopvspin is specified. Right? That is why I made the current > choice. I can certainly change to unfair if it is what you guys want. No, when we are keeping xen_nopvspin (even as deprecated) it should behave as designed, so locktype_queued is correct. Juergen
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt index c98d9c7..683a817 100644 --- a/Documentation/admin-guide/kernel-parameters.txt +++ b/Documentation/admin-guide/kernel-parameters.txt @@ -4596,10 +4596,6 @@ the unplug protocol never -- do not unplug even if version check succeeds - xen_nopvspin [X86,XEN] - Disables the ticketlock slowpath using Xen PV - optimizations. - xen_nopv [X86] Disables the PV optimizations forcing the HVM guest to run as generic HVM guest with no PV drivers. diff --git a/arch/x86/xen/spinlock.c b/arch/x86/xen/spinlock.c index d5f79ac..19e2e75 100644 --- a/arch/x86/xen/spinlock.c +++ b/arch/x86/xen/spinlock.c @@ -20,7 +20,6 @@ static DEFINE_PER_CPU(int, lock_kicker_irq) = -1; static DEFINE_PER_CPU(char *, irq_name); -static bool xen_pvspin = true; #include <asm/qspinlock.h> @@ -81,12 +80,8 @@ void xen_init_lock_cpu(int cpu) int irq; char *name; - if (!xen_pvspin || - (pv_spinlock_type & (locktype_queued|locktype_unfair))) { - if ((cpu == 0) && !pv_spinlock_type) - static_branch_disable(&virt_spin_lock_key); + if (pv_spinlock_type & (locktype_queued|locktype_unfair)) return; - } WARN(per_cpu(lock_kicker_irq, cpu) >= 0, "spinlock on CPU%d exists on IRQ%d!\n", cpu, per_cpu(lock_kicker_irq, cpu)); @@ -110,8 +105,7 @@ void xen_init_lock_cpu(int cpu) void xen_uninit_lock_cpu(int cpu) { - if (!xen_pvspin || - (pv_spinlock_type & (locktype_queued|locktype_unfair))) + if (pv_spinlock_type & (locktype_queued|locktype_unfair)) return; unbind_from_irqhandler(per_cpu(lock_kicker_irq, cpu), NULL); @@ -132,8 +126,7 @@ void xen_uninit_lock_cpu(int cpu) */ void __init xen_init_spinlocks(void) { - if (!xen_pvspin || - (pv_spinlock_type & (locktype_queued|locktype_unfair))) { + if (pv_spinlock_type & (locktype_queued|locktype_unfair)) { printk(KERN_DEBUG "xen: PV spinlocks disabled\n"); return; } @@ -147,10 +140,12 @@ void __init xen_init_spinlocks(void) pv_lock_ops.vcpu_is_preempted = PV_CALLEE_SAVE(xen_vcpu_stolen); } +/* TODO: To be removed in a future kernel version */ static __init int xen_parse_nopvspin(char *arg) { - xen_pvspin = false; + pr_warn("xen_nopvspin is deprecated, replace it with \"pvlock_type=queued\"!\n"); + if (!pv_spinlock_type) + pv_spinlock_type = locktype_queued; return 0; } early_param("xen_nopvspin", xen_parse_nopvspin); -
With the new pvlock_type kernel parameter, xen_nopvspin is no longer needed. This patch deprecates the xen_nopvspin parameter by removing its documentation and treating it as an alias of "pvlock_type=queued". Signed-off-by: Waiman Long <longman@redhat.com> --- Documentation/admin-guide/kernel-parameters.txt | 4 ---- arch/x86/xen/spinlock.c | 19 +++++++------------ 2 files changed, 7 insertions(+), 16 deletions(-)