From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1031945AbbKEGiZ (ORCPT ); Thu, 5 Nov 2015 01:38:25 -0500 Received: from mail-wi0-f170.google.com ([209.85.212.170]:38224 "EHLO mail-wi0-f170.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1032160AbbKEGiS (ORCPT ); Thu, 5 Nov 2015 01:38:18 -0500 Date: Thu, 5 Nov 2015 07:38:13 +0100 From: Ingo Molnar To: Thomas Gleixner Cc: LKML , Linus Torvalds , Peter Anvin , Borislav Petkov , Peter Zijlstra , Mike Travis , Daniel J Blueman Subject: Re: [patch 02/14] x86/apic: Implement single target IPI function for x2apic_cluster Message-ID: <20151105063813.GA12730@gmail.com> References: <20151104215716.810356093@linutronix.de> <20151104220848.817975597@linutronix.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20151104220848.817975597@linutronix.de> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org * Thomas Gleixner wrote: > From: Linus Torvalds > > [ tglx: Split it out from the patch which provides the new callback > and wrapped it into local_irq_save/restore ] > > Signed-off-by: Linus Torvalds > Signed-off-by: Thomas Gleixner > --- > arch/x86/kernel/apic/x2apic_cluster.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > Index: linux/arch/x86/kernel/apic/x2apic_cluster.c > =================================================================== > --- linux.orig/arch/x86/kernel/apic/x2apic_cluster.c > +++ linux/arch/x86/kernel/apic/x2apic_cluster.c > @@ -23,6 +23,17 @@ static inline u32 x2apic_cluster(int cpu > return per_cpu(x86_cpu_to_logical_apicid, cpu) >> 16; > } > > +static void x2apic_send_IPI(int cpu, int vector) > +{ > + u32 dest = per_cpu(x86_cpu_to_logical_apicid, cpu); > + unsigned long flags; > + > + x2apic_wrmsr_fence(); > + local_irq_save(flags); > + __x2apic_send_IPI_dest(dest, vector, APIC_DEST_LOGICAL); > + local_irq_restore(flags); > +} So the series looks good to me: Reviewed-by: Ingo Molnar but in the above sequence I think we can do even better: we don't need the local_irq_save()/restore() I think. The reason, this is how __x2apic_send_IPI_dest() looks like: unsigned long cfg = __prepare_ICR(0, vector, dest); native_x2apic_icr_write(cfg, apicid); __prepare_ICR(), which is a confusing misnomer as it does not prepare anything about the ICR register, it just pre-calculates some values, is obviously interrupt-safe: static inline unsigned int __prepare_ICR(unsigned int shortcut, int vector, unsigned int dest) { unsigned int icr = shortcut | dest; switch (vector) { default: icr |= APIC_DM_FIXED | vector; break; case NMI_VECTOR: icr |= APIC_DM_NMI; break; } return icr; } and native_x2apic_icr_write() is a single WRMSR: static inline void native_x2apic_icr_write(u32 low, u32 id) { wrmsrl(APIC_BASE_MSR + (APIC_ICR >> 4), ((__u64) id) << 32 | low); } which is interrupt-safe as well. So we can save another 10-20 cycles of CLI/POPF overhead from this hotpath. I'd do it as a patch on top, to keep the series simpler - something like the below. (Completely untested: may the Force be with you.) Thanks, Ingo Signed-off-by: Ingo Molnar --- arch/x86/kernel/apic/x2apic_cluster.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/arch/x86/kernel/apic/x2apic_cluster.c b/arch/x86/kernel/apic/x2apic_cluster.c index 3329dab47efc..aca8b75c1552 100644 --- a/arch/x86/kernel/apic/x2apic_cluster.c +++ b/arch/x86/kernel/apic/x2apic_cluster.c @@ -26,12 +26,9 @@ static inline u32 x2apic_cluster(int cpu) static void x2apic_send_IPI(int cpu, int vector) { u32 dest = per_cpu(x86_cpu_to_logical_apicid, cpu); - unsigned long flags; x2apic_wrmsr_fence(); - local_irq_save(flags); __x2apic_send_IPI_dest(dest, vector, APIC_DEST_LOGICAL); - local_irq_restore(flags); } static void