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

[George Dunlap <george.dunlap@xxxxxxxxxx>]

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.

Because a number of the traces include a certain amount if "marshalling"
work ahead of time; there's no point in making space for a struct on the
stack, carefully reading and sorting all kinds if things to put into it,
only to have the data ignored because tracing isn't enabled (which is by
far the most common case).

 -George


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