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

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

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.

> > 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.


> > +#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.

But I certainly can give it a try doing it as you say, and see how it
ends up looking like.

Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

Attachment: signature.asc
Description: This is a digitally signed message part

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