Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200426110740.123638-1-zong.li@sifive.com> <CAAhSdy3FCdzLV-nH03T=PBxB2tdZXhRrugcC2NcoA=22qpv+Lw@mail.gmail.com>
 <CANXhq0qW9ORoZ5qc5g8ikO9QdeYX=p0fwoP8pyFFkk02a7imnw@mail.gmail.com>
 <CAAhSdy2f2-SQP6TdgxA0HM2ft3eBJd6kEkB--RH=2gUuLktXLQ@mail.gmail.com>
 <CANXhq0pDYa2QfGZX87d5gyO5V2uzA3-ttPZXf7s1EkMUcG37Cw@mail.gmail.com>
 <CAAhSdy188_Kkfsz2bX05T3Rr12XDNtwGiwfqaT2TFVW7auGUaw@mail.gmail.com> <CANXhq0qGq33u34q7nhJE4GG03pZ8BBJrnusr=FgmTeJwtq-=4Q@mail.gmail.com>
In-Reply-To: <CANXhq0qGq33u34q7nhJE4GG03pZ8BBJrnusr=FgmTeJwtq-=4Q@mail.gmail.com>
From:   Greentime Hu <greentime.hu@sifive.com>
Date:   Mon, 27 Apr 2020 14:49:42 +0800
Message-ID: <CAHCEeh+FVYd6GKDuN4fXz9ku57vmdyVR1y_mzu89-sanNa_E3A@mail.gmail.com>
Subject: Re: [PATCH] irqchip/sifive-plic: allow many cores to handle IRQs
To:     Zong Li <zong.li@sifive.com>
Cc:     Anup Patel <anup@brainfault.org>,
        David Abdurachmanov <david.abdurachmanov@sifive.com>,
        Marc Zyngier <maz@kernel.org>,
        "linux-kernel@vger.kernel.org List" <linux-kernel@vger.kernel.org>,
        Palmer Dabbelt <palmer@dabbelt.com>,
        Paul Walmsley <paul.walmsley@sifive.com>,
        linux-riscv <linux-riscv@lists.infradead.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Zong Li <zong.li@sifive.com> =E6=96=BC 2020=E5=B9=B44=E6=9C=8826=E6=97=A5 =
=E9=80=B1=E6=97=A5 =E4=B8=8B=E5=8D=8811:35=E5=AF=AB=E9=81=93=EF=BC=9A
>
> On Sun, Apr 26, 2020 at 11:21 PM Anup Patel <anup@brainfault.org> wrote:
> >
> > On Sun, Apr 26, 2020 at 8:42 PM Zong Li <zong.li@sifive.com> wrote:
> > >
> > > On Sun, Apr 26, 2020 at 9:38 PM Anup Patel <anup@brainfault.org> wrot=
e:
> > > >
> > > > +Mark Z
> > > >
> > > > On Sun, Apr 26, 2020 at 6:49 PM Zong Li <zong.li@sifive.com> wrote:
> > > > >
> > > > > On Sun, Apr 26, 2020 at 8:47 PM Anup Patel <anup@brainfault.org> =
wrote:
> > > > > >
> > > > > > On Sun, Apr 26, 2020 at 4:37 PM Zong Li <zong.li@sifive.com> wr=
ote:
> > > > > > >
> > > > > > > Currently, driver forces the IRQs to be handled by only one c=
ore. This
> > > > > > > patch provides the way to enable others cores to handle IRQs =
if needed,
> > > > > > > so users could decide how many cores they wanted on default b=
y boot
> > > > > > > argument.
> > > > > > >
> > > > > > > Use 'irqaffinity' boot argument to determine affinity. If the=
re is no
> > > > > > > irqaffinity in dts or kernel configuration, use irq default a=
ffinity,
> > > > > > > so all harts would try to claim IRQ.
> > > > > > >
> > > > > > > For example, add irqaffinity=3D0 in chosen node to set irq af=
finity to
> > > > > > > hart 0. It also supports more than one harts to handle irq, s=
uch as set
> > > > > > > irqaffinity=3D0,3,4.
> > > > > > >
> > > > > > > You can change IRQ affinity from user-space using procfs. For=
 example,
