2023-09-25 02:38:26

by Rex Nie

[permalink] [raw]
Subject: [PATCH] irqchip/gic-v3: move reading of GICR_TYPER later

This changeset will reduce one reading of GICR_TYPER register
when any of next 2 condictions meets:
1. found redistributor for current cpu
2. single_redist is true for gic_data.redist_regions[i]

Signed-off-by: Rex Nie <[email protected]>
---
drivers/irqchip/irq-gic-v3.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index eedfa8e9f077..d08a4773f631 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -982,7 +982,6 @@ static int gic_iterate_rdists(int (*fn)(struct redist_region *, void __iomem *))
}

do {
- typer = gic_read_typer(ptr + GICR_TYPER);
ret = fn(gic_data.redist_regions + i, ptr);
if (!ret)
return 0;
@@ -990,6 +989,7 @@ static int gic_iterate_rdists(int (*fn)(struct redist_region *, void __iomem *))
if (gic_data.redist_regions[i].single_redist)
break;

+ typer = gic_read_typer(ptr + GICR_TYPER);
if (gic_data.redist_stride) {
ptr += gic_data.redist_stride;
} else {
--
2.34.1


2023-09-26 16:43:01

by Marc Zyngier

[permalink] [raw]
Subject: Re: [PATCH] irqchip/gic-v3: move reading of GICR_TYPER later

On Mon, 25 Sep 2023 03:34:08 +0100,
Rex Nie <[email protected]> wrote:
>
> This changeset will reduce one reading of GICR_TYPER register
> when any of next 2 condictions meets:
> 1. found redistributor for current cpu
> 2. single_redist is true for gic_data.redist_regions[i]
>
> Signed-off-by: Rex Nie <[email protected]>
> ---
> drivers/irqchip/irq-gic-v3.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> index eedfa8e9f077..d08a4773f631 100644
> --- a/drivers/irqchip/irq-gic-v3.c
> +++ b/drivers/irqchip/irq-gic-v3.c
> @@ -982,7 +982,6 @@ static int gic_iterate_rdists(int (*fn)(struct redist_region *, void __iomem *))
> }
>
> do {
> - typer = gic_read_typer(ptr + GICR_TYPER);
> ret = fn(gic_data.redist_regions + i, ptr);
> if (!ret)
> return 0;
> @@ -990,6 +989,7 @@ static int gic_iterate_rdists(int (*fn)(struct redist_region *, void __iomem *))
> if (gic_data.redist_regions[i].single_redist)
> break;
>
> + typer = gic_read_typer(ptr + GICR_TYPER);
> if (gic_data.redist_stride) {
> ptr += gic_data.redist_stride;
> } else {

I personally prefer establishing the exit condition for the loop as
early as possible. Doing it later makes this code slightly more
fragile, and I somehow doubt that you will notice the extra reads.

Can you at least quantify the improvement?

M.

--
Without deviation from the norm, progress is not possible.

2023-10-08 12:57:34

by Rex Nie

[permalink] [raw]
Subject: 答复: [PATCH] irqchip/gic-v3: move reading of GICR_TYPER later

HI Marc,
Thanks for your code review, Agree with you about the changeset will make this code more fragile.
BRs
Rex Nie

-----?ʼ?ԭ??-----
??????: Marc Zyngier <[email protected]>
????ʱ??: 2023??9??27?? 0:40
?ռ???: Rex Nie <[email protected]>
????: [email protected]; [email protected]
????: Re: [PATCH] irqchip/gic-v3: move reading of GICR_TYPER later

On Mon, 25 Sep 2023 03:34:08 +0100,
Rex Nie <[email protected]> wrote:
>
> This changeset will reduce one reading of GICR_TYPER register when any
> of next 2 condictions meets:
> 1. found redistributor for current cpu 2. single_redist is true for
> gic_data.redist_regions[i]
>
> Signed-off-by: Rex Nie <[email protected]>
> ---
> drivers/irqchip/irq-gic-v3.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/irqchip/irq-gic-v3.c
> b/drivers/irqchip/irq-gic-v3.c index eedfa8e9f077..d08a4773f631 100644
> --- a/drivers/irqchip/irq-gic-v3.c
> +++ b/drivers/irqchip/irq-gic-v3.c
> @@ -982,7 +982,6 @@ static int gic_iterate_rdists(int (*fn)(struct redist_region *, void __iomem *))
> }
>
> do {
> - typer = gic_read_typer(ptr + GICR_TYPER);
> ret = fn(gic_data.redist_regions + i, ptr);
> if (!ret)
> return 0;
> @@ -990,6 +989,7 @@ static int gic_iterate_rdists(int (*fn)(struct redist_region *, void __iomem *))
> if (gic_data.redist_regions[i].single_redist)
> break;
>
> + typer = gic_read_typer(ptr + GICR_TYPER);
> if (gic_data.redist_stride) {
> ptr += gic_data.redist_stride;
> } else {

I personally prefer establishing the exit condition for the loop as early as possible. Doing it later makes this code slightly more fragile, and I somehow doubt that you will notice the extra reads.

Can you at least quantify the improvement?

M.

--
Without deviation from the norm, progress is not possible.