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

[Julien Grall <julien.grall@arm.com>]

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@lists.xen.org
https://lists.xen.org/xen-devel