[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v2 1/3] x86/vlapic: Introduce vlapic_update_timer
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
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |