public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
* [GIT pull] irq/urgent for v6.11-rc3
@ 2024-08-11 13:58 Thomas Gleixner
  2024-08-11 13:58 ` [GIT pull] timers/urgent " Thomas Gleixner
                   ` (2 more replies)
  0 siblings, 3 replies; 13+ messages in thread
From: Thomas Gleixner @ 2024-08-11 13:58 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: linux-kernel, x86

Linus,

please pull the latest irq/urgent branch from:

   git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git irq-urgent-2024-08-11

up to:  03f9885c60ad: irqchip/riscv-aplic: Retrigger MSI interrupt on source configuration


Three small fixes for interrupt core and drivers:

    - The interrupt core fails to honor caller supplied affinity hints for
      non-managed interrupts and uses the system default affinity on
      startup instead. Set the missing flag in the descriptor to tell the
      core to use the provided affinity.

    - Fix a shift out of bounds error in the Xilinx driver

    - Handle switching to level trigger correctly in the RISCV APLIC
      driver. It failed to retrigger the interrupt which causes it to
      become stale.

Thanks,

	tglx

------------------>
Radhey Shyam Pandey (1):
      irqchip/xilinx: Fix shift out of bounds

Shay Drory (1):
      genirq/irqdesc: Honor caller provided affinity in alloc_desc()

Yong-Xuan Wang (1):
      irqchip/riscv-aplic: Retrigger MSI interrupt on source configuration


 drivers/irqchip/irq-riscv-aplic-msi.c | 32 +++++++++++++++++++++++++-------
 drivers/irqchip/irq-xilinx-intc.c     |  2 +-
 kernel/irq/irqdesc.c                  |  1 +
 3 files changed, 27 insertions(+), 8 deletions(-)

diff --git a/drivers/irqchip/irq-riscv-aplic-msi.c b/drivers/irqchip/irq-riscv-aplic-msi.c
index 028444af48bd..d7773f76e5d0 100644
--- a/drivers/irqchip/irq-riscv-aplic-msi.c
+++ b/drivers/irqchip/irq-riscv-aplic-msi.c
@@ -32,15 +32,10 @@ static void aplic_msi_irq_unmask(struct irq_data *d)
 	aplic_irq_unmask(d);
 }
 
-static void aplic_msi_irq_eoi(struct irq_data *d)
+static void aplic_msi_irq_retrigger_level(struct irq_data *d)
 {
 	struct aplic_priv *priv = irq_data_get_irq_chip_data(d);
 
-	/*
-	 * EOI handling is required only for level-triggered interrupts
-	 * when APLIC is in MSI mode.
-	 */
-
 	switch (irqd_get_trigger_type(d)) {
 	case IRQ_TYPE_LEVEL_LOW:
 	case IRQ_TYPE_LEVEL_HIGH:
@@ -59,6 +54,29 @@ static void aplic_msi_irq_eoi(struct irq_data *d)
 	}
 }
 
