Re: [Xen-devel] [PATCH v2 1/3] x86/vlapic: Introduce vlapic_update_timer

["Jan Beulich" <jbeulich@xxxxxxxx>]

>>> Anthony PERARD <anthony.perard@xxxxxxxxxx> 08/04/17 12:52 PM >>>
>On Thu, Aug 03, 2017 at 08:59:10AM -0600, Jan Beulich wrote:
>> >>> Anthony PERARD <anthony.perard@xxxxxxxxxx> 07/18/17 7:10 PM >>>
>> >+static void vlapic_update_timer(struct vlapic *vlapic, uint32_t lvtt);
>> >+{
>> >+    uint64_t period;
>> >+    uint64_t delta;
>> 
>> Why two lines (but see also below)?
>
>Why not? Anyway, I'll change it.
>
>Also I realize that delta is going to be initialize to 0 here in the
>next patch, which is why I think there is two lines.

For both this and ...

>> >+    bool is_periodic;
>> >+
>> >+    is_periodic = (lvtt & APIC_TIMER_MODE_MASK) == 
>> >APIC_TIMER_MODE_PERIODIC;
>> >+
>> >+    period = (uint64_t)vlapic_get_reg(vlapic, APIC_TMICT)
>> >+        * APIC_BUS_CYCLE_NS * vlapic->hw.timer_divisor;
>> >+
>> >+    /* Calculate the next time the timer should trigger an interrupt. */
>> >+    delta = period;
>> 
>> What is the point of having the same value in two variables?
>
>It might look like it but there are not the same values, the description
>is accurate, even if the calculation at this stage is very simple.
>
>More importantly, this line is going away in the next patch and will be
>replaced by a more complexe calculation.

... and this - irrespective of subsequent patches, the one here would better
be self-contained, or otherwise its description should point out that certain
things are done in a way easing subsequent ones (but only if that was really
the case, which I don't think it is here - as you say, the questionable
constructs are being touched again later anyway, so could as well be left
out).

Jan


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