| Message ID | 20200624195811.435857-16-maz@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm/arm64: Turning IPIs into normal interrupts | expand |
On 24/06/20 20:58, Marc Zyngier wrote: > @@ -801,26 +802,15 @@ void show_ipi_list(struct seq_file *p, int prec) > unsigned int cpu, i; > > for (i = 0; i < NR_IPI; i++) { > + unsigned int irq = irq_desc_get_irq(ipi_desc[i]); > seq_printf(p, "%*s%u:%s", prec - 1, "IPI", i, > prec >= 4 ? " " : ""); > for_each_online_cpu(cpu) > - seq_printf(p, "%10u ", > - __get_irq_stat(cpu, ipi_irqs[i])); > + seq_printf(p, "%10u ", kstat_irqs_cpu(irq, cpu)); > seq_printf(p, " %s\n", ipi_types[i]); How attached are we to that custom IPI printout? AIUI we *could* give them a "prettier" name in request_percpu_irq() and let the standard procfs printout take the wheel.
On 2020-06-25 19:26, Valentin Schneider wrote: > On 24/06/20 20:58, Marc Zyngier wrote: >> @@ -801,26 +802,15 @@ void show_ipi_list(struct seq_file *p, int prec) >> unsigned int cpu, i; >> >> for (i = 0; i < NR_IPI; i++) { >> + unsigned int irq = irq_desc_get_irq(ipi_desc[i]); >> seq_printf(p, "%*s%u:%s", prec - 1, "IPI", i, >> prec >= 4 ? " " : ""); >> for_each_online_cpu(cpu) >> - seq_printf(p, "%10u ", >> - __get_irq_stat(cpu, ipi_irqs[i])); >> + seq_printf(p, "%10u ", kstat_irqs_cpu(irq, cpu)); >> seq_printf(p, " %s\n", ipi_types[i]); > > How attached are we to that custom IPI printout? AIUI we *could* give > them > a "prettier" name in request_percpu_irq() and let the standard procfs > printout take the wheel. I wish. Unfortunately, /proc/interrupts is likely to be considered ABI, and I'm really worried to change this (see what happened last time we tried to update /proc/cpuinfo to be less braindead...). M.
On 26/06/20 12:58, Marc Zyngier wrote: > On 2020-06-25 19:26, Valentin Schneider wrote: >> On 24/06/20 20:58, Marc Zyngier wrote: >>> @@ -801,26 +802,15 @@ void show_ipi_list(struct seq_file *p, int prec) >>> unsigned int cpu, i; >>> >>> for (i = 0; i < NR_IPI; i++) { >>> + unsigned int irq = irq_desc_get_irq(ipi_desc[i]); >>> seq_printf(p, "%*s%u:%s", prec - 1, "IPI", i, >>> prec >= 4 ? " " : ""); >>> for_each_online_cpu(cpu) >>> - seq_printf(p, "%10u ", >>> - __get_irq_stat(cpu, ipi_irqs[i])); >>> + seq_printf(p, "%10u ", kstat_irqs_cpu(irq, cpu)); >>> seq_printf(p, " %s\n", ipi_types[i]); >> >> How attached are we to that custom IPI printout? AIUI we *could* give >> them >> a "prettier" name in request_percpu_irq() and let the standard procfs >> printout take the wheel. > > I wish. Unfortunately, /proc/interrupts is likely to be considered ABI, > and I'm really worried to change this (see what happened last time we > tried to update /proc/cpuinfo to be less braindead...). > Hmph, it's borderline here I think: we're not introducing a new field or format in the file, so any tool that can parse /proc/interrupts can parse the IPIs if they are formatted like the other "regular" interrupts. But then said tool could die in flames when not seeing the special IPI fields because sturdiness is overrated :( Oh well. > M.
On 2020-06-27 00:15, Valentin Schneider wrote: > On 26/06/20 12:58, Marc Zyngier wrote: >> On 2020-06-25 19:26, Valentin Schneider wrote: >>> On 24/06/20 20:58, Marc Zyngier wrote: >>>> @@ -801,26 +802,15 @@ void show_ipi_list(struct seq_file *p, int >>>> prec) >>>> unsigned int cpu, i; >>>> >>>> for (i = 0; i < NR_IPI; i++) { >>>> + unsigned int irq = irq_desc_get_irq(ipi_desc[i]); >>>> seq_printf(p, "%*s%u:%s", prec - 1, "IPI", i, >>>> prec >= 4 ? " " : ""); >>>> for_each_online_cpu(cpu) >>>> - seq_printf(p, "%10u ", >>>> - __get_irq_stat(cpu, ipi_irqs[i])); >>>> + seq_printf(p, "%10u ", kstat_irqs_cpu(irq, cpu)); >>>> seq_printf(p, " %s\n", ipi_types[i]); >>> >>> How attached are we to that custom IPI printout? AIUI we *could* give >>> them >>> a "prettier" name in request_percpu_irq() and let the standard procfs >>> printout take the wheel. >> >> I wish. Unfortunately, /proc/interrupts is likely to be considered >> ABI, >> and I'm really worried to change this (see what happened last time we >> tried to update /proc/cpuinfo to be less braindead...). >> > > Hmph, it's borderline here I think: we're not introducing a new field > or > format in the file, so any tool that can parse /proc/interrupts can > parse > the IPIs if they are formatted like the other "regular" interrupts. But > then said tool could die in flames when not seeing the special IPI > fields > because sturdiness is overrated :( Which is exactly what I'm worried about. People do stupid things, and stupidity becomes ABI. I hate luserspace. M.
On 24/06/20 20:58, Marc Zyngier wrote: > Let's switch the arm64 code to the core accounting, which already > does everything we need. > > Signed-off-by: Marc Zyngier <maz@kernel.org> > --- > arch/arm64/include/asm/hardirq.h | 9 --------- > arch/arm64/kernel/smp.c | 24 ++++++------------------ > 2 files changed, 6 insertions(+), 27 deletions(-) > diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c > index ed8a8184e3b6..8b903ceef2be 100644 > --- a/arch/arm64/kernel/smp.c > +++ b/arch/arm64/kernel/smp.c > @@ -991,7 +979,7 @@ void __init set_smp_ipi_range(int ipi_base, int n) > int err; > > err = request_percpu_irq(ipi_base + i, ipi_handler, > - "IPI", &irq_stat); > + "IPI", &cpu_number); That pointer remains unused in ipi_handler(); I suppose it's just a matter of cosmetics? I see that we leave it as irq_stat in arm, I suppose simply because we don't have a cpu_number equivalent there and need *some* percpu variable for the request to happen. > WARN_ON(err); > > ipi_desc[i] = irq_to_desc(ipi_base + i);
diff --git a/arch/arm64/include/asm/hardirq.h b/arch/arm64/include/asm/hardirq.h index 985493af704b..5ffa4bacdad3 100644 --- a/arch/arm64/include/asm/hardirq.h +++ b/arch/arm64/include/asm/hardirq.h @@ -13,21 +13,12 @@ #include <asm/kvm_arm.h> #include <asm/sysreg.h> -#define NR_IPI 7 - typedef struct { unsigned int __softirq_pending; - unsigned int ipi_irqs[NR_IPI]; } ____cacheline_aligned irq_cpustat_t; #include <linux/irq_cpustat.h> /* Standard mappings for irq_cpustat_t above */ -#define __inc_irq_stat(cpu, member) __IRQ_STAT(cpu, member)++ -#define __get_irq_stat(cpu, member) __IRQ_STAT(cpu, member) - -u64 smp_irq_stat_cpu(unsigned int cpu); -#define arch_irq_stat_cpu smp_irq_stat_cpu - #define __ARCH_IRQ_EXIT_IRQS_DISABLED 1 struct nmi_ctx { diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c index ed8a8184e3b6..8b903ceef2be 100644 --- a/arch/arm64/kernel/smp.c +++ b/arch/arm64/kernel/smp.c @@ -73,7 +73,8 @@ enum ipi_msg_type { IPI_CPU_CRASH_STOP, IPI_TIMER, IPI_IRQ_WORK, - IPI_WAKEUP + IPI_WAKEUP, + NR_IPI }; static int ipi_irq_base; @@ -801,26 +802,15 @@ void show_ipi_list(struct seq_file *p, int prec) unsigned int cpu, i; for (i = 0; i < NR_IPI; i++) { + unsigned int irq = irq_desc_get_irq(ipi_desc[i]); seq_printf(p, "%*s%u:%s", prec - 1, "IPI", i, prec >= 4 ? " " : ""); for_each_online_cpu(cpu) - seq_printf(p, "%10u ", - __get_irq_stat(cpu, ipi_irqs[i])); + seq_printf(p, "%10u ", kstat_irqs_cpu(irq, cpu)); seq_printf(p, " %s\n", ipi_types[i]); } } -u64 smp_irq_stat_cpu(unsigned int cpu) -{ - u64 sum = 0; - int i; - - for (i = 0; i < NR_IPI; i++) - sum += __get_irq_stat(cpu, ipi_irqs[i]); - - return sum; -} - void arch_send_call_function_ipi_mask(const struct cpumask *mask) { smp_cross_call(mask, IPI_CALL_FUNC); @@ -893,10 +883,8 @@ static void do_handle_IPI(int ipinr) { unsigned int cpu = smp_processor_id(); - if ((unsigned)ipinr < NR_IPI) { + if ((unsigned)ipinr < NR_IPI) trace_ipi_entry_rcuidle(ipi_types[ipinr]); - __inc_irq_stat(cpu, ipi_irqs[ipinr]); - } switch (ipinr) { case IPI_RESCHEDULE: @@ -991,7 +979,7 @@ void __init set_smp_ipi_range(int ipi_base, int n) int err; err = request_percpu_irq(ipi_base + i, ipi_handler, - "IPI", &irq_stat); + "IPI", &cpu_number); WARN_ON(err); ipi_desc[i] = irq_to_desc(ipi_base + i);
Let's switch the arm64 code to the core accounting, which already does everything we need. Signed-off-by: Marc Zyngier <maz@kernel.org> --- arch/arm64/include/asm/hardirq.h | 9 --------- arch/arm64/kernel/smp.c | 24 ++++++------------------ 2 files changed, 6 insertions(+), 27 deletions(-)