[PATCH] irqchip/sifive-plic: allow many cores to handle IRQs

[PATCH] irqchip/sifive-plic: allow many cores to handle IRQs

[Zong Li]

Currently, driver forces the IRQs to be handled by only one core. 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 by boot
argument.

Use 'irqaffinity' boot argument to determine affinity. If there is no
irqaffinity in dts or kernel configuration, use irq default affinity,
so all harts would try to claim IRQ.

For example, add irqaffinity=0 in chosen node to set irq affinity to
hart 0. It also supports more than one harts to handle irq, such as set
irqaffinity=0,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 following 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/irqchip/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(const struct cpumask *mask,
 static void plic_irq_unmask(struct irq_data *d)
 {
 	struct cpumask amask;
-	unsigned int cpu;
 	struct plic_priv *priv = irq_get_chip_data(d->irq);
 
 	cpumask_and(&amask, &priv->lmask, cpu_online_mask);
-	cpu = cpumask_any_and(irq_data_get_affinity_mask(d),
-					   &amask);
-	if (WARN_ON_ONCE(cpu >= nr_cpu_ids))
-		return;
-	plic_irq_toggle(cpumask_of(cpu), d, 1);
+	cpumask_and(&amask, &amask, irq_data_get_affinity_mask(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_data *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 = irq_get_chip_data(d->irq);
 
 	cpumask_and(&amask, &priv->lmask, mask_val);
 
 	if (force)
-		cpu = cpumask_first(&amask);
+		cpumask_copy(&amask, mask_val);
 	else
-		cpu = cpumask_any_and(&amask, cpu_online_mask);
-
-	if (cpu >= 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

Re: [PATCH] irqchip/sifive-plic: allow many cores to handle IRQs

[Anup Patel]

On Sun, Apr 26, 2020 at 4:37 PM Zong Li <zong.li@sifive.com> wrote:
>
> Currently, driver forces the IRQs to be handled by only one core. 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 by boot
> argument.
>
> Use 'irqaffinity' boot argument to determine affinity. If there is no
> irqaffinity in dts or kernel configuration, use irq default affinity,
> so all harts would try to claim IRQ.
>
> For example, add irqaffinity=0 in chosen node to set irq affinity to
> hart 0. It also supports more than one harts to handle irq, such as set
> irqaffinity=0,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 following 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/irqchip/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(const struct cpumask *mask,
>  static void plic_irq_unmask(struct irq_data *d)
>  {
>         struct cpumask amask;
> -       unsigned int cpu;
>         struct plic_priv *priv = irq_get_chip_data(d->irq);
>
>         cpumask_and(&amask, &priv->lmask, cpu_online_mask);
> -       cpu = cpumask_any_and(irq_data_get_affinity_mask(d),
> -                                          &amask);
> -       if (WARN_ON_ONCE(cpu >= nr_cpu_ids))
> -               return;
> -       plic_irq_toggle(cpumask_of(cpu), d, 1);
> +       cpumask_and(&amask, &amask, irq_data_get_affinity_mask(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_data *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 = irq_get_chip_data(d->irq);
>
>         cpumask_and(&amask, &priv->lmask, mask_val);
>
>         if (force)
> -               cpu = cpumask_first(&amask);
> +               cpumask_copy(&amask, mask_val);
>         else
> -               cpu = cpumask_any_and(&amask, cpu_online_mask);
> -
> -       if (cpu >= 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 occurs:
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 what
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.

In fact, the current PLIC approach is actually a performance
optimization. This implementation also works fine with in-kernel
load-balancer and user space load balancer.

Regards,
Anup

Re: [PATCH] irqchip/sifive-plic: allow many cores to handle IRQs

[Zong Li]

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> wrote:
> >
> > Currently, driver forces the IRQs to be handled by only one core. 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 by boot
> > argument.
> >
> > Use 'irqaffinity' boot argument to determine affinity. If there is no
> > irqaffinity in dts or kernel configuration, use irq default affinity,
> > so all harts would try to claim IRQ.
> >
> > For example, add irqaffinity=0 in chosen node to set irq affinity to
> > hart 0. It also supports more than one harts to handle irq, such as set
> > irqaffinity=0,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 following 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/irqchip/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(const struct cpumask *mask,
> >  static void plic_irq_unmask(struct irq_data *d)
> >  {
> >         struct cpumask amask;
> > -       unsigned int cpu;
> >         struct plic_priv *priv = irq_get_chip_data(d->irq);
> >
> >         cpumask_and(&amask, &priv->lmask, cpu_online_mask);
> > -       cpu = cpumask_any_and(irq_data_get_affinity_mask(d),
> > -                                          &amask);
> > -       if (WARN_ON_ONCE(cpu >= nr_cpu_ids))
> > -               return;
> > -       plic_irq_toggle(cpumask_of(cpu), d, 1);
> > +       cpumask_and(&amask, &amask, irq_data_get_affinity_mask(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_data *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 = irq_get_chip_data(d->irq);
> >
> >         cpumask_and(&amask, &priv->lmask, mask_val);
> >
> >         if (force)
> > -               cpu = cpumask_first(&amask);
> > +               cpumask_copy(&amask, mask_val);
> >         else
> > -               cpu = cpumask_any_and(&amask, cpu_online_mask);
> > -
> > -       if (cpu >= 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 occurs:
> 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 what
> 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.
>
> In fact, the current PLIC approach is actually a performance
> optimization. This implementation also works fine with in-kernel
> load-balancer and user space load balancer.
>

Yes, it's exactly, I know what you pointed out. But the idea of this
patch is just providing a way that users could enable other cores if
they wanted, it could still enable only one core by this change. The
purpose here is thinking of flexible use, rather than limitation.
Maybe it would be a happy medium that we make the default case enable
only one core? It is a good open discussion.

> Regards,
> Anup

Re: [PATCH] irqchip/sifive-plic: allow many cores to handle IRQs

[Anup Patel]

+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> wrote:
> > >
> > > Currently, driver forces the IRQs to be handled by only one core. 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 by boot
> > > argument.
> > >
> > > Use 'irqaffinity' boot argument to determine affinity. If there is no
> > > irqaffinity in dts or kernel configuration, use irq default affinity,
> > > so all harts would try to claim IRQ.
> > >
> > > For example, add irqaffinity=0 in chosen node to set irq affinity to
> > > hart 0. It also supports more than one harts to handle irq, such as set
> > > irqaffinity=0,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 following 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/irqchip/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(const struct cpumask *mask,
> > >  static void plic_irq_unmask(struct irq_data *d)
> > >  {
> > >         struct cpumask amask;
> > > -       unsigned int cpu;
> > >         struct plic_priv *priv = irq_get_chip_data(d->irq);
> > >
> > >         cpumask_and(&amask, &priv->lmask, cpu_online_mask);
> > > -       cpu = cpumask_any_and(irq_data_get_affinity_mask(d),
> > > -                                          &amask);
> > > -       if (WARN_ON_ONCE(cpu >= nr_cpu_ids))
> > > -               return;
> > > -       plic_irq_toggle(cpumask_of(cpu), d, 1);
> > > +       cpumask_and(&amask, &amask, irq_data_get_affinity_mask(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_data *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 = irq_get_chip_data(d->irq);
> > >
> > >         cpumask_and(&amask, &priv->lmask, mask_val);
> > >
> > >         if (force)
> > > -               cpu = cpumask_first(&amask);
> > > +               cpumask_copy(&amask, mask_val);
> > >         else
> > > -               cpu = cpumask_any_and(&amask, cpu_online_mask);
> > > -
> > > -       if (cpu >= 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 occurs:
> > 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 what
> > 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.
> >
> > In fact, the current PLIC approach is actually a performance
> > optimization. This implementation also works fine with in-kernel
> > load-balancer and user space load balancer.
> >
>
> Yes, it's exactly, I know what you pointed out. But the idea of this
> patch is just providing a way that users could enable other cores if
> they wanted, it could still enable only one core by this change. The
> purpose here is thinking of flexible use, rather than limitation.
> Maybe it would be a happy medium that we make the default case enable
> only one core? It is a good open discussion.

Making the default case as enable only one core is just a work-around.

As-per my understanding, if we set affinity mask of N CPUs for IRQ X
then it does not mean all N CPUs should receive IRQ X rather it means
that exactly one of the N CPUs will receive IRQ X and the IRQ receiving
CPU will be fixed (reflected by effective affinity returned by the driver).

If we ignore above semantics and still provide a mechanism to target
IRQ X to N CPUs then most likely someone will try and run into
performance issues.

Please don't go this path. The performance impact in case of Guest/VM
is even worst because PLIC is trap-n-emulated by hypervisors as MMIO
device.

Regards,
Anup

Re: [PATCH] irqchip/sifive-plic: allow many cores to handle IRQs

[Zong Li]

On Sun, Apr 26, 2020 at 9:38 PM Anup Patel <anup@brainfault.org> wrote:
>
> +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> wrote:
> > > >
> > > > Currently, driver forces the IRQs to be handled by only one core. 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 by boot
> > > > argument.
> > > >
> > > > Use 'irqaffinity' boot argument to determine affinity. If there is no
> > > > irqaffinity in dts or kernel configuration, use irq default affinity,
> > > > so all harts would try to claim IRQ.
> > > >
> > > > For example, add irqaffinity=0 in chosen node to set irq affinity to
> > > > hart 0. It also supports more than one harts to handle irq, such as set
> > > > irqaffinity=0,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 following 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/irqchip/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(const struct cpumask *mask,
> > > >  static void plic_irq_unmask(struct irq_data *d)
> > > >  {
> > > >         struct cpumask amask;
> > > > -       unsigned int cpu;
> > > >         struct plic_priv *priv = irq_get_chip_data(d->irq);
> > > >
> > > >         cpumask_and(&amask, &priv->lmask, cpu_online_mask);
> > > > -       cpu = cpumask_any_and(irq_data_get_affinity_mask(d),
> > > > -                                          &amask);
> > > > -       if (WARN_ON_ONCE(cpu >= nr_cpu_ids))
> > > > -               return;
> > > > -       plic_irq_toggle(cpumask_of(cpu), d, 1);
> > > > +       cpumask_and(&amask, &amask, irq_data_get_affinity_mask(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_data *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 = irq_get_chip_data(d->irq);
> > > >
> > > >         cpumask_and(&amask, &priv->lmask, mask_val);
> > > >
> > > >         if (force)
> > > > -               cpu = cpumask_first(&amask);
> > > > +               cpumask_copy(&amask, mask_val);
> > > >         else
> > > > -               cpu = cpumask_any_and(&amask, cpu_online_mask);
> > > > -
> > > > -       if (cpu >= 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 occurs:
> > > 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 what
> > > 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.
> > >
> > > In fact, the current PLIC approach is actually a performance
> > > optimization. This implementation also works fine with in-kernel
> > > load-balancer and user space load balancer.
> > >
> >
> > Yes, it's exactly, I know what you pointed out. But the idea of this
> > patch is just providing a way that users could enable other cores if
> > they wanted, it could still enable only one core by this change. The
> > purpose here is thinking of flexible use, rather than limitation.
> > Maybe it would be a happy medium that we make the default case enable
> > only one core? It is a good open discussion.
>
> Making the default case as enable only one core is just a work-around.
>
> As-per my understanding, if we set affinity mask of N CPUs for IRQ X
> then it does not mean all N CPUs should receive IRQ X rather it means
> that exactly one of the N CPUs will receive IRQ X and the IRQ receiving
> CPU will be fixed (reflected by effective affinity returned by the driver).

is there a case that we only bundle the IRQ to CPU0, but CPU0 is more
much busy than other CPUs, and it would be better if another CPU could
take the IRQ?

>
> If we ignore above semantics and still provide a mechanism to target
> IRQ X to N CPUs then most likely someone will try and run into
> performance issues.
>
> Please don't go this path. The performance impact in case of Guest/VM
> is even worst because PLIC is trap-n-emulated by hypervisors as MMIO
> device.

OK, I won't persist in that, just wanna figure out the situation.

>
> Regards,
> Anup

Re: [PATCH] irqchip/sifive-plic: allow many cores to handle IRQs

[Anup Patel]

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> wrote:
> >
> > +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> wrote:
> > > > >
> > > > > Currently, driver forces the IRQs to be handled by only one core. 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 by boot
> > > > > argument.
> > > > >
> > > > > Use 'irqaffinity' boot argument to determine affinity. If there is no
> > > > > irqaffinity in dts or kernel configuration, use irq default affinity,
> > > > > so all harts would try to claim IRQ.
> > > > >
> > > > > For example, add irqaffinity=0 in chosen node to set irq affinity to
> > > > > hart 0. It also supports more than one harts to handle irq, such as set
> > > > > irqaffinity=0,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 following 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/irqchip/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(const struct cpumask *mask,
> > > > >  static void plic_irq_unmask(struct irq_data *d)
> > > > >  {
> > > > >         struct cpumask amask;
> > > > > -       unsigned int cpu;
> > > > >         struct plic_priv *priv = irq_get_chip_data(d->irq);
> > > > >
> > > > >         cpumask_and(&amask, &priv->lmask, cpu_online_mask);
> > > > > -       cpu = cpumask_any_and(irq_data_get_affinity_mask(d),
> > > > > -                                          &amask);
> > > > > -       if (WARN_ON_ONCE(cpu >= nr_cpu_ids))
> > > > > -               return;
> > > > > -       plic_irq_toggle(cpumask_of(cpu), d, 1);
> > > > > +       cpumask_and(&amask, &amask, irq_data_get_affinity_mask(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_data *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 = irq_get_chip_data(d->irq);
> > > > >
> > > > >         cpumask_and(&amask, &priv->lmask, mask_val);
> > > > >
> > > > >         if (force)
> > > > > -               cpu = cpumask_first(&amask);
> > > > > +               cpumask_copy(&amask, mask_val);
> > > > >         else
> > > > > -               cpu = cpumask_any_and(&amask, cpu_online_mask);
> > > > > -
> > > > > -       if (cpu >= 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 occurs:
> > > > 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 what
> > > > 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.
> > > >
> > > > In fact, the current PLIC approach is actually a performance
> > > > optimization. This implementation also works fine with in-kernel
> > > > load-balancer and user space load balancer.
> > > >
> > >
> > > Yes, it's exactly, I know what you pointed out. But the idea of this
> > > patch is just providing a way that users could enable other cores if
> > > they wanted, it could still enable only one core by this change. The
> > > purpose here is thinking of flexible use, rather than limitation.
> > > Maybe it would be a happy medium that we make the default case enable
> > > only one core? It is a good open discussion.
> >
> > Making the default case as enable only one core is just a work-around.
> >
> > As-per my understanding, if we set affinity mask of N CPUs for IRQ X
> > then it does not mean all N CPUs should receive IRQ X rather it means
> > that exactly one of the N CPUs will receive IRQ X and the IRQ receiving
> > CPU will be fixed (reflected by effective affinity returned by the driver).
>
> is there a case that we only bundle the IRQ to CPU0, but CPU0 is more
> much busy than other CPUs, and it would be better if another CPU could
> take the IRQ?

This is a common problem across architectures.

To tackle this, we typically run irqbalance daemon in user-space which will
change IRQ affinity based on CPU load.

Refer, https://linux.die.net/man/1/irqbalance

>
> >
> > If we ignore above semantics and still provide a mechanism to target
> > IRQ X to N CPUs then most likely someone will try and run into
> > performance issues.
> >
> > Please don't go this path. The performance impact in case of Guest/VM
> > is even worst because PLIC is trap-n-emulated by hypervisors as MMIO
> > device.
>
> OK, I won't persist in that, just wanna figure out the situation.
>
> >
> > Regards,
> > Anup

Regards,
Anup

Re: [PATCH] irqchip/sifive-plic: allow many cores to handle IRQs

[Zong Li]

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> wrote:
> > >
> > > +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> wrote:
> > > > > >
> > > > > > Currently, driver forces the IRQs to be handled by only one core. 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 by boot
> > > > > > argument.
> > > > > >
> > > > > > Use 'irqaffinity' boot argument to determine affinity. If there is no
> > > > > > irqaffinity in dts or kernel configuration, use irq default affinity,
> > > > > > so all harts would try to claim IRQ.
> > > > > >
> > > > > > For example, add irqaffinity=0 in chosen node to set irq affinity to
> > > > > > hart 0. It also supports more than one harts to handle irq, such as set
> > > > > > irqaffinity=0,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 following 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/irqchip/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(const struct cpumask *mask,
> > > > > >  static void plic_irq_unmask(struct irq_data *d)
> > > > > >  {
> > > > > >         struct cpumask amask;
> > > > > > -       unsigned int cpu;
> > > > > >         struct plic_priv *priv = irq_get_chip_data(d->irq);
> > > > > >
> > > > > >         cpumask_and(&amask, &priv->lmask, cpu_online_mask);
> > > > > > -       cpu = cpumask_any_and(irq_data_get_affinity_mask(d),
> > > > > > -                                          &amask);
> > > > > > -       if (WARN_ON_ONCE(cpu >= nr_cpu_ids))
> > > > > > -               return;
> > > > > > -       plic_irq_toggle(cpumask_of(cpu), d, 1);
> > > > > > +       cpumask_and(&amask, &amask, irq_data_get_affinity_mask(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_data *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 = irq_get_chip_data(d->irq);
> > > > > >
> > > > > >         cpumask_and(&amask, &priv->lmask, mask_val);
> > > > > >
> > > > > >         if (force)
> > > > > > -               cpu = cpumask_first(&amask);
> > > > > > +               cpumask_copy(&amask, mask_val);
> > > > > >         else
> > > > > > -               cpu = cpumask_any_and(&amask, cpu_online_mask);
> > > > > > -
> > > > > > -       if (cpu >= 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 occurs:
> > > > > 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 what
> > > > > 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.
> > > > >
> > > > > In fact, the current PLIC approach is actually a performance
> > > > > optimization. This implementation also works fine with in-kernel
> > > > > load-balancer and user space load balancer.
> > > > >
> > > >
> > > > Yes, it's exactly, I know what you pointed out. But the idea of this
> > > > patch is just providing a way that users could enable other cores if
> > > > they wanted, it could still enable only one core by this change. The
> > > > purpose here is thinking of flexible use, rather than limitation.
> > > > Maybe it would be a happy medium that we make the default case enable
> > > > only one core? It is a good open discussion.
> > >
> > > Making the default case as enable only one core is just a work-around.
> > >
> > > As-per my understanding, if we set affinity mask of N CPUs for IRQ X
> > > then it does not mean all N CPUs should receive IRQ X rather it means
> > > that exactly one of the N CPUs will receive IRQ X and the IRQ receiving
> > > CPU will be fixed (reflected by effective affinity returned by the driver).
> >
> > is there a case that we only bundle the IRQ to CPU0, but CPU0 is more
> > much busy than other CPUs, and it would be better if another CPU could
> > take the IRQ?
>
> This is a common problem across architectures.
>
> To tackle this, we typically run irqbalance daemon in user-space which will
> change IRQ affinity based on CPU load.
>
> Refer, https://linux.die.net/man/1/irqbalance

OK, thank you for the information and figuring out the issue. Let's
stop this patch unless there are other voices.

>
> >
> > >
> > > If we ignore above semantics and still provide a mechanism to target
> > > IRQ X to N CPUs then most likely someone will try and run into
> > > performance issues.
> > >
> > > Please don't go this path. The performance impact in case of Guest/VM
> > > is even worst because PLIC is trap-n-emulated by hypervisors as MMIO
> > > device.
> >
> > OK, I won't persist in that, just wanna figure out the situation.
> >
> > >
> > > Regards,
> > > Anup
>
> Regards,
> Anup

Re: [PATCH] irqchip/sifive-plic: allow many cores to handle IRQs

[Greentime Hu]

Zong Li <zong.li@sifive.com> 於 2020年4月26日 週日 下午11:35寫道：
>
> 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> wrote:
> > > >
> > > > +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> wrote:
> > > > > > >
> > > > > > > Currently, driver forces the IRQs to be handled by only one core. 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 by boot
> > > > > > > argument.
> > > > > > >
> > > > > > > Use 'irqaffinity' boot argument to determine affinity. If there is no
> > > > > > > irqaffinity in dts or kernel configuration, use irq default affinity,
> > > > > > > so all harts would try to claim IRQ.
> > > > > > >
> > > > > > > For example, add irqaffinity=0 in chosen node to set irq affinity to
> > > > > > > hart 0. It also supports more than one harts to handle irq, such as set
> > > > > > > irqaffinity=0,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 following 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/irqchip/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(const struct cpumask *mask,
> > > > > > >  static void plic_irq_unmask(struct irq_data *d)
> > > > > > >  {
> > > > > > >         struct cpumask amask;
> > > > > > > -       unsigned int cpu;
> > > > > > >         struct plic_priv *priv = irq_get_chip_data(d->irq);
> > > > > > >
> > > > > > >         cpumask_and(&amask, &priv->lmask, cpu_online_mask);
> > > > > > > -       cpu = cpumask_any_and(irq_data_get_affinity_mask(d),
> > > > > > > -                                          &amask);
> > > > > > > -       if (WARN_ON_ONCE(cpu >= nr_cpu_ids))
> > > > > > > -               return;
> > > > > > > -       plic_irq_toggle(cpumask_of(cpu), d, 1);
> > > > > > > +       cpumask_and(&amask, &amask, irq_data_get_affinity_mask(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_data *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 = irq_get_chip_data(d->irq);
> > > > > > >
> > > > > > >         cpumask_and(&amask, &priv->lmask, mask_val);
> > > > > > >
> > > > > > >         if (force)
> > > > > > > -               cpu = cpumask_first(&amask);
> > > > > > > +               cpumask_copy(&amask, mask_val);
> > > > > > >         else
> > > > > > > -               cpu = cpumask_any_and(&amask, cpu_online_mask);
> > > > > > > -
> > > > > > > -       if (cpu >= 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 occurs:
> > > > > > 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 what
> > > > > > 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?

Re: [PATCH] irqchip/sifive-plic: allow many cores to handle IRQs

[Anup Patel]

On Mon, Apr 27, 2020 at 12:19 PM Greentime Hu <greentime.hu@sifive.com> wrote:
>
> Zong Li <zong.li@sifive.com> 於 2020年4月26日 週日 下午11:35寫道：
> >
> > 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> wrote:
> > > > >
> > > > > +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> wrote:
> > > > > > > >
> > > > > > > > Currently, driver forces the IRQs to be handled by only one core. 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 by boot
> > > > > > > > argument.
> > > > > > > >
> > > > > > > > Use 'irqaffinity' boot argument to determine affinity. If there is no
> > > > > > > > irqaffinity in dts or kernel configuration, use irq default affinity,
> > > > > > > > so all harts would try to claim IRQ.
> > > > > > > >
> > > > > > > > For example, add irqaffinity=0 in chosen node to set irq affinity to
> > > > > > > > hart 0. It also supports more than one harts to handle irq, such as set
> > > > > > > > irqaffinity=0,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 following 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/irqchip/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(const struct cpumask *mask,
> > > > > > > >  static void plic_irq_unmask(struct irq_data *d)
> > > > > > > >  {
> > > > > > > >         struct cpumask amask;
> > > > > > > > -       unsigned int cpu;
> > > > > > > >         struct plic_priv *priv = irq_get_chip_data(d->irq);
> > > > > > > >
> > > > > > > >         cpumask_and(&amask, &priv->lmask, cpu_online_mask);
> > > > > > > > -       cpu = cpumask_any_and(irq_data_get_affinity_mask(d),
> > > > > > > > -                                          &amask);
> > > > > > > > -       if (WARN_ON_ONCE(cpu >= nr_cpu_ids))
> > > > > > > > -               return;
> > > > > > > > -       plic_irq_toggle(cpumask_of(cpu), d, 1);
> > > > > > > > +       cpumask_and(&amask, &amask, irq_data_get_affinity_mask(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_data *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 = irq_get_chip_data(d->irq);
> > > > > > > >
> > > > > > > >         cpumask_and(&amask, &priv->lmask, mask_val);
> > > > > > > >
> > > > > > > >         if (force)
> > > > > > > > -               cpu = cpumask_first(&amask);
> > > > > > > > +               cpumask_copy(&amask, mask_val);
> > > > > > > >         else
> > > > > > > > -               cpu = cpumask_any_and(&amask, cpu_online_mask);
> > > > > > > > -
> > > > > > > > -       if (cpu >= 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 occurs:
> > > > > > > 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 what
> > > > > > > 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?

The way you described issue, it clearly means that other device driver
you are using does not have a optimized interrupt handler and the driver
should be fixed right away. I don't see the point in hacking PLIC driver
and slowing it down because some other device driver spending too
much time in interrupt context.

Seeing your comment, I quickly looked at Cadance MACB driver which
is very important on SiFive Unleashed. It turns out that Cadance MACB
driver does not have well designed top-half and bottom-half. Best thing
would be to change Candance MACB driver to use threaded irqs so that
majority of packet processing work on SiFive unleashed is done in kernel
thread (which is preemtible).

Regards,
Anup

Re: [PATCH] irqchip/sifive-plic: allow many cores to handle IRQs

[Marc Zyngier]

Hi Anup,

On 2020-04-26 14:38, Anup Patel wrote:
> +Mark Z

s/k/c/ ;-)

> 
> 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> wrote:
>> > >
>> > > Currently, driver forces the IRQs to be handled by only one core. 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 by boot
>> > > argument.
>> > >
>> > > Use 'irqaffinity' boot argument to determine affinity. If there is no
>> > > irqaffinity in dts or kernel configuration, use irq default affinity,
>> > > so all harts would try to claim IRQ.
>> > >
>> > > For example, add irqaffinity=0 in chosen node to set irq affinity to
>> > > hart 0. It also supports more than one harts to handle irq, such as set
>> > > irqaffinity=0,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 following 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/irqchip/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(const struct cpumask *mask,
>> > >  static void plic_irq_unmask(struct irq_data *d)
>> > >  {
>> > >         struct cpumask amask;
>> > > -       unsigned int cpu;
>> > >         struct plic_priv *priv = irq_get_chip_data(d->irq);
>> > >
>> > >         cpumask_and(&amask, &priv->lmask, cpu_online_mask);
>> > > -       cpu = cpumask_any_and(irq_data_get_affinity_mask(d),
>> > > -                                          &amask);
>> > > -       if (WARN_ON_ONCE(cpu >= nr_cpu_ids))
>> > > -               return;
>> > > -       plic_irq_toggle(cpumask_of(cpu), d, 1);
>> > > +       cpumask_and(&amask, &amask, irq_data_get_affinity_mask(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_data *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 = irq_get_chip_data(d->irq);
>> > >
>> > >         cpumask_and(&amask, &priv->lmask, mask_val);
>> > >
>> > >         if (force)
>> > > -               cpu = cpumask_first(&amask);
>> > > +               cpumask_copy(&amask, mask_val);
>> > >         else
>> > > -               cpu = cpumask_any_and(&amask, cpu_online_mask);
>> > > -
>> > > -       if (cpu >= 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 occurs:
>> > 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 what
>> > 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.
>> >
>> > In fact, the current PLIC approach is actually a performance
>> > optimization. This implementation also works fine with in-kernel
>> > load-balancer and user space load balancer.
>> >
>> 
>> Yes, it's exactly, I know what you pointed out. But the idea of this
>> patch is just providing a way that users could enable other cores if
>> they wanted, it could still enable only one core by this change. The
>> purpose here is thinking of flexible use, rather than limitation.
>> Maybe it would be a happy medium that we make the default case enable
>> only one core? It is a good open discussion.
> 
> Making the default case as enable only one core is just a work-around.
> 
> As-per my understanding, if we set affinity mask of N CPUs for IRQ X
> then it does not mean all N CPUs should receive IRQ X rather it means
> that exactly one of the N CPUs will receive IRQ X and the IRQ receiving
> CPU will be fixed (reflected by effective affinity returned by the 
> driver).
> 
> If we ignore above semantics and still provide a mechanism to target
> IRQ X to N CPUs then most likely someone will try and run into
> performance issues.
> 
> Please don't go this path. The performance impact in case of Guest/VM
> is even worst because PLIC is trap-n-emulated by hypervisors as MMIO
> device.

Just to add my view on this, as we get the same request on the ARM side
once every other year:

- the "broadcast" aspect of the interrupt signalling is usually a waste
   of CPU cycles on all CPUs but the one that has actually takes it.

- the fact that you have to use MMIO to interact with the interrupt
   controller potentially makes this even worse if you don't have some
   per-CPU datapath that will handle this really fast.

- not hitting the same CPU at all times for a given interrupt means that
   the caches are usually cold for this interrupt, meaning the gain in
   latency is pretty dubious.

There is a very small (but also very vocal) fringe of users that insist
that they cannot live without this broadcast aspect, because it allows
them to shave a couple of cycles off a trivial interrupt handler on 
systems
that are otherwise mostly idle.

My answer to this is that Linux is a general purpose OS, and not a
glorified interrupt handler. We aim to have balanced response times, and
give *userspace* control over where the interrupt happens in the long 
run.

HTH,

         M.
-- 
Jazz is not dead. It just smells funny...