Re: [Xen-devel] [PATCH 06/15] xen: trace IRQ enabling/disabling

[Julien Grall <julien.grall@xxxxxxx>]

On 07/06/17 16:22, Dario Faggioli wrote:
> On Wed, 2017-06-07 at 12:16 +0100, Julien Grall wrote:
> > Hi Dario,
> Hi,
>
> > On 01/06/17 18:34, Dario Faggioli wrote:
> > > diff --git a/xen/common/spinlock.c b/xen/common/spinlock.c
> > > index 2a06406..33b903e 100644
> > > --- a/xen/common/spinlock.c
> > > +++ b/xen/common/spinlock.c
> > > @@ -150,7 +150,9 @@ void _spin_lock(spinlock_t *lock)
> > >  void _spin_lock_irq(spinlock_t *lock)
> > >  {
> > >      ASSERT(local_irq_is_enabled());
> > > -    local_irq_disable();
> > > +    _local_irq_disable();
> > > +    if ( unlikely(tb_init_done) )
> > __trace_var already contain a "if ( !tb_init_done )". It sounds
> > pointless to do it twice, and also I think it is not obvious to
> > understand the meaning of the check (i.e what is tb_init_done).
> >
> > Would it be possible to hide this check and avoid check tb_init_done
> > twice?
> I totally agree. And in fact, in another patch, I remove the
> tb_init_done check in __trace_var(). Reason behind this is that all the
> callers of __trace_var() (the main one being trace_var()), already
> checks for tb_init_done themselves.
>
> In fact, the explicit check in the caller, is the (only) basis on which
> one decides to call either trace_var() or __trace_var().
>
> This here is one of the call sites where I think the check is better
> done in the caller.
I admit, it is a bit confusing to require the caller to check 
tb_init_done. It adds more code and IHMO counterintuitive.
> > > diff --git a/xen/include/asm-arm/arm32/system.h b/xen/include/asm-
> > > arm/arm32/system.h
> > > index c617b40..20871ad 100644
> > > --- a/xen/include/asm-arm/arm32/system.h
> > > +++ b/xen/include/asm-arm/arm32/system.h
> > > @@ -4,6 +4,8 @@
> > >
> > >  #include <asm/arm32/cmpxchg.h>
> > >
> > > +#include <xen/trace.h>
> > > +
> > >  #define local_irq_disable() asm volatile ( "cpsid i @
> > > local_irq_disable\n" : : : "cc" )
> > >  #define local_irq_enable()  asm volatile ( "cpsie i @
> > > local_irq_enable\n" : : : "cc" )
> > >
> > > @@ -41,6 +43,16 @@ static inline int local_irq_is_enabled(void)
> > >  #define local_abort_enable() __asm__("cpsie a  @ __sta\n" : : :
> > > "memory", "cc")
> > >  #define local_abort_disable() __asm__("cpsid a @ __sta\n" : : :
> > > "memory", "cc")
> > >
> > > +/* We do not support tracing (at all) yet */
> > I know that some bits are missing for ARM, but the patch #5
> > contradicts
> > this comment as you have CONFIG_TRACE=y by default.
> No sure what you mean. Tracing is de facto on by default right now,
> despite it not being implemented for ARM. So what I'm doing is
> basically keeping things as they are.
>
> If you think it should be off by default, well, let's talk about it...
> but I'm not sure this is really what you are saying/asking.
I mean that CONFIG_TRACE=y by default gives the impression that tracing 
is supported on ARM. This is not the case, hence your comment. So I 
think it should be off my default until we get the support. BTW, it is 
only a couple of simple patches but they never made upstreamed :/
> > > +#define trace_irq_disable_ret()   do { } while ( 0 )
> > > +#define trace_irq_enable_ret()    do { } while ( 0 )
> > > +#define trace_irq_save_ret(_x)    do { } while ( 0 )
> > > +#define trace_irq_restore_ret(_x) do { } while ( 0 )
> > > +#define _local_irq_disable()      local_irq_disable()
> > > +#define _local_irq_enable()       local_irq_enable()
> > > +#define _local_irq_save(_x)       local_irq_save(_x)
> > > +#define _local_irq_restore(_x)    local_irq_restore(_x)
> > > +
> > This does not need to be duplicated in both asm-
> > arm/arm{32,64}/system.h.
> > You could directly implement them in asm-arm/system.h.
> Ok, thanks.
>
> > Also, I would prefer if you stay consistent with x86. I.e non-
> > underscore
> > versions are calling the underscore versions and not the invert.
> Well, I know it is counterintuitive, but it's the easiest way of
> getting over this, i.e., without the need of a lot of stubs, and
> touching as few existing code as possible.
Can you detail here? The only things I was asking is to do:

#define _local_irq_disable() ......

#define local_irq_disable() _local_irq_disable.

The callers would still use local_irq_disable() and the code would be 
consistent with x86.
Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel