Re: [Xen-devel] [RFC 16/16] gic: vgic: align frequently accessed data by cache line size

[Julien Grall <julien.grall@xxxxxxx>]

On 28/11/2018 21:32, Andrii Anisov wrote:
> From: Andrii Anisov <andrii_anisov@xxxxxxxx>
>
> Cache line size assumed 64 byte as for H3. Align the `struct
> pending_irq` and allocate lrs shadow aligned to cache line size.
The arch_vcpu is already aligned to a cache size. So how does it improve the 
performance by making lr also aligned to a cache line?
I can believe that pending_irq would be nice to have it aligned to a cache line 
(or half of it). But you need to explain the trade-off as you are going to use 
more memory.
> Signed-off-by: Andrii Anisov <andrii_anisov@xxxxxxxx>
> ---
>   xen/arch/arm/domain.c        | 4 ++++
>   xen/arch/arm/vgic.c          | 9 +++++++++
>   xen/include/asm-arm/config.h | 2 +-
>   xen/include/asm-arm/gic.h    | 2 +-
>   xen/include/asm-arm/vgic.h   | 2 +-
>   5 files changed, 16 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 8e886b7..8bcb667 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -558,6 +558,10 @@ int arch_vcpu_create(struct vcpu *v)
>       v->arch.saved_context.sp = (register_t)v->arch.cpu_info;
>       v->arch.saved_context.pc = (register_t)continue_new_vcpu;
>   
> +    v->arch.gic.v2.lr = xzalloc_bytes(sizeof(uint32_t) * gic_number_lines());
> +    if ( v->arch.gic.v2.lr == NULL )
> +        return -ENOMEM;
> +
>       /* Idle VCPUs don't need the rest of this setup */
>       if ( is_idle_vcpu(v) )
>           return rc;
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index 7691310..bedfb99 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -166,6 +166,15 @@ int domain_vgic_init(struct domain *d, unsigned int 
> nr_spis)
>   
>       d->arch.vgic.pending_irqs =
>           xzalloc_array(struct pending_irq, d->arch.vgic.nr_spis);
> +
> +    if ( sizeof(struct pending_irq) != dcache_line_bytes )
> +    {
> +        printk ("sizeof(struct pending_irq) = %lu  is not equal to cacheline"
> +                "size %lu. Is it expected?\n", sizeof(struct pending_irq),
> +                dcache_line_bytes);
> +        WARN();
> +    }
What could happen if pending_irq is not aligned to your cacheline? If your 
cacheline is smaller than pending_irq, then you are going to use multiple cache 
line. So nothing to worry.
If your cacheline is bigger, then it is a potential concern as you would share 
the cacheline with something else. This may be an issue, but I would like to see 
numbers first.
> +
>       if ( d->arch.vgic.pending_irqs == NULL )
>           return -ENOMEM;
>   
> diff --git a/xen/include/asm-arm/config.h b/xen/include/asm-arm/config.h
> index bc89e84..4f3669f 100644
> --- a/xen/include/asm-arm/config.h
> +++ b/xen/include/asm-arm/config.h
> @@ -28,7 +28,7 @@
>   
>   #define CONFIG_ARM 1
>   
> -#define CONFIG_ARM_L1_CACHE_SHIFT 7 /* XXX */
> +#define CONFIG_ARM_L1_CACHE_SHIFT 6 /* XXX */
That's not acceptable, this value is based on the biggest cache line used by Arm 
processors. If you want to use 64 bytes cache line, then you either make sure 
the structure fits in 64-bytes by adding padding.
>   
>   #define CONFIG_SMP 1
>   
> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index add2566..fe44d3a 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -186,7 +186,7 @@ struct gic_v2 {
>       uint32_t hcr;
>       uint32_t vmcr;
>       uint32_t apr;
> -    uint32_t lr[64];
> +    uint32_t *lr;
>       uint64_t lr_update_mask;
>   };
>   
> diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
> index a27a1a9..d4ec96f 100644
> --- a/xen/include/asm-arm/vgic.h
> +++ b/xen/include/asm-arm/vgic.h
> @@ -99,7 +99,7 @@ struct pending_irq
>        * TODO: when implementing irq migration, taking only the current
>        * vgic lock is not going to be enough. */
>       struct list_head lr_queue;
> -};
> +}__cacheline_aligned;
>   
>   #define NR_INTERRUPT_PER_RANK   32
>   #define INTERRUPT_RANK_MASK (NR_INTERRUPT_PER_RANK - 1)
Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel