Received-SPF: pass (google.com: domain of linux-kernel+bounces-122742-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Precedence: bulk
MIME-Version: 1.0
References: <20240325035102.15245-1-adrianhuang0701@gmail.com>
 <874jcueydh.ffs@tglx> <CAHKZfL1xq-pemEMo7m8DctEVxSG_ASQWxCC_HsN=-92UAjP_Mw@mail.gmail.com>
 <87h6gsefae.ffs@tglx>
In-Reply-To: <87h6gsefae.ffs@tglx>
From: Huang Adrian <adrianhuang0701@gmail.com>
Date: Thu, 28 Mar 2024 18:03:02 +0800
Message-ID: <CAHKZfL1P+7FyhcYcxjgu7_R2uJAUz38bo1oh68hOYqZgtU73pw@mail.gmail.com>
Subject: Re: [PATCH 1/1] genirq/proc: Try to jump over the unallocated irq
 hole whenever possible
To: Thomas Gleixner <tglx@linutronix.de>
Cc: linux-kernel@vger.kernel.org, Jiwei Sun <sunjw10@lenovo.com>, 
	Adrian Huang <ahuang12@lenovo.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Wed, Mar 27, 2024 at 7:36=E2=80=AFAM Thomas Gleixner <tglx@linutronix.de=
> wrote:
>
> > @@ -19,6 +19,10 @@ static void *int_seq_next(struct seq_file *f, void
> > *v, loff_t *pos)
> [ 3 more citation lines. Click/Enter to show. ]
> >         (*pos)++;
> >         if (*pos > nr_irqs)
> >                 return NULL;
> > +
> > +       if (!irq_to_desc(*pos))
> > +               *pos =3D irq_get_next_irq(*pos);
>
> How is irq_get_next_irq() valid without either holding the sparse irq
> lock or RCU read lock?

Indeed, thanks for pointing this out. How about the patch below?

---
diff --git a/fs/proc/interrupts.c b/fs/proc/interrupts.c
index cb0edc7cb..111ea8a3c 100644
--- a/fs/proc/interrupts.c
+++ b/fs/proc/interrupts.c
@@ -19,6 +19,12 @@ static void *int_seq_next(struct seq_file *f, void
*v, loff_t *pos)
        (*pos)++;
        if (*pos > nr_irqs)
                return NULL;
+
+       rcu_read_lock();
+       if (!irq_to_desc(*pos))
+               *pos =3D irq_get_next_irq(*pos);
+       rcu_read_unlock();
+
        return pos;
 }


> But that aside you are missing a massive performance problem in the
> generic show_interrupts() code:
>
>         if (desc->kstat_irqs) {
>                 for_each_online_cpu(j)
>                         any_count |=3D data_race(*per_cpu_ptr(desc->kstat=
_irqs, j));
>         }
>
>         if ((!desc->action || irq_desc_is_chained(desc)) && !any_count)
>                 goto outsparse;
>

Yes, good point. The change is shown below.
BTW, I also made the change for iterating each cpu's irq stat. (No
need to check the percpu 'kstat_irqs' pointer for each iteration).

If you're ok, I'll send out the series.

---
diff --git a/kernel/irq/proc.c b/kernel/irq/proc.c
index 623b8136e..bfa341fac 100644
--- a/kernel/irq/proc.c
+++ b/kernel/irq/proc.c
@@ -461,7 +461,7 @@ int show_interrupts(struct seq_file *p, void *v)
 {
        static int prec;

-       unsigned long flags, any_count =3D 0;
+       unsigned long flags, print_irq =3D 1;
        int i =3D *(loff_t *) v, j;
        struct irqaction *action;
        struct irq_desc *desc;
@@ -488,18 +488,28 @@ int show_interrupts(struct seq_file *p, void *v)
        if (!desc || irq_settings_is_hidden(desc))
                goto outsparse;

-       if (desc->kstat_irqs) {
-               for_each_online_cpu(j)
-                       any_count |=3D
data_race(*per_cpu_ptr(desc->kstat_irqs, j));
+       if ((!desc->action || irq_desc_is_chained(desc)) && desc->kstat_irq=
s) {
+               print_irq =3D 0;
+               for_each_online_cpu(j) {
+                       if (data_race(*per_cpu_ptr(desc->kstat_irqs, j))) {
+                               print_irq =3D 1;
+                               break;
+                       }
+               }
        }

-       if ((!desc->action || irq_desc_is_chained(desc)) && !any_count)
+       if (!print_irq)
                goto outsparse;

        seq_printf(p, "%*d: ", prec, i);
-       for_each_online_cpu(j)
-               seq_printf(p, "%10u ", desc->kstat_irqs ?
-                                       *per_cpu_ptr(desc->kstat_irqs, j) :=
 0);
+
+       if (desc->kstat_irqs) {
+               for_each_online_cpu(j)
+                       seq_printf(p, "%10u ",
data_race(*per_cpu_ptr(desc->kstat_irqs, j)));
+       } else {
+               for_each_online_cpu(j)
+                       seq_printf(p, "%10u ", 0);
+       }

        raw_spin_lock_irqsave(&desc->lock, flags);
        if (desc->irq_data.chip) {


-- Adrian