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

[Dario Faggioli <dario.faggioli@citrix.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3327 bytes --]

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?

> Does the logic cope with the fact that interrupt gates automatically
> disable interrupts?
> 
Ah, right. No, it does not. I probably should mention this in the
changelog. Any ideas of how to deal with that? If yes, I'm more than
happy to fix this...

Thanks and 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 #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

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