Re: [Xen-devel] [RFC 10/16] gic:vgic:gic-vgic: introduce non-atomic bitops

[Andre Przywara <andre.przywara@xxxxxxx>]

On Wed, 28 Nov 2018 23:32:05 +0200
Andrii Anisov <andrii.anisov@xxxxxxxxx> wrote:

Hi,

> From: Andrii Anisov <andrii_anisov@xxxxxxxx>
> 
> All bit operations for gic, vgic and gic-vgic are performed under
> spinlocks, so there is no need for atomic bit ops here, they only
> introduce excessive call to functions used more expensive exclusive
> ARM instructions.
> 
> Signed-off-by: Andrii Anisov <andrii_anisov@xxxxxxxx>
> ---
>  xen/arch/arm/gic-vgic.c | 16 ++++++++++++++++
>  xen/arch/arm/gic.c      | 16 ++++++++++++++++
>  xen/arch/arm/vgic.c     | 16 ++++++++++++++++
>  3 files changed, 48 insertions(+)
> 
> diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c
> index f0e6c6f..5b73bbd 100644
> --- a/xen/arch/arm/gic-vgic.c
> +++ b/xen/arch/arm/gic-vgic.c
> @@ -25,6 +25,22 @@
>  #include <asm/gic.h>
>  #include <asm/vgic.h>
>  
> +#undef set_bit
> +#define set_bit(nr, addr) (*(addr) |= (1<<nr))
> +
> +#undef clear_bit
> +#define clear_bit(nr, addr) (*(addr) &= ~(1<<nr))
> +
> +#undef test_bit
> +#define test_bit(nr,addr) (*(addr) & (1<<nr))
> +
> +#undef test_and_clear_bit
> +#define test_and_clear_bit(nr,addr) ({                    \
> +    bool _x; \
> +    _x = (*(addr) & (1<<nr));                        \
> +    (*(addr) &= ~(1<<nr));                                \
> +    (_x);})
> +
>  #define lr_all_full() (this_cpu(lr_mask) == ((1 << gic_get_nr_lrs())
> - 1)) 
>  #undef GIC_DEBUG
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 52e4270..d558059 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -40,6 +40,22 @@
>  
>  DEFINE_PER_CPU(uint64_t, lr_mask);
>  
> +#undef set_bit
> +#define set_bit(nr, addr) (*(addr) |= (1<<nr))
> +
> +#undef clear_bit
> +#define clear_bit(nr, addr) (*(addr) &= ~(1<<nr))
> +
> +#undef test_bit
> +#define test_bit(nr,addr) (*(addr) & (1<<nr))
> +
> +#undef test_and_clear_bit
> +#define test_and_clear_bit(nr,addr) ({                    \
> +    bool _x; \
> +    _x = (*(addr) & (1<<nr));                        \
> +    (*(addr) &= ~(1<<nr));                                \
> +    (_x);})
> +
>  #undef GIC_DEBUG
>  
>  const struct gic_hw_operations *gic_hw_ops;
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index c142476..7691310 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -33,6 +33,22 @@
>  #include <asm/gic.h>
>  #include <asm/vgic.h>
>  
> +#undef set_bit

Nah, please don't do this. Can you show that atomic bit ops are a
problem? They shouldn't be expensive unless contended, also pretty
lightweight on small systems (single cluster).

But if you really think this is useful, why not go with the Linux way
of using __set_bit to provide a non-atomic version?
This would have the big advantage that you can replace them on a
case-by-case base, which is much less risky than unconditionally
replacing every (even future!) usage in the whole file.

Cheers,
Andre.

> +#define set_bit(nr, addr) (*(addr) |= (1<<nr))
> +
> +#undef clear_bit
> +#define clear_bit(nr, addr) (*(addr) &= ~(1<<nr))
> +
> +#undef test_bit
> +#define test_bit(nr,addr) (*(addr) & (1<<nr))
> +
> +#undef test_and_clear_bit
> +#define test_and_clear_bit(nr,addr) ({                    \
> +    bool _x = (*(addr) & (1<<nr));                        \
> +    (*(addr) &= ~(1<<nr));                                \
> +    return (_x);                                                 \
> +})
> +
>  static inline struct vgic_irq_rank *vgic_get_rank(struct vcpu *v,
> int rank) {
>      if ( rank == 0 )


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