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

["Jan Beulich" <JBeulich@suse.com>]

>>> On 08.06.17 at 16:53, <george.dunlap@citrix.com> wrote:
> On 07/06/17 16:58, Jan Beulich wrote:
>>>>> On 07.06.17 at 17:45, <dario.faggioli@citrix.com> wrote:
>>> On Wed, 2017-06-07 at 09:05 -0600, Jan Beulich wrote:
>>>>>>> On 01.06.17 at 19:33, <dario.faggioli@citrix.com> 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.
> 
> This patch changes thing to initialize tsc_in on declaration (at the top
> of the function).

Oh, it's only now that I actually see an initializer is being added.
I'm sorry for the noise then.

> If we want to keep the compiler "uninitialized variable" analysis
> around, that would have to go away and we'd want to do something like
> was there before.  (I'm ambivalent about it, but I know Jan likes it.)

No, I'm fine with the change. I was just being blind.

Jan


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