From: Marc Zyngier <maz@kernel.org> To: Jingyi Wang <wangjingyi11@huawei.com> Cc: <linux-kernel@vger.kernel.org>, <linux-arm-kernel@lists.infradead.org>, <tglx@linutronix.de>, <wanghaibin.wang@huawei.com>, <yuzenghui@huawei.com>, <zhukeqian1@huawei.com> Subject: Re: [RFC PATCH 1/3] irqchip/gic-v3: Make use of ICC_SGI1R IRM bit Date: Mon, 29 Mar 2021 10:55:23 +0100 [thread overview] Message-ID: <87wntqqo6s.wl-maz@kernel.org> (raw) In-Reply-To: <20210329085210.11524-2-wangjingyi11@huawei.com> On Mon, 29 Mar 2021 09:52:08 +0100, Jingyi Wang <wangjingyi11@huawei.com> wrote: > > IRM, bit[40] in ICC_SGI1R, determines how the generated SGIs > are distributed to PEs. If the bit is set, interrupts are routed > to all PEs in the system excluding "self". We use cpumask to > determine if this bit should be set and make use of that. > > This will reduce vm trap when broadcast IPIs are sent. I remember writing similar code about 4 years ago, only to realise what: - the cost of computing the resulting mask is pretty high for large machines - Linux almost never sends broadcast IPIs, so the complexity was all in vain What changed? Please provide supporting data showing how many IPIs we actually save, and for which workload. > > Signed-off-by: Jingyi Wang <wangjingyi11@huawei.com> > --- > drivers/irqchip/irq-gic-v3.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c > index eb0ee356a629..8ecc1b274ea8 100644 > --- a/drivers/irqchip/irq-gic-v3.c > +++ b/drivers/irqchip/irq-gic-v3.c > @@ -1127,6 +1127,7 @@ static void gic_send_sgi(u64 cluster_id, u16 tlist, unsigned int irq) > static void gic_ipi_send_mask(struct irq_data *d, const struct cpumask *mask) > { > int cpu; > + cpumask_t tmp; > > if (WARN_ON(d->hwirq >= 16)) > return; > @@ -1137,6 +1138,17 @@ static void gic_ipi_send_mask(struct irq_data *d, const struct cpumask *mask) > */ > wmb(); > > + if (!cpumask_and(&tmp, mask, cpumask_of(smp_processor_id()))) { Are you sure this does the right thing? This is checking that the current CPU is not part of the mask. But it not checking that the mask is actually "all but self". This means you are potentially sending IPIs to CPUs that are not part of the mask, making performance potentially worse. Thanks, M. > + /* Set Interrupt Routing Mode bit */ > + u64 val; > + val = (d->hwirq) << ICC_SGI1R_SGI_ID_SHIFT; > + val |= BIT_ULL(ICC_SGI1R_IRQ_ROUTING_MODE_BIT); > + gic_write_sgi1r(val); > + > + isb(); > + return; > + } > + > for_each_cpu(cpu, mask) { > u64 cluster_id = MPIDR_TO_SGI_CLUSTER_ID(cpu_logical_map(cpu)); > u16 tlist; > -- > 2.19.1 > > -- Without deviation from the norm, progress is not possible.
WARNING: multiple messages have this Message-ID (diff)
From: Marc Zyngier <maz@kernel.org> To: Jingyi Wang <wangjingyi11@huawei.com> Cc: <linux-kernel@vger.kernel.org>, <linux-arm-kernel@lists.infradead.org>, <tglx@linutronix.de>, <wanghaibin.wang@huawei.com>, <yuzenghui@huawei.com>, <zhukeqian1@huawei.com> Subject: Re: [RFC PATCH 1/3] irqchip/gic-v3: Make use of ICC_SGI1R IRM bit Date: Mon, 29 Mar 2021 10:55:23 +0100 [thread overview] Message-ID: <87wntqqo6s.wl-maz@kernel.org> (raw) In-Reply-To: <20210329085210.11524-2-wangjingyi11@huawei.com> On Mon, 29 Mar 2021 09:52:08 +0100, Jingyi Wang <wangjingyi11@huawei.com> wrote: > > IRM, bit[40] in ICC_SGI1R, determines how the generated SGIs > are distributed to PEs. If the bit is set, interrupts are routed > to all PEs in the system excluding "self". We use cpumask to > determine if this bit should be set and make use of that. > > This will reduce vm trap when broadcast IPIs are sent. I remember writing similar code about 4 years ago, only to realise what: - the cost of computing the resulting mask is pretty high for large machines - Linux almost never sends broadcast IPIs, so the complexity was all in vain What changed? Please provide supporting data showing how many IPIs we actually save, and for which workload. > > Signed-off-by: Jingyi Wang <wangjingyi11@huawei.com> > --- > drivers/irqchip/irq-gic-v3.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c > index eb0ee356a629..8ecc1b274ea8 100644 > --- a/drivers/irqchip/irq-gic-v3.c > +++ b/drivers/irqchip/irq-gic-v3.c > @@ -1127,6 +1127,7 @@ static void gic_send_sgi(u64 cluster_id, u16 tlist, unsigned int irq) > static void gic_ipi_send_mask(struct irq_data *d, const struct cpumask *mask) > { > int cpu; > + cpumask_t tmp; > > if (WARN_ON(d->hwirq >= 16)) > return; > @@ -1137,6 +1138,17 @@ static void gic_ipi_send_mask(struct irq_data *d, const struct cpumask *mask) > */ > wmb(); > > + if (!cpumask_and(&tmp, mask, cpumask_of(smp_processor_id()))) { Are you sure this does the right thing? This is checking that the current CPU is not part of the mask. But it not checking that the mask is actually "all but self". This means you are potentially sending IPIs to CPUs that are not part of the mask, making performance potentially worse. Thanks, M. > + /* Set Interrupt Routing Mode bit */ > + u64 val; > + val = (d->hwirq) << ICC_SGI1R_SGI_ID_SHIFT; > + val |= BIT_ULL(ICC_SGI1R_IRQ_ROUTING_MODE_BIT); > + gic_write_sgi1r(val); > + > + isb(); > + return; > + } > + > for_each_cpu(cpu, mask) { > u64 cluster_id = MPIDR_TO_SGI_CLUSTER_ID(cpu_logical_map(cpu)); > u16 tlist; > -- > 2.19.1 > > -- Without deviation from the norm, progress is not possible. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2021-03-29 9:56 UTC|newest] Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-03-29 8:52 [RFC PATCH 0/3] arm: Some IPI injection optimization Jingyi Wang 2021-03-29 8:52 ` Jingyi Wang 2021-03-29 8:52 ` [RFC PATCH 1/3] irqchip/gic-v3: Make use of ICC_SGI1R IRM bit Jingyi Wang 2021-03-29 8:52 ` Jingyi Wang 2021-03-29 9:55 ` Marc Zyngier [this message] 2021-03-29 9:55 ` Marc Zyngier 2021-03-29 10:38 ` Jingyi Wang 2021-03-29 10:38 ` Jingyi Wang 2021-03-29 11:28 ` Marc Zyngier 2021-03-29 11:28 ` Marc Zyngier 2021-03-29 8:52 ` [RFC PATCH 2/3] irqchip/gic-v3: Implement gic_ipi_send_single() Jingyi Wang 2021-03-29 8:52 ` Jingyi Wang 2021-03-29 8:52 ` [RFC PATCH 3/3] arm/arm64: Use gic_ipi_send_single() to inject single IPI Jingyi Wang 2021-03-29 8:52 ` Jingyi Wang 2021-03-29 10:07 ` Marc Zyngier 2021-03-29 10:07 ` Marc Zyngier 2021-03-29 11:03 ` Jingyi Wang 2021-03-29 11:03 ` Jingyi Wang 2021-03-29 15:09 ` kernel test robot
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=87wntqqo6s.wl-maz@kernel.org \ --to=maz@kernel.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=tglx@linutronix.de \ --cc=wanghaibin.wang@huawei.com \ --cc=wangjingyi11@huawei.com \ --cc=yuzenghui@huawei.com \ --cc=zhukeqian1@huawei.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.