> > > > > > > you can make CPU0 and CPU2 serve IRQ together by the followin=
g command:
> > > > > > >
> > > > > > > echo 4 > /proc/irq/<x>/smp_affinity
> > > > > > >
> > > > > > > Signed-off-by: Zong Li <zong.li@sifive.com>
> > > > > > > ---
> > > > > > >  drivers/irqchip/irq-sifive-plic.c | 21 +++++++--------------
> > > > > > >  1 file changed, 7 insertions(+), 14 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqc=
hip/irq-sifive-plic.c
> > > > > > > index d0a71febdadc..bc1440d54185 100644
> > > > > > > --- a/drivers/irqchip/irq-sifive-plic.c
> > > > > > > +++ b/drivers/irqchip/irq-sifive-plic.c
> > > > > > > @@ -111,15 +111,12 @@ static inline void plic_irq_toggle(cons=
t struct cpumask *mask,
> > > > > > >  static void plic_irq_unmask(struct irq_data *d)
> > > > > > >  {
> > > > > > >         struct cpumask amask;
> > > > > > > -       unsigned int cpu;
> > > > > > >         struct plic_priv *priv =3D irq_get_chip_data(d->irq);
> > > > > > >
> > > > > > >         cpumask_and(&amask, &priv->lmask, cpu_online_mask);
> > > > > > > -       cpu =3D cpumask_any_and(irq_data_get_affinity_mask(d)=
,
> > > > > > > -                                          &amask);
> > > > > > > -       if (WARN_ON_ONCE(cpu >=3D nr_cpu_ids))
> > > > > > > -               return;
> > > > > > > -       plic_irq_toggle(cpumask_of(cpu), d, 1);
> > > > > > > +       cpumask_and(&amask, &amask, irq_data_get_affinity_mas=
k(d));
> > > > > > > +
> > > > > > > +       plic_irq_toggle(&amask, d, 1);
> > > > > > >  }
> > > > > > >
> > > > > > >  static void plic_irq_mask(struct irq_data *d)
> > > > > > > @@ -133,24 +130,20 @@ static void plic_irq_mask(struct irq_da=
ta *d)
> > > > > > >  static int plic_set_affinity(struct irq_data *d,
> > > > > > >                              const struct cpumask *mask_val, =
bool force)
> > > > > > >  {
> > > > > > > -       unsigned int cpu;
> > > > > > >         struct cpumask amask;
> > > > > > >         struct plic_priv *priv =3D irq_get_chip_data(d->irq);
> > > > > > >
> > > > > > >         cpumask_and(&amask, &priv->lmask, mask_val);
> > > > > > >
> > > > > > >         if (force)
> > > > > > > -               cpu =3D cpumask_first(&amask);
> > > > > > > +               cpumask_copy(&amask, mask_val);
> > > > > > >         else
> > > > > > > -               cpu =3D cpumask_any_and(&amask, cpu_online_ma=
sk);
> > > > > > > -
> > > > > > > -       if (cpu >=3D nr_cpu_ids)
> > > > > > > -               return -EINVAL;
> > > > > > > +               cpumask_and(&amask, &amask, cpu_online_mask);
> > > > > > >
> > > > > > >         plic_irq_toggle(&priv->lmask, d, 0);
> > > > > > > -       plic_irq_toggle(cpumask_of(cpu), d, 1);
> > > > > > > +       plic_irq_toggle(&amask, d, 1);
> > > > > > >
> > > > > > > -       irq_data_update_effective_affinity(d, cpumask_of(cpu)=
);
> > > > > > > +       irq_data_update_effective_affinity(d, &amask);
> > > > > > >
> > > > > > >         return IRQ_SET_MASK_OK_DONE;
> > > > > > >  }
> > > > > > > --
> > > > > > > 2.26.1
> > > > > > >
> > > > > >
> > > > > > I strongly oppose (NACK) this patch due to performance reasons.
> > > > > >
> > > > > > In PLIC, if we enable an IRQ X for N CPUs then when IRQ X occur=
s:
> > > > > > 1) All N CPUs will take interrupt
> > > > > > 2) All N CPUs will try to read PLIC CLAIM register
> > > > > > 3) Only one of the CPUs will see IRQ X using the CLAIM register
> > > > > > but other N - 1 CPUs will see no interrupt and return back to w=
hat
> > > > > > they were doing. In other words, N - 1 CPUs will just waste CPU
> > > > > > every time IRQ X occurs.
> > > > > >
> > > > > > Example1, one Application doing heavy network traffic will
> > > > > > degrade performance of other applications because with every
> > > > > > network RX/TX interrupt N-1 CPUs will waste CPU trying to
> > > > > > process network interrupt.
> > > > > >
> > > > > > Example1, one Application doing heavy MMC/SD traffic will
> > > > > > degrade performance of other applications because with every
> > > > > > SPI read/write interrupt N-1 CPUs will waste CPU trying to
> > > > > > process it.
> > > > > >

Hi Anup,

If there is a case that there are a lot of interrupts from a single
irq number coming very quickly, the first hart is still serving the
first interrupt and still in its top half so it doesn't enable
interrupt yet but the second interrupt is coming and waiting, only the
rest of the harts can serve it. If they are masked, the interrupt
won't be able to be served it right away. In this case, I think this
patch can get better performance. Maybe we can still keep this patch
for user to define their usage in his case?