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

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Fri, Aug 04, 2017 at 09:40:32AM -0600, Jan Beulich wrote:
> >>> 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).

Fine, I'll get rid of delta in this patch.

-- 
Anthony PERARD

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