+static void aplic_msi_irq_eoi(struct irq_data *d)
+{
+	/*
+	 * EOI handling is required only for level-triggered interrupts
+	 * when APLIC is in MSI mode.
+	 */
+	aplic_msi_irq_retrigger_level(d);
+}
+
+static int aplic_msi_irq_set_type(struct irq_data *d, unsigned int type)
+{
+	int rc = aplic_irq_set_type(d, type);
+
+	if (rc)
+		return rc;
+	/*
+	 * Updating sourcecfg register for level-triggered interrupts
+	 * requires interrupt retriggering when APLIC is in MSI mode.
+	 */
+	aplic_msi_irq_retrigger_level(d);
+	return 0;
+}
+
 static void aplic_msi_write_msg(struct irq_data *d, struct msi_msg *msg)
 {
 	unsigned int group_index, hart_index, guest_index, val;
@@ -130,7 +148,7 @@ static const struct msi_domain_template aplic_msi_template = {
 		.name			= "APLIC-MSI",
 		.irq_mask		= aplic_msi_irq_mask,
 		.irq_unmask		= aplic_msi_irq_unmask,
-		.irq_set_type		= aplic_irq_set_type,
+		.irq_set_type		= aplic_msi_irq_set_type,
 		.irq_eoi		= aplic_msi_irq_eoi,
 #ifdef CONFIG_SMP
 		.irq_set_affinity	= irq_chip_set_affinity_parent,
diff --git a/drivers/irqchip/irq-xilinx-intc.c b/drivers/irqchip/irq-xilinx-intc.c
index 238d3d344949..7e08714d507f 100644
--- a/drivers/irqchip/irq-xilinx-intc.c
+++ b/drivers/irqchip/irq-xilinx-intc.c
@@ -189,7 +189,7 @@ static int __init xilinx_intc_of_init(struct device_node *intc,
 		irqc->intr_mask = 0;
 	}
 
-	if (irqc->intr_mask >> irqc->nr_irq)
+	if ((u64)irqc->intr_mask >> irqc->nr_irq)
 		pr_warn("irq-xilinx: mismatch in kind-of-intr param\n");
 
 	pr_info("irq-xilinx: %pOF: num_irq=%d, edge=0x%x\n",
diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
index 07e99c936ba5..1dee88ba0ae4 100644
--- a/kernel/irq/irqdesc.c
+++ b/kernel/irq/irqdesc.c
@@ -530,6 +530,7 @@ static int alloc_descs(unsigned int start, unsigned int cnt, int node,
 				flags = IRQD_AFFINITY_MANAGED |
 					IRQD_MANAGED_SHUTDOWN;
 			}
+			flags |= IRQD_AFFINITY_SET;
 			mask = &affinity->mask;
 			node = cpu_to_node(cpumask_first(mask));
 			affinity++;


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [GIT pull] timers/urgent for v6.11-rc3
  2024-08-11 13:58 [GIT pull] irq/urgent for v6.11-rc3 Thomas Gleixner
@ 2024-08-11 13:58 ` Thomas Gleixner
  2024-08-11 18:22   ` pr-tracker-bot
  2024-08-11 13:58 ` [GIT pull] x86/urgent " Thomas Gleixner
  2024-08-11 18:22 ` [GIT pull] irq/urgent " pr-tracker-bot
  2 siblings, 1 reply; 13+ messages in thread
From: Thomas Gleixner @ 2024-08-11 13:58 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: linux-kernel, x86

Linus,

please pull the latest timers/urgent branch from:

   git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git timers-urgent-2024-08-11

up to:  5916be8a53de: timekeeping: Fix bogus clock_was_set() invocation in do_adjtimex()


Updates for time keeping:

  - Fix a couple of issues in the NTP code where user supplied values are
    neither sanity checked nor clamped to the operating range. This results
    in integer overflows and eventualy NTP getting out of sync.

    According to the history the sanity checks had been removed in favor of
    clamping the values, but the clamping never worked correctly under all
    circumstances. The NTP people asked to not bring the sanity checks back
    as it might break existing applications.

    Make the clamping work correctly and add it where it's missing

  - If adjtimex() sets the clock it has to trigger the hrtimer subsystem so
    it can adjust and if the clock was set into the future expire timers if
    needed. The caller should provide a bitmask to tell hrtimers which
    clocks have been adjusted. adjtimex() uses not the proper constant and
    uses CLOCK_REALTIME instead, which is 0. So hrtimers adjusts only the
    clocks, but does not check for expired timers, which might make them
    expire really late. Use the proper bitmask constant instead.


Thanks,

	tglx

------------------>
Justin Stitt (2):
      ntp: Clamp maxerror and esterror to operating range
      ntp: Safeguard against time_constant overflow

Thomas Gleixner (1):
      timekeeping: Fix bogus clock_was_set() invocation in do_adjtimex()


 kernel/time/ntp.c         | 9 ++++-----
 kernel/time/timekeeping.c | 2 +-
 2 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c
index 406dccb79c2b..8d2dd214ec68 100644
--- a/kernel/time/ntp.c
+++ b/kernel/time/ntp.c
@@ -727,17 +727,16 @@ static inline void process_adjtimex_modes(const struct __kernel_timex *txc,
 	}
 
 	if (txc->modes & ADJ_MAXERROR)
-		time_maxerror = txc->maxerror;
+		time_maxerror = clamp(txc->maxerror, 0, NTP_PHASE_LIMIT);
 
 	if (txc->modes & ADJ_ESTERROR)
-		time_esterror = txc->esterror;
+		time_esterror = clamp(txc->esterror, 0, NTP_PHASE_LIMIT);
 
 	if (txc->modes & ADJ_TIMECONST) {
-		time_constant = txc->constant;
+		time_constant = clamp(txc->constant, 0, MAXTC);
 		if (!(time_status & STA_NANO))
 			time_constant += 4;
-		time_constant = min(time_constant, (long)MAXTC);
-		time_constant = max(time_constant, 0l);
+		time_constant = clamp(time_constant, 0, MAXTC);
 	}
 
 	if (txc->modes & ADJ_TAI &&
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index 2fa87dcfeda9..5391e4167d60 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -2606,7 +2606,7 @@ int do_adjtimex(struct __kernel_timex *txc)
 		clock_set |= timekeeping_advance(TK_ADV_FREQ);
 
 	if (clock_set)
-		clock_was_set(CLOCK_REALTIME);
+		clock_was_set(CLOCK_SET_WALL);
 
 	ntp_notify_cmos_timer();
 


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [GIT pull] x86/urgent for v6.11-rc3
  2024-08-11 13:58 [GIT pull] irq/urgent for v6.11-rc3 Thomas Gleixner
  2024-08-11 13:58 ` [GIT pull] timers/urgent " Thomas Gleixner
@ 2024-08-11 13:58 ` Thomas Gleixner
  2024-08-11 17:30   ` Linus Torvalds
                     ` (2 more replies)
  2024-08-11 18:22 ` [GIT pull] irq/urgent " pr-tracker-bot
  2 siblings, 3 replies; 13+ messages in thread
From: Thomas Gleixner @ 2024-08-11 13:58 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: linux-kernel, x86

Linus,

please pull the latest x86/urgent branch from:

   git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git x86-urgent-2024-08-11

up to:  919f18f961c0: x86/mtrr: Check if fixed MTRRs exist before saving them

A set of x86 fixes:

  - Fix 32-bit PTI for real. pti_clone_entry_text() is called twice, once
    before initcalls so that initcalls can use the user-mode helper and
    then again after text is set read only. Setting read only on 32-bit
    might break up the PMD mapping, which makes the second invocation of
    pti_clone_entry_text() find the mappings out of sync and failing.

    Allow the second call to split the existing PMDs in the user mapping
    and synchronize with the kernel mapping.

  - Don't make acpi_mp_wake_mailbox not read only after init as the mail
    box must be writable in the case that CPU hotplug operations happen
    after boot. Otherwise the attempt to start a CPU crashes with a write
    to read only memory.

  - Add a missing sanity check in mtrr_save_state() to ensure that the
    fixed MTRR MSRs are supported.

    Otherwise mtrr_save_state() ends up in a #GP, which is fixed up, but
    the WARN_ON() can bring systems down when panic on warn is set.

Thanks,

	tglx

------------------>
Andi Kleen (1):
      x86/mtrr: Check if fixed MTRRs exist before saving them

Chen Yu (1):
      x86/paravirt: Fix incorrect virt spinlock setting on bare metal

Thomas Gleixner (1):
      x86/mm: Fix PTI for i386 some more

Zhiquan Li (1):
      x86/acpi: Remove __ro_after_init from acpi_mp_wake_mailbox


 arch/x86/include/asm/qspinlock.h   | 12 +++++-----
 arch/x86/kernel/acpi/madt_wakeup.c |  2 +-
 arch/x86/kernel/cpu/mtrr/mtrr.c    |  2 +-
 arch/x86/kernel/paravirt.c         |  7 +++---
 arch/x86/mm/pti.c                  | 45 ++++++++++++++++++++++++--------------
 5 files changed, 41 insertions(+), 27 deletions(-)

diff --git a/arch/x86/include/asm/qspinlock.h b/arch/x86/include/asm/qspinlock.h
index a053c1293975..68da67df304d 100644
--- a/arch/x86/include/asm/qspinlock.h
+++ b/arch/x86/include/asm/qspinlock.h
@@ -66,13 +66,15 @@ static inline bool vcpu_is_preempted(long cpu)
 
 #ifdef CONFIG_PARAVIRT
 /*
- * virt_spin_lock_key - enables (by default) the virt_spin_lock() hijack.
+ * virt_spin_lock_key - disables by default the virt_spin_lock() hijack.
  *
- * Native (and PV wanting native due to vCPU pinning) should disable this key.
- * It is done in this backwards fashion to only have a single direction change,
- * which removes ordering between native_pv_spin_init() and HV setup.
+ * Native (and PV wanting native due to vCPU pinning) should keep this key
+ * disabled. Native does not touch the key.
+ *
+ * When in a guest then native_pv_lock_init() enables the key first and
+ * KVM/XEN might conditionally disable it later in the boot process again.
  */
-DECLARE_STATIC_KEY_TRUE(virt_spin_lock_key);
+DECLARE_STATIC_KEY_FALSE(virt_spin_lock_key);
 
 /*
  * Shortcut for the queued_spin_lock_slowpath() function that allows
diff --git a/arch/x86/kernel/acpi/madt_wakeup.c b/arch/x86/kernel/acpi/madt_wakeup.c
index 6cfe762be28b..d5ef6215583b 100644
--- a/arch/x86/kernel/acpi/madt_wakeup.c
+++ b/arch/x86/kernel/acpi/madt_wakeup.c
@@ -19,7 +19,7 @@
 static u64 acpi_mp_wake_mailbox_paddr __ro_after_init;
 
 /* Virtual address of the Multiprocessor Wakeup Structure mailbox */
-static struct acpi_madt_multiproc_wakeup_mailbox *acpi_mp_wake_mailbox __ro_after_init;
+static struct acpi_madt_multiproc_wakeup_mailbox *acpi_mp_wake_mailbox;
 
 static u64 acpi_mp_pgd __ro_after_init;
 static u64 acpi_mp_reset_vector_paddr __ro_after_init;
diff --git a/arch/x86/kernel/cpu/mtrr/mtrr.c b/arch/x86/kernel/cpu/mtrr/mtrr.c
index 767bf1c71aad..2a2fc14955cd 100644
--- a/arch/x86/kernel/cpu/mtrr/mtrr.c
+++ b/arch/x86/kernel/cpu/mtrr/mtrr.c
@@ -609,7 +609,7 @@ void mtrr_save_state(void)
 {
 	int first_cpu;
 
-	if (!mtrr_enabled())
+	if (!mtrr_enabled() || !mtrr_state.have_fixed)
 		return;
 
 	first_cpu = cpumask_first(cpu_online_mask);
diff --git a/arch/x86/kernel/paravirt.c b/arch/x86/kernel/paravirt.c
index 5358d43886ad..fec381533555 100644
--- a/arch/x86/kernel/paravirt.c
+++ b/arch/x86/kernel/paravirt.c
@@ -51,13 +51,12 @@ DEFINE_ASM_FUNC(pv_native_irq_enable, "sti", .noinstr.text);
 DEFINE_ASM_FUNC(pv_native_read_cr2, "mov %cr2, %rax", .noinstr.text);
 #endif
 
-DEFINE_STATIC_KEY_TRUE(virt_spin_lock_key);
+DEFINE_STATIC_KEY_FALSE(virt_spin_lock_key);
 
 void __init native_pv_lock_init(void)
 {
-	if (IS_ENABLED(CONFIG_PARAVIRT_SPINLOCKS) &&
-	    !boot_cpu_has(X86_FEATURE_HYPERVISOR))
-		static_branch_disable(&virt_spin_lock_key);
+	if (boot_cpu_has(X86_FEATURE_HYPERVISOR))
+		static_branch_enable(&virt_spin_lock_key);
 }
 
 static void native_tlb_remove_table(struct mmu_gather *tlb, void *table)
diff --git a/arch/x86/mm/pti.c b/arch/x86/mm/pti.c
index bfdf5f45b137..851ec8f1363a 100644
--- a/arch/x86/mm/pti.c
+++ b/arch/x86/mm/pti.c
@@ -241,7 +241,7 @@ static pmd_t *pti_user_pagetable_walk_pmd(unsigned long address)
  *
  * Returns a pointer to a PTE on success, or NULL on failure.
  */
-static pte_t *pti_user_pagetable_walk_pte(unsigned long address)
+static pte_t *pti_user_pagetable_walk_pte(unsigned long address, bool late_text)
 {
 	gfp_t gfp = (GFP_KERNEL | __GFP_NOTRACK | __GFP_ZERO);
 	pmd_t *pmd;
@@ -251,10 +251,15 @@ static pte_t *pti_user_pagetable_walk_pte(unsigned long address)
 	if (!pmd)
 		return NULL;
 
-	/* We can't do anything sensible if we hit a large mapping. */
+	/* Large PMD mapping found */
 	if (pmd_leaf(*pmd)) {
-		WARN_ON(1);
-		return NULL;
+		/* Clear the PMD if we hit a large mapping from the first round */
+		if (late_text) {
+			set_pmd(pmd, __pmd(0));
+		} else {
+			WARN_ON_ONCE(1);
+			return NULL;
+		}
 	}
 
 	if (pmd_none(*pmd)) {
@@ -283,7 +288,7 @@ static void __init pti_setup_vsyscall(void)
 	if (!pte || WARN_ON(level != PG_LEVEL_4K) || pte_none(*pte))
 		return;
 
-	target_pte = pti_user_pagetable_walk_pte(VSYSCALL_ADDR);
+	target_pte = pti_user_pagetable_walk_pte(VSYSCALL_ADDR, false);
 	if (WARN_ON(!target_pte))
 		return;
 
@@ -301,7 +306,7 @@ enum pti_clone_level {
 
 static void
 pti_clone_pgtable(unsigned long start, unsigned long end,
-		  enum pti_clone_level level)
+		  enum pti_clone_level level, bool late_text)
 {
 	unsigned long addr;
 
@@ -390,7 +395,7 @@ pti_clone_pgtable(unsigned long start, unsigned long end,
 				return;
 
 			/* Allocate PTE in the user page-table */
-			target_pte = pti_user_pagetable_walk_pte(addr);
+			target_pte = pti_user_pagetable_walk_pte(addr, late_text);
 			if (WARN_ON(!target_pte))
 				return;
 
@@ -452,7 +457,7 @@ static void __init pti_clone_user_shared(void)
 		phys_addr_t pa = per_cpu_ptr_to_phys((void *)va);
 		pte_t *target_pte;
 
-		target_pte = pti_user_pagetable_walk_pte(va);
+		target_pte = pti_user_pagetable_walk_pte(va, false);
 		if (WARN_ON(!target_pte))
 			return;
 
@@ -475,7 +480,7 @@ static void __init pti_clone_user_shared(void)
 	start = CPU_ENTRY_AREA_BASE;
 	end   = start + (PAGE_SIZE * CPU_ENTRY_AREA_PAGES);
 
-	pti_clone_pgtable(start, end, PTI_CLONE_PMD);
+	pti_clone_pgtable(start, end, PTI_CLONE_PMD, false);
 }
 #endif /* CONFIG_X86_64 */
 
@@ -492,11 +497,11 @@ static void __init pti_setup_espfix64(void)
 /*
  * Clone the populated PMDs of the entry text and force it RO.
  */
-static void pti_clone_entry_text(void)
+static void pti_clone_entry_text(bool late)
 {
 	pti_clone_pgtable((unsigned long) __entry_text_start,
 			  (unsigned long) __entry_text_end,
-			  PTI_LEVEL_KERNEL_IMAGE);
+			  PTI_LEVEL_KERNEL_IMAGE, late);
 }
 
 /*
@@ -571,7 +576,7 @@ static void pti_clone_kernel_text(void)
 	 * pti_set_kernel_image_nonglobal() did to clear the
 	 * global bit.
 	 */
-	pti_clone_pgtable(start, end_clone, PTI_LEVEL_KERNEL_IMAGE);
+	pti_clone_pgtable(start, end_clone, PTI_LEVEL_KERNEL_IMAGE, false);
 
 	/*
 	 * pti_clone_pgtable() will set the global bit in any PMDs
@@ -638,8 +643,15 @@ void __init pti_init(void)
 
 	/* Undo all global bits from the init pagetables in head_64.S: */
 	pti_set_kernel_image_nonglobal();
+
 	/* Replace some of the global bits just for shared entry text: */
-	pti_clone_entry_text();
+	/*
+	 * This is very early in boot. Device and Late initcalls can do
+	 * modprobe before free_initmem() and mark_readonly(). This
+	 * pti_clone_entry_text() allows those user-mode-helpers to function,
+	 * but notably the text is still RW.
+	 */
+	pti_clone_entry_text(false);
 	pti_setup_espfix64();
 	pti_setup_vsyscall();
 }
@@ -656,10 +668,11 @@ void pti_finalize(void)
 	if (!boot_cpu_has(X86_FEATURE_PTI))
 		return;
 	/*
-	 * We need to clone everything (again) that maps parts of the
-	 * kernel image.
+	 * This is after free_initmem() (all initcalls are done) and we've done
+	 * mark_readonly(). Text is now NX which might've split some PMDs
+	 * relative to the early clone.
 	 */
-	pti_clone_entry_text();
+	pti_clone_entry_text(true);
 	pti_clone_kernel_text();
 
 	debug_checkwx_user();


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [GIT pull] x86/urgent for v6.11-rc3
  2024-08-11 13:58 ` [GIT pull] x86/urgent " Thomas Gleixner
@ 2024-08-11 17:30   ` Linus Torvalds
  2024-08-12  8:04     ` Thomas Gleixner
  2024-08-11 18:07   ` Linus Torvalds
  2024-08-11 18:22   ` [GIT pull] x86/urgent for v6.11-rc3 pr-tracker-bot
  2 siblings, 1 reply; 13+ messages in thread
From: Linus Torvalds @ 2024-08-11 17:30 UTC (permalink / raw)
  To: Thomas Gleixner; +Cc: linux-kernel, x86

On Sun, 11 Aug 2024 at 06:58, Thomas Gleixner <tglx@linutronix.de> wrote:
>
>   - Don't make acpi_mp_wake_mailbox not read only after init [..]

I assume there's one negation too many there, and it should just be
"Don't make acpi_mp_wake_mailbox read-only".

Also, while parsing the merge message and looking at the patch to make
sure I got it right, it struck me that it would have been really nice
to have found this "non-init function writes to __ro_after_init
variable" automatically.

Sadly, our section analysis is purely based on "this section has a
reference to that other section", and thus cannot see what kind of
access it is. And I'm not seeing how to try to figure that kind of
thing out without lots of work (ie objtool kind of stuff), so I guess
we're stuck with figuring these things out manually.

Oh well.

            Linus

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [GIT pull] x86/urgent for v6.11-rc3
  2024-08-11 13:58 ` [GIT pull] x86/urgent " Thomas Gleixner
  2024-08-11 17:30   ` Linus Torvalds
@ 2024-08-11 18:07   ` Linus Torvalds
  2024-08-12  8:21     ` Thomas Gleixner
  2024-08-11 18:22   ` [GIT pull] x86/urgent for v6.11-rc3 pr-tracker-bot
  2 siblings, 1 reply; 13+ messages in thread
From: Linus Torvalds @ 2024-08-11 18:07 UTC (permalink / raw)
  To: Thomas Gleixner; +Cc: linux-kernel, x86

On Sun, 11 Aug 2024 at 06:58, Thomas Gleixner <tglx@linutronix.de> wrote:
>
> Chen Yu (1):
>       x86/paravirt: Fix incorrect virt spinlock setting on bare metal

Oh, and while looking at this pull some more, and agreeing violently
with this change, I did notice that the code that *uses* the
virt_spin_lock_key is a bit odd:

  static inline bool virt_spin_lock(struct qspinlock *lock)

does this:

        if (!static_branch_likely(&virt_spin_lock_key))
                return false;

and this looks really odd to me.

Our static key code is pretty confusing, but we basically have

 - virt_spin_lock_key is now a "struct static_key_false", which means
that we consider the virt case the unlikely case.

   I agree whole-heartedly, because it's going to be the slow case
anyway, so this is good.

 - that means that 'static_branch_likely()' will generate a branch
(because the key is marked unlikely0

Isn't this wrong? So instead of falling through to the native
qspinlock case, we will branch to it, and we fall through to the
virt-spinlock case?

So i think that static_branch_likely() should have been changed to a
static_branch_unlikely() too, but it's possible that I've just
confused myself.

Anyway, somebody should double-check me.

I doubt it actually matters, since I think this all is fundamentally
just in the slow-path, so the "do a branch or a no-op" is likely
entirely in the noise even if I followed the code right. But it looked
off to me.

               Linus

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [GIT pull] irq/urgent for v6.11-rc3
  2024-08-11 13:58 [GIT pull] irq/urgent for v6.11-rc3 Thomas Gleixner
  2024-08-11 13:58 ` [GIT pull] timers/urgent " Thomas Gleixner
  2024-08-11 13:58 ` [GIT pull] x86/urgent " Thomas Gleixner
@ 2024-08-11 18:22 ` pr-tracker-bot
  2 siblings, 0 replies; 13+ messages in thread
From: pr-tracker-bot @ 2024-08-11 18:22 UTC (permalink / raw)
  To: Thomas Gleixner; +Cc: Linus Torvalds, linux-kernel, x86

The pull request you sent on Sun, 11 Aug 2024 15:58:05 +0200 (CEST):

> git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git irq-urgent-2024-08-11

has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/56fe0a6a9f8941b154bd6a41ed828e9e1078b67b

Thank you!

-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/prtracker.html

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [GIT pull] timers/urgent for v6.11-rc3
  2024-08-11 13:58 ` [GIT pull] timers/urgent " Thomas Gleixner
@ 2024-08-11 18:22   ` pr-tracker-bot
  0 siblings, 0 replies; 13+ messages in thread
From: pr-tracker-bot @ 2024-08-11 18:22 UTC (permalink / raw)
  To: Thomas Gleixner; +Cc: Linus Torvalds, linux-kernel, x86

The pull request you sent on Sun, 11 Aug 2024 15:58:06 +0200 (CEST):

> git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git timers-urgent-2024-08-11

has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/7270e931b53025c1070c2dda30a0ba7f1b2c072c

Thank you!

-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/prtracker.html

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [GIT pull] x86/urgent for v6.11-rc3
  2024-08-11 13:58 ` [GIT pull] x86/urgent " Thomas Gleixner
  2024-08-11 17:30   ` Linus Torvalds
  2024-08-11 18:07   ` Linus Torvalds
@ 2024-08-11 18:22   ` pr-tracker-bot
  2 siblings, 0 replies; 13+ messages in thread
From: pr-tracker-bot @ 2024-08-11 18:22 UTC (permalink / raw)
  To: Thomas Gleixner; +Cc: Linus Torvalds, linux-kernel, x86

The pull request you sent on Sun, 11 Aug 2024 15:58:08 +0200 (CEST):

> git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git x86-urgent-2024-08-11

has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/7006fe2f7f781fc96c8bab9df0c0417fd670a8e1

Thank you!

-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/prtracker.html

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [GIT pull] x86/urgent for v6.11-rc3
  2024-08-11 17:30   ` Linus Torvalds
@ 2024-08-12  8:04     ` Thomas Gleixner
  0 siblings, 0 replies; 13+ messages in thread
From: Thomas Gleixner @ 2024-08-12  8:04 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: linux-kernel, x86

On Sun, Aug 11 2024 at 10:30, Linus Torvalds wrote:

> On Sun, 11 Aug 2024 at 06:58, Thomas Gleixner <tglx@linutronix.de> wrote:
>>
>>   - Don't make acpi_mp_wake_mailbox not read only after init [..]
>
> I assume there's one negation too many there, and it should just be
> "Don't make acpi_mp_wake_mailbox read-only".

Right. It's way too hot here to think straight...

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [GIT pull] x86/urgent for v6.11-rc3
  2024-08-11 18:07   ` Linus Torvalds
@ 2024-08-12  8:21     ` Thomas Gleixner
  2024-08-13  3:15       ` Qiuxu Zhuo
  0 siblings, 1 reply; 13+ messages in thread
From: Thomas Gleixner @ 2024-08-12  8:21 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: linux-kernel, x86

On Sun, Aug 11 2024 at 11:07, Linus Torvalds wrote:
> On Sun, 11 Aug 2024 at 06:58, Thomas Gleixner <tglx@linutronix.de> wrote:
> Our static key code is pretty confusing, but we basically have
>
>  - virt_spin_lock_key is now a "struct static_key_false", which means
> that we consider the virt case the unlikely case.
>
>    I agree whole-heartedly, because it's going to be the slow case
> anyway, so this is good.
>
>  - that means that 'static_branch_likely()' will generate a branch
> (because the key is marked unlikely0
>
> Isn't this wrong? So instead of falling through to the native
> qspinlock case, we will branch to it, and we fall through to the
> virt-spinlock case?
>
> So i think that static_branch_likely() should have been changed to a
> static_branch_unlikely() too, but it's possible that I've just
> confused myself.

You are right. It creates a branch for the !virt_lock case.

> Anyway, somebody should double-check me.
>
> I doubt it actually matters, since I think this all is fundamentally
> just in the slow-path, so the "do a branch or a no-op" is likely
> entirely in the noise even if I followed the code right. But it looked
> off to me.

It is off and yes it won't matter much in the slowpath maze.

Thanks

        tglx

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [GIT pull] x86/urgent for v6.11-rc3
  2024-08-12  8:21     ` Thomas Gleixner
@ 2024-08-13  3:15       ` Qiuxu Zhuo
  2024-08-13 17:33         ` Thomas Gleixner
  0 siblings, 1 reply; 13+ messages in thread
From: Qiuxu Zhuo @ 2024-08-13  3:15 UTC (permalink / raw)
  To: tglx; +Cc: linux-kernel, torvalds, x86, qiuxu.zhuo

> From: Thomas Gleixner <tglx@linutronix.de>
> To: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: linux-kernel@vger.kernel.org, x86@kernel.org
> Subject: Re: [GIT pull] x86/urgent for v6.11-rc3
> 
> On Sun, Aug 11 2024 at 11:07, Linus Torvalds wrote:
> [...]
> > So i think that static_branch_likely() should have been changed to a
> > static_branch_unlikely() too, but it's possible that I've just
> > confused myself.
> 
> You are right. It creates a branch for the !virt_lock case.
> 
> > Anyway, somebody should double-check me.
> >
> > I doubt it actually matters, since I think this all is fundamentally
> > just in the slow-path, so the "do a branch or a no-op" is likely
> > entirely in the noise even if I followed the code right. But it looked
> > off to me.
> 
> It is off and yes it won't matter much in the slowpath maze.

static_branch_unlikely() matches that 'virt_spin_lock_key' is set to false 
in default after the commit,

  e639222a5119 ("x86/paravirt: Fix incorrect virt spinlock setting on bare metal")
  
although it offers little performance benefit since it is in the slow path.

Thomas, do you think it's worth making a patch to convert static_branch_likely()
to static_branch_unlikely() for this check, as suggested by Linus?
If so, I can assist with this.

Thanks!
-Qiuxu

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [GIT pull] x86/urgent for v6.11-rc3
  2024-08-13  3:15       ` Qiuxu Zhuo
@ 2024-08-13 17:33         ` Thomas Gleixner
  2024-08-14  3:14           ` [PATCH 1/1] x86/paravirt: Use static_branch_unlikely() for virt_spin_lock_key test Qiuxu Zhuo
  0 siblings, 1 reply; 13+ messages in thread
From: Thomas Gleixner @ 2024-08-13 17:33 UTC (permalink / raw)
  To: Qiuxu Zhuo; +Cc: linux-kernel, torvalds, x86, qiuxu.zhuo

On Tue, Aug 13 2024 at 11:15, Qiuxu Zhuo wrote:
>> From: Thomas Gleixner <tglx@linutronix.de>
>> On Sun, Aug 11 2024 at 11:07, Linus Torvalds wrote:
>> > I doubt it actually matters, since I think this all is fundamentally
>> > just in the slow-path, so the "do a branch or a no-op" is likely
>> > entirely in the noise even if I followed the code right. But it looked
>> > off to me.
>> 
>> It is off and yes it won't matter much in the slowpath maze.
>
> static_branch_unlikely() matches that 'virt_spin_lock_key' is set to false 
> in default after the commit,
>
>   e639222a5119 ("x86/paravirt: Fix incorrect virt spinlock setting on bare metal")
>   
> although it offers little performance benefit since it is in the slow path.
>
> Thomas, do you think it's worth making a patch to convert static_branch_likely()
> to static_branch_unlikely() for this check, as suggested by Linus?
> If so, I can assist with this.

Sure, but I'm not so sure that it actually matters and makes a
measurable difference.

Thanks,

        tglx

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 1/1] x86/paravirt: Use static_branch_unlikely() for virt_spin_lock_key test
  2024-08-13 17:33         ` Thomas Gleixner
@ 2024-08-14  3:14           ` Qiuxu Zhuo
  0 siblings, 0 replies; 13+ messages in thread
From: Qiuxu Zhuo @ 2024-08-14  3:14 UTC (permalink / raw)
  To: tglx; +Cc: torvalds, x86, linux-kernel, qiuxu.zhuo

Commit

  e639222a5119 ("x86/paravirt: Fix incorrect virt spinlock setting on bare metal")

sets virt_spin_lock_key to false by default. Use static_branch_unlikely() for
virt_spin_lock_key test.

Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
---
 arch/x86/include/asm/qspinlock.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/qspinlock.h b/arch/x86/include/asm/qspinlock.h
index 68da67df304d..b6f336a227da 100644
--- a/arch/x86/include/asm/qspinlock.h
+++ b/arch/x86/include/asm/qspinlock.h
@@ -89,7 +89,7 @@ static inline bool virt_spin_lock(struct qspinlock *lock)
 {
 	int val;
 
-	if (!static_branch_likely(&virt_spin_lock_key))
+	if (!static_branch_unlikely(&virt_spin_lock_key))
 		return false;
 
 	/*
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2024-08-14  3:27 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-08-11 13:58 [GIT pull] irq/urgent for v6.11-rc3 Thomas Gleixner
2024-08-11 13:58 ` [GIT pull] timers/urgent " Thomas Gleixner
2024-08-11 18:22   ` pr-tracker-bot
2024-08-11 13:58 ` [GIT pull] x86/urgent " Thomas Gleixner
2024-08-11 17:30   ` Linus Torvalds
2024-08-12  8:04     ` Thomas Gleixner
2024-08-11 18:07   ` Linus Torvalds
2024-08-12  8:21     ` Thomas Gleixner
2024-08-13  3:15       ` Qiuxu Zhuo
2024-08-13 17:33         ` Thomas Gleixner
2024-08-14  3:14           ` [PATCH 1/1] x86/paravirt: Use static_branch_unlikely() for virt_spin_lock_key test Qiuxu Zhuo
2024-08-11 18:22   ` [GIT pull] x86/urgent for v6.11-rc3 pr-tracker-bot
2024-08-11 18:22 ` [GIT pull] irq/urgent " pr-tracker-bot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox