Re: [Xen-devel] [PATCH 03/15] xen/tools: tracing: several improvements on IRQs tracing

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 07.06.17 at 17:45, <dario.faggioli@xxxxxxxxxx> wrote:
> On Wed, 2017-06-07 at 09:05 -0600, Jan Beulich wrote:
>> > > > On 01.06.17 at 19:33, <dario.faggioli@xxxxxxxxxx> wrote:
>> > @@ -884,9 +919,13 @@ void do_IRQ(struct cpu_user_regs *regs)
>> >              desc->rl_quantum_start = now;
>> >          }
>> >  
>> > -        tsc_in = tb_init_done ? get_cycles() : 0;
>> > +        if ( unlikely(tb_init_done) )
>> > +        {
>> > +            __trace_var(TRC_HW_IRQ_GUEST, 0, sizeof(irq), &irq);
>> > +            tsc_in = get_cycles();
>> > +        }
>> >          __do_IRQ_guest(irq);
>> > -        TRACE_3D(TRC_HW_IRQ_HANDLED, irq, tsc_in, get_cycles());
>> > +        trace_irq_handled(irq, get_cycles() - tsc_in);
>> >          goto out_no_end;
>> >      }
>> 
>> Before this change, the get_cycles() invocation was after
>> the tb_init_done check. Now you move it ahead of it (as
>> the function arguments are evaluated before executing the
>> function body) - are you sure all compiler versions will suitably
>> re-order this?
>> 
>> Additionally I'm afraid this will trigger compiler warnings on at
>> least some compiler versions, as tsc_in is now possibly
>> uninitialized (and there's no clear way to disprove this for the
>> compiler, again because function arguments are being
>> evaluated before the function body is reached).
>> 
> I understand and (now that I see it) agree with your remark on when
> get_cycles() is called. I'll reorganize things so that the patched
> behavior matches the existing one.
> 
> OTOH, I'm not sure I see the "potentially uninitialized" issue for
> tsc_in, but since I'm reworking the code, I'll keep that in mind too.

You initialize tsc_in only when tb_init_done is set, but you use
it without that conditional. And even if you used it under that
same conditional, older compiler versions aren't able to track
that fact (same conditional) and raise a warning anyway.

>> As to get_cycles() itself - is the relatively imprecise time
>> stamp it produces really good enough for tracing? I notice
>> that there are only very few other users of that function.
>> 
> Yes, that's also something I was wondering. It's what is being used
> currently, so I thought it was like that for a reason, and that it
> wasn't this patch job to change that.
> 
> But if it's judged to be fine to turn this into NOW() (done either
> here, or in a dedicated patch)., I'm all for it.

Right, I didn't necessarily mean for this to be done here, I merely
wanted to raise the fact (remembering that for the other time
handling code using "synced" TSC reads was quite a bit of an
accuracy improvement).

>> > @@ -922,6 +962,7 @@ void do_IRQ(struct cpu_user_regs *regs)
>> >      spin_unlock(&desc->lock);
>> >   out_no_unlock:
>> >      irq_exit();
>> > +    TRACE_3D(TRC_HW_IRQ_EXIT, irq, desc == NULL ? -1 : desc-
>> > >status, in_irq());
>> 
>> The ordering of irq_{enter,exit}() and TRACE_{1,3}D() may be
>> preferable from a trace quality pov, but as far as the system is
>> concerned you're adding code which runs in interrupt context
>> without being aware of that. This may be a latent issue.
>> 
> Sorry, I don't understand your concern(s). What is it that I am not
> aware of, and what is it that could be a latent issue?
> 
> About the position of those tracepoints: the nice thing about IRQ_EXIT
> following irq_exit() is that I can record the result of in_irq(), which
> will tell whether or not we're dealing with a nested interrupt. I don't
>  do the same for IRQ_ENTER, but, not that I think about it, I guess I
> could.
> 
> However, if what you're saying is that they need to stay within the
> irq_enter()-irq_exit() section, I can certainly make that happen. The
> trace needs to be interpreted knowing where the tracepoints are anyway
> (and that's true for each and every event already), so no big deal,
> really.

Yes, it's really just this latter fact - it would feel more safe from
an overall system pov if you had

    irq_enter();
    TRACE_1D();
    ...
    TRACE_3D();
    irq_exit();

Jan

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