Re: [PATCH] RISC-V: Show IPI stats

From: Anup Patel
Date: Wed Sep 12 2018 - 00:34:02 EST


On Mon, Sep 10, 2018 at 7:16 PM, Christoph Hellwig <hch@xxxxxxxxxxxxx> wrote:
> On Fri, Sep 07, 2018 at 06:14:29PM +0530, Anup Patel wrote:
>> This patch provides arch_show_interrupts() implementation to
>> show IPI stats via /proc/interrupts.
>>
>> Now the contents of /proc/interrupts" will look like below:
>> CPU0 CPU1 CPU2 CPU3
>> 8: 17 7 6 14 SiFive PLIC 8 virtio0
>> 10: 10 10 9 11 SiFive PLIC 10 ttyS0
>> IPI0: 170 673 251 79 Rescheduling interrupts
>> IPI1: 1 12 27 1 Function call interrupts
>> IPI2: 0 0 0 0 CPU wake-up interrupts
>>
>> Signed-off-by: Anup Patel <anup@xxxxxxxxxxxxxx>
>
> Thanks, this looks pretty sensible to me. Maybe we want to also show
> timer interrupts if we do this?

Let's not include timer stats here until RISCV INTC driver is
concluded. We can do it as separate patch if required.

>
>> --- a/arch/riscv/kernel/irq.c
>> +++ b/arch/riscv/kernel/irq.c
>> @@ -8,6 +8,7 @@
>> #include <linux/interrupt.h>
>> #include <linux/irqchip.h>
>> #include <linux/irqdomain.h>
>> +#include <linux/seq_file.h>
>>
>> /*
>> * Possible interrupt causes:
>> @@ -24,6 +25,14 @@
>> */
>> #define INTERRUPT_CAUSE_FLAG (1UL << (__riscv_xlen - 1))
>>
>> +int arch_show_interrupts(struct seq_file *p, int prec)
>> +{
>> +#ifdef CONFIG_SMP
>> + show_ipi_stats(p, prec);
>> +#endif
>> + return 0;
>> +}
>
> If we don't also add timer stats I'd just move arch_show_interrupts
> to smp.c and make it conditional. If we don't this split might make
> more sense.

I understand you want to avoid #ifdef here. We can do same thing
by having empty inline function show_ipi_stats() in asm/smp.h for
!CONFIG_SMP case. This way we can keep arch_show_interrupts()
in kernel/irq.c which is intuitively correct location for
arch_show_interrupts().

>
>> +static const char *ipi_names[IPI_MAX] = {
>> + [IPI_RESCHEDULE] = "Rescheduling interrupts",
>> + [IPI_CALL_FUNC] = "Function call interrupts",
>> + [IPI_CALL_WAKEUP] = "CPU wake-up interrupts",
>> +};
>
> No need for the explicit array size. Also please use a few tabs to
> align this nicely:
>
> static const char *ipi_names[] = {
> [IPI_RESCHEDULE] = "Rescheduling interrupts",
> [IPI_CALL_FUNC] = "Function call interrupts",
> [IPI_CALL_WAKEUP] = "CPU wake-up interrupts",
> };

Sure, will do.

Regards,
Anup