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

[Palmer Dabbelt]

On Fri, 28 Sep 2018 23:09:56 PDT (-0700), anup@xxxxxxxxxxxxxx wrote:
> On Sat, Sep 29, 2018 at 7:15 AM Palmer Dabbelt <palmer@xxxxxxxxxx> wrote:
> > On Mon, 10 Sep 2018 06:46:59 PDT (-0700), Christoph Hellwig 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?
> >
> > IIRC we used to have some issue where the timer interrupt ID in
> > /proc/interrupts aliased with a possible PLIC interrupt ID, but that was back
> > when we had a big mess of chained interrupt drivers that didn't really talk to
> > each other.  I think at some point I might have just removed the timer
> > interrupt from /proc/interrupts as a hack, but now that our interrupt
> > controller mess is sorted out it'd be better to have it.
>
> If we have irqdomain and irqchip for HLIC then showing timer interrupts
> becomes cleaner. My RISCV INTC patch achieves this.
>
> > I'm fine taking this without the timer interrupts, as something is better than
> > nothing.
>
> Sure.
>
> > >> --- 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.
> >
> > Makes sense, but I think timer interrupts are more interesting to see than IPIs
> > so we'll eventually pipe them through.  Might just be my workloads, though :)
> >
> > >> +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",
> > > };
> >
> > I don't see a v2 of this, was there one?  If not then I'll just clean up
> > ipi_names and drop this on for-next.
> >
> > Thanks for the patch!
>
> I will send v3 patch because IPI_CALL_WAKEUP is not going to be available.
>
> I will base v3 patch upon v5 of Atish's patchset. Atish will take v3 of this
> patch and make it part of his v6 patchset so that it will be easy for you
> to apply.

Thanks!