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

[Julien Grall <julien.grall@xxxxxxx>]

Hi,

On 29/11/2018 12:14, Andre Przywara wrote:
> 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?
We already have __set_bit/__clear_bit on Xen. However, it looks like the arm 
version is a wrapper to the atomic version. Rationale from the comment is those 
functions should be interrupt safe.
I don't know where that requirement come from. You already have race if that 
variable is modified simultaneously. So I would just re-use the Linux 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 )
--
Julien Grall

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