From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail.mindbit.ro (xs1.mindbit.ro [80.86.107.70]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 389663F99EE for ; Thu, 2 Apr 2026 16:39:32 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=80.86.107.70 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1775147983; cv=none; b=OedOh89WX4idz/3TVGiNyMNNmk3LKy7QMDxv4VqsmsN8nHle+/nNwuFMRqhES0dsi3hksQsnC/QiHjmIbfxPpTN2X/62IbGya5fhD7VPNxOzBYZ7AUvysCowsLh4TEicx1R5GMA60z8M1jToKjopCElof5ynRKDQwES3kJ5iib0= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1775147983; c=relaxed/simple; bh=41tqnSuLzwHfuuFTMS+2HuLfTrQbU7Uqg5/a6ZW7wRE=; h=Message-ID:Subject:From:To:Cc:Date:In-Reply-To:References: Content-Type:MIME-Version; b=UJ7ujIGAX/Up7erzD+JGnBW/u+mJ2QxjomS1yb1p4y1EXfFqtSIXdl+4D28z3gb610UtXrEtei5wq2KDQGRZGJT5p431wmjCO5E+8y/LsZ3tihd1IsGaf5z8IkCwyaHpeTWNTvqYtxBk4TNmFE7Qgsj0eukpc/kidDQ5L0Qghmw= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=rendec.net; spf=pass smtp.mailfrom=rendec.net; dkim=pass (2048-bit key) header.d=rendec.net header.i=@rendec.net header.b=Y1SIkvUO; arc=none smtp.client-ip=80.86.107.70 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=rendec.net Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=rendec.net Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=rendec.net header.i=@rendec.net header.b="Y1SIkvUO" Received: from dog.kanata.rendec.net (pool-174-112-193-187.cpe.net.cable.rogers.com [174.112.193.187]) by mail.mindbit.ro (Postfix) with ESMTPSA id BB8EEC2540; Thu, 2 Apr 2026 19:39:23 +0300 (EEST) DKIM-Filter: OpenDKIM Filter v2.11.0 mail.mindbit.ro BB8EEC2540 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=rendec.net; s=default; t=1775147965; bh=ncc4kgD8iurioIwbtW5LfqjdaINlcve5c3fk7pCaYEE=; h=Subject:From:To:Cc:Date:In-Reply-To:References:From; b=Y1SIkvUOX2z5Qpdn4xYKI8B6h0FwVeDXALj0t+V844x8X/Y97SVGWvLleTLUHzzZB ZU4eVm+XERKYmgqeTsYlk3aG6BffhDaP6qSy736Pn+3BResRqUlSCOFt3O+8kjBUqW Awn5ZgJkgAXbXEhsJh/cKg6oCQ9XRbz5xeJhbxRmrVrWgBb0CgCrniGzLz9Orm0sMU UiRj1kjNGOQzktnkDqa85f43JW++8/lGhTWpEWxBFunNtE3dViTlB9QgZjvpJ3vu+m KISFwL9wGWiXrHbGJH+8NDmSsR9JBj+yn5mXeCs+SvTBd1WJko0WIUcP22mJx7Whgc Asc3+zrgtdawg== Message-ID: Subject: Re: [patch V5 05/15] x86/irq: Suppress unlikely interrupt stats by default From: Radu Rendec To: Thomas Gleixner , LKML Cc: x86@kernel.org, Michael Kelley , Dmitry Ilvokhin , Jan Kiszka , Kieran Bingham , Florian Fainelli , Marc Zyngier Date: Thu, 02 Apr 2026 12:39:21 -0400 In-Reply-To: <20260401201348.355938635@kernel.org> References: <20260401195625.213446764@kernel.org> <20260401201348.355938635@kernel.org> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable User-Agent: Evolution 3.56.2 (3.56.2-2.fc42) Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 It looks like we had a "race condition", and you haven't picked up my Reviewed-by tag on this patch on v4. There was a minor comment too. I'm re-including them here. On Wed, 2026-04-01 at 23:51 +0200, Thomas Gleixner wrote: > From: Thomas Gleixner >=20 > Unlikely interrupt counters like the spurious vector and the synthetic AP= IC > ICR read retry show up in /proc/interrupts with all counts 0 most of the > time. >=20 > As these are events which should never happen, suppress them by default a= nd > enable them for output when they actually happen. >=20 > This requires a seperate bitmap as the description array is marked > __ro_after_init. With that bitmap in place it becomes RO data. >=20 > Signed-off-by: Thomas Gleixner > --- > V5: Move irq_stat_inc_and_enable() here > V4: Fix the bad idea of writing to __ro_after_init marked data > V3: New patch > --- > =C2=A0arch/x86/include/asm/hardirq.h |=C2=A0=C2=A0=C2=A0 1 + > =C2=A0arch/x86/kernel/apic/apic.c=C2=A0=C2=A0=C2=A0 |=C2=A0=C2=A0=C2=A0 2= +- > =C2=A0arch/x86/kernel/apic/ipi.c=C2=A0=C2=A0=C2=A0=C2=A0 |=C2=A0=C2=A0=C2= =A0 2 +- > =C2=A0arch/x86/kernel/irq.c=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0 |=C2=A0=C2=A0 38 ++++++++++++++++++++++++++++---------- > =C2=A04 files changed, 31 insertions(+), 12 deletions(-) > --- a/arch/x86/include/asm/hardirq.h > +++ b/arch/x86/include/asm/hardirq.h > @@ -68,6 +68,7 @@ DECLARE_PER_CPU_ALIGNED(struct pi_desc, > =C2=A0#define __ARCH_IRQ_STAT > =C2=A0 > =C2=A0#define inc_irq_stat(index) this_cpu_inc(irq_stat.counts[IRQ_COUNT_= ##index]) > +void irq_stat_inc_and_enable(enum irq_stat_counts which); > =C2=A0 > =C2=A0#ifdef CONFIG_X86_LOCAL_APIC > =C2=A0#define inc_perf_irq_stat() inc_irq_stat(APIC_PERF) > --- a/arch/x86/kernel/apic/apic.c > +++ b/arch/x86/kernel/apic/apic.c > @@ -2108,7 +2108,7 @@ static noinline void handle_spurious_int > =C2=A0 > =C2=A0 trace_spurious_apic_entry(vector); > =C2=A0 > - inc_irq_stat(SPURIOUS); > + irq_stat_inc_and_enable(IRQ_COUNT_SPURIOUS); This is just a matter of style but for symmetry with inc_irq_stat(), I would prepend '__' to the function name and create a wrapper macro that adds the 'IRQ_COUNT_' prefix. And for consistency (you also have inc_perf_irq_stat()) I would call it inc_and_enable_irq_stat(). And yes, my comments above are totally bike shedding :) > =C2=A0 > =C2=A0 /* > =C2=A0 * If this is a spurious interrupt then do not acknowledge > --- a/arch/x86/kernel/apic/ipi.c > +++ b/arch/x86/kernel/apic/ipi.c > @@ -120,7 +120,7 @@ u32 apic_mem_wait_icr_idle_timeout(void) > =C2=A0 for (cnt =3D 0; cnt < 1000; cnt++) { > =C2=A0 if (!(apic_read(APIC_ICR) & APIC_ICR_BUSY)) > =C2=A0 return 0; > - inc_irq_stat(ICR_READ_RETRY); > + irq_stat_inc_and_enable(IRQ_COUNT_ICR_READ_RETRY); > =C2=A0 udelay(100); > =C2=A0 } > =C2=A0 return APIC_ICR_BUSY; > --- a/arch/x86/kernel/irq.c > +++ b/arch/x86/kernel/irq.c > @@ -69,19 +69,24 @@ struct irq_stat_info { > =C2=A0 const char *text; > =C2=A0}; > =C2=A0 > +#define DEFAULT_SUPPRESSED_VECTOR UINT_MAX > + > =C2=A0#define ISS(idx, sym, txt) [IRQ_COUNT_##idx] =3D { .symbol =3D sym,= .text =3D txt } > =C2=A0 > =C2=A0#define ITS(idx, sym, txt) [IRQ_COUNT_##idx] =3D \ > =C2=A0 { .skip_vector =3D idx## _VECTOR, .symbol =3D sym, .text =3D txt } > =C2=A0 > -static struct irq_stat_info irq_stat_info[IRQ_COUNT_MAX] __ro_after_init= =3D { > +#define IDS(idx, sym, txt) [IRQ_COUNT_##idx] =3D \ > + { .skip_vector =3D DEFAULT_SUPPRESSED_VECTOR, .symbol =3D sym, .text = =3D txt } > + > +static const struct irq_stat_info irq_stat_info[IRQ_COUNT_MAX] =3D { > =C2=A0 ISS(NMI, "NMI", "=C2=A0 Non-maskable interrupts\n"), > =C2=A0#ifdef CONFIG_X86_LOCAL_APIC > =C2=A0 ISS(APIC_TIMER, "LOC", "=C2=A0 Local timer interrupts\n"), > - ISS(SPURIOUS, "SPU", "=C2=A0 Spurious interrupts\n"), > + IDS(SPURIOUS, "SPU", "=C2=A0 Spurious interrupts\n"), > =C2=A0 ISS(APIC_PERF, "PMI", "=C2=A0 Performance monitoring interrupts\= n"), > =C2=A0 ISS(IRQ_WORK, "IWI", "=C2=A0 IRQ work interrupts\n"), > - ISS(ICR_READ_RETRY, "RTR", "=C2=A0 APIC ICR read retries\n"), > + IDS(ICR_READ_RETRY, "RTR", "=C2=A0 APIC ICR read retries\n"), > =C2=A0 ISS(X86_PLATFORM_IPI, "PLT", "=C2=A0 Platform interrupts\n"), > =C2=A0#endif > =C2=A0#ifdef CONFIG_SMP > @@ -122,34 +127,47 @@ static struct irq_stat_info irq_stat_inf > =C2=A0#endif > =C2=A0}; > =C2=A0 > +static DECLARE_BITMAP(irq_stat_count_show, IRQ_COUNT_MAX) __read_mostly; > + > =C2=A0static int __init irq_init_stats(void) > =C2=A0{ > - struct irq_stat_info *info =3D irq_stat_info; > + const struct irq_stat_info *info =3D irq_stat_info; > =C2=A0 > =C2=A0 for (unsigned int i =3D 0; i < ARRAY_SIZE(irq_stat_info); i++, inf= o++) { > - if (info->skip_vector && test_bit(info->skip_vector, system_vectors)) > - info->skip_vector =3D 0; > + if (!info->skip_vector || (info->skip_vector !=3D DEFAULT_SUPPRESSED_V= ECTOR && > + =C2=A0=C2=A0 test_bit(info->skip_vector, system_vectors))) > + set_bit(i, irq_stat_count_show); > =C2=A0 } > =C2=A0 > =C2=A0#ifdef CONFIG_X86_LOCAL_APIC > =C2=A0 if (!x86_platform_ipi_callback) > - irq_stat_info[IRQ_COUNT_X86_PLATFORM_IPI].skip_vector =3D 1; > + clear_bit(IRQ_COUNT_X86_PLATFORM_IPI, irq_stat_count_show); > =C2=A0#endif > =C2=A0 > =C2=A0#ifdef CONFIG_X86_POSTED_MSI > =C2=A0 if (!posted_msi_enabled()) > - irq_stat_info[IRQ_COUNT_POSTED_MSI_NOTIFICATION].skip_vector =3D 1; > + clear_bit(IRQ_COUNT_POSTED_MSI_NOTIFICATION, irq_stat_count_show); > =C2=A0#endif > =C2=A0 > =C2=A0#ifdef CONFIG_X86_MCE_AMD > =C2=A0 if (boot_cpu_data.x86_vendor !=3D X86_VENDOR_AMD && > =C2=A0 =C2=A0=C2=A0=C2=A0 boot_cpu_data.x86_vendor !=3D X86_VENDOR_HYGON) > - irq_stat_info[IRQ_COUNT_DEFERRED_ERROR].skip_vector =3D 1; > + clear_bit(IRQ_COUNT_DEFERRED_ERROR, irq_stat_count_show); > =C2=A0#endif > =C2=A0 return 0; > =C2=A0} > =C2=A0late_initcall(irq_init_stats); > =C2=A0 > +/* > + * Used for default enabled counters to increment the stats and to enabl= e the > + * entry for /proc/interrupts output. > + */ > +void irq_stat_inc_and_enable(enum irq_stat_counts which) > +{ > + this_cpu_inc(irq_stat.counts[which]); > + set_bit(which, irq_stat_count_show); > +} > + > =C2=A0#ifdef CONFIG_PROC_FS > =C2=A0/* > =C2=A0 * /proc/interrupts printing for arch specific interrupts > @@ -159,7 +177,7 @@ int arch_show_interrupts(struct seq_file > =C2=A0 const struct irq_stat_info *info =3D irq_stat_info; > =C2=A0 > =C2=A0 for (unsigned int i =3D 0; i < ARRAY_SIZE(irq_stat_info); i++, inf= o++) { > - if (info->skip_vector) > + if (!test_bit(i, irq_stat_count_show)) > =C2=A0 continue; > =C2=A0 > =C2=A0 seq_printf(p, "%*s:", prec, info->symbol); Reviewed-by: Radu Rendec