Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1262968

Re: [patch 02/14] x86/apic: Implement single target IPI function for x2apic_cluster

From Ingo Molnar <mingo@kernel.org>
Newsgroups linux.kernel
Subject Re: [patch 02/14] x86/apic: Implement single target IPI function for x2apic_cluster
Date 2015-11-05 08:10 +0100
Message-ID <qrmQh-3q6-15@gated-at.bofh.it> (permalink)
References <qrfc5-6Fb-3@gated-at.bofh.it> <qrflN-6Yk-33@gated-at.bofh.it>
Organization linux.* mail to news gateway

Show all headers | View raw

* Thomas Gleixner <tglx@linutronix.de> wrote:

> From: Linus Torvalds <torvalds@linux-foundation.org>
> 
> [ 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 <torvalds@linux-foundation.org>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>  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 <mingo@kernel.org>

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 <mingo@kernel.org>
---
 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[patch 00/14] x86/apic: Implement single target IPI callback Thomas Gleixner <tglx@linutronix.de> - 2015-11-05 00:10 +0100
  [patch 06/14] x86/apic: Remove pointless indirections from bigsmp_apic Thomas Gleixner <tglx@linutronix.de> - 2015-11-05 00:10 +0100
    [tip:x86/apic] x86/apic:   Remove pointless indirections from bigsmp_apic tip-bot for Thomas Gleixner <tipbot@zytor.com> - 2015-11-05 15:50 +0100
  [patch 01/14] x86/apic: Add a single-target IPI function to the apic Thomas Gleixner <tglx@linutronix.de> - 2015-11-05 00:10 +0100
    [tip:x86/apic] x86/apic:   Add a single-target IPI function to the apic tip-bot for Linus Torvalds <tipbot@zytor.com> - 2015-11-05 15:50 +0100
  [patch 05/14] x86/apic: Wire up single IPI for apic_physflat Thomas Gleixner <tglx@linutronix.de> - 2015-11-05 00:10 +0100
    [tip:x86/apic] x86/apic: Wire up single IPI for apic_physflat tip-bot for Thomas Gleixner <tipbot@zytor.com> - 2015-11-05 15:50 +0100
  [patch 04/14] x86/apic: Remove pointless indirections from  apic_physflat Thomas Gleixner <tglx@linutronix.de> - 2015-11-05 00:10 +0100
    [tip:x86/apic] x86/apic:   Remove pointless indirections from apic_physflat tip-bot for Thomas Gleixner <tipbot@zytor.com> - 2015-11-05 15:50 +0100
  [patch 08/14] x86/apic: Implement single IPI for x2apic_phys Thomas Gleixner <tglx@linutronix.de> - 2015-11-05 00:10 +0100
    [tip:x86/apic] x86/apic: Implement single IPI for x2apic_phys tip-bot for Thomas Gleixner <tipbot@zytor.com> - 2015-11-05 15:50 +0100
  [patch 02/14] x86/apic: Implement single target IPI function for  x2apic_cluster Thomas Gleixner <tglx@linutronix.de> - 2015-11-05 00:10 +0100
    Re: [patch 02/14] x86/apic: Implement single target IPI function for x2apic_cluster Linus Torvalds <torvalds@linux-foundation.org> - 2015-11-05 08:10 +0100
      Re: [patch 02/14] x86/apic: Implement single target IPI function  for x2apic_cluster Thomas Gleixner <tglx@linutronix.de> - 2015-11-05 09:10 +0100
    Re: [patch 02/14] x86/apic: Implement single target IPI function for  x2apic_cluster Ingo Molnar <mingo@kernel.org> - 2015-11-05 08:10 +0100
    [tip:x86/apic] x86/apic:   Implement single target IPI function for x2apic_cluster tip-bot for Linus Torvalds <tipbot@zytor.com> - 2015-11-05 15:50 +0100
  [patch 10/14] x86/apic: Implement single IPI for apic_noop Thomas Gleixner <tglx@linutronix.de> - 2015-11-05 00:10 +0100
    [tip:x86/apic] x86/apic: Implement single IPI for apic_noop tip-bot for Thomas Gleixner <tipbot@zytor.com> - 2015-11-05 15:50 +0100

csiph-web