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

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 02/06/17 00:42, Dario Faggioli wrote:
> On Thu, 2017-06-01 at 20:08 +0100, Andrew Cooper wrote:
>> 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_irq_disable_ret();
>>>      _spin_lock(lock);
>>>  }
>>
>> Is it sensible to do this way around?
>>
> Well, I guess either variant has up and down sides, and it looks to me
> that there is no way to measure this, without interfering with the
> behavior of the thing being measured ("Once upon a time, there was a
> cat, who lived in a box. His name was Schrödinger..." :-D :-D :-D)
> 
>> By writing the trace record while interrupts are disabled, you do
>> prevent nesting in the general case (but not in NMIs/MCEs or the
>> irqsave() variants), 
>>
> Forgive the ignorance, what's special about NMIs/MCAs that is relevant
> for this? About _irqsave(), I'm also not sure what you mean, as
> _irqsave() is already done differently than this.
> 
>> but you increase the critical region while
>> interrupts are disabled.
>>
> I know.
> 
>> Alternatively, writing the trace record outside of the critical
>> region
>> would reduce the size of the region.  Interpretation logic already
>> needs
>> to cope with nesting, so is one extra level of nesting in this corner
>> case a problem?
>>
> TBH, I was indeed trying to offer to the component that will be looking
> at and interpreting the data, the as clear as possible view on when
> IRQs are _actually_ disabled and enabled. As in, if nesting occurs,
> only the event corresponding to:
>  - the first local_irq_disable()
>  - the last local_irq_enable()
> 
> I.e., to not require that it (the interpretation logic) does understand
> nesting.
> 
> But more than this, what I was concerned about was the accuracy of the
> reporting itself. In fact, if I do as you suggest, I can be interrupted
> (as interrupts are still on) after having called trace_irq_disable().
> That means the report will show higher IRQ disabled time period, for
> this instance, than what the reality is.
> 
> And the same is true on the enabling side, when I call
> trace_irq_enable() _before_ re-enabling interrupts, which has the same
> bad effect on the length of IRQ disabled section.
> 
> Maybe, considering that anything that will interrupt me in these cases,
> are other interrupts, that will most likely disable interrupts
> themselves, I should not worry too much about this... What do you
> think?

I think it would make the analysis easier to do it the way you've done
it.  It certainly does increase the length of the actual critical
section, but we're already assuming that all of this code is going to be
disabled unless people specifically want to do IRQ analysis, so I don't
think that should be an issue.

 -George


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