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

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

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