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

[Julien Grall <julien.grall@xxxxxxx>]

On 09/06/17 11:51, Julien Grall wrote:
> 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.
BTW, I am not responsible of this code, so if the REST maintainers are 
happy with that then fine.
> > > > 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