irqchip/gic-v3: move reading of GICR_TYPER later

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

Posted by Rex Nie 2 years, 4 months ago

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 <rex.nie@jaguarmicro.com>
---
 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

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

Posted by Marc Zyngier 2 years, 4 months ago

On Mon, 25 Sep 2023 03:34:08 +0100,
Rex Nie <rex.nie@jaguarmicro.com> 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 <rex.nie@jaguarmicro.com>
> ---
>  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.

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

Posted by Rex Nie 2 years, 4 months ago

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

-----邮件原件-----
发件人: Marc Zyngier <maz@kernel.org> 
发送时间: 2023年9月27日 0:40
收件人: Rex Nie <rex.nie@jaguarmicro.com>
抄送: tglx@linutronix.de; linux-kernel@vger.kernel.org
主题: Re: [PATCH] irqchip/gic-v3: move reading of GICR_TYPER later

On Mon, 25 Sep 2023 03:34:08 +0100,
Rex Nie <rex.nie@jaguarmicro.com> 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 <rex.nie@jaguarmicro.com>
> ---
>  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.