On Wed, Mar 20, 2024 at 1:45 PM Jan Beulich <jbeul...@suse.com> wrote:
>
> On 18.03.2024 17:35, Andrew Cooper wrote:
> > @@ -736,9 +736,19 @@ static void vlapic_update_timer(struct vlapic *vlapic,
> > uint32_t lvtt,
> > delta = delta * vlapic->hw.timer_divisor / old_divisor;
> > }
> >
> > - TRACE_2_LONG_3D(TRC_HVM_EMUL_LAPIC_START_TIMER,
> > TRC_PAR_LONG(delta),
> > - TRC_PAR_LONG(is_periodic ? period : 0),
> > - vlapic->pt.irq);
> > + if ( unlikely(tb_init_done) )
> > + {
> > + struct {
> > + uint64_t delta, period;
> > + uint32_t irq;
> > + } __packed d = {
> > + .delta = delta,
> > + .period = is_periodic ? period : 0,
> > + .irq = vlapic->pt.irq,
> > + };
> > +
> > + trace_time(TRC_HVM_EMUL_LAPIC_START_TIMER, sizeof(d), &d);
> > + }
>
> Instead of this open-coding, how about
>
> if ( is_periodic )
> TRACE_TIME(TRC_HVM_EMUL_LAPIC_START_TIMER, delta, delta >> 32,
> period, period >> 32, vlapic->pt.irq);
> else
> TRACE_TIME(TRC_HVM_EMUL_LAPIC_START_TIMER, delta, delta >> 32,
> 0, 0, vlapic->pt.irq);
>
> thus improving similarity / grep-ability with ...
Yuck -- I'm not keen on breaking the similarity, but I'm *really* not
a fan of duplicating code. Both are kind of "code smell"-y to me, but
I think the second seems worse.
It sort of looks to me like the source of the problem is that the
`period` variable is overloaded somehow; in that it's used to update
some calculation even if !is_periodic, and so the two places it's used
as an actual period (the trace code, and the call to
`create_periodic_time()`) need to figure out if `periodic` is for them
to use or not.
What about adding a variable, "timer_period" for that purpose?
Something like the following?
diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
index dcbcf4a1fe..ea14fc1587 100644
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -729,6 +729,8 @@ static void vlapic_update_timer(struct vlapic
*vlapic, uint32_t lvtt,
if ( delta && (is_oneshot || is_periodic) )
{
+ uint64_t timer_period = 0;
+
if ( vlapic->hw.timer_divisor != old_divisor )
{
period = (uint64_t)vlapic_get_reg(vlapic, APIC_TMICT)
@@ -736,12 +738,15 @@ static void vlapic_update_timer(struct vlapic
*vlapic, uint32_t lvtt,
delta = delta * vlapic->hw.timer_divisor / old_divisor;
}
- TRACE_2_LONG_3D(TRC_HVM_EMUL_LAPIC_START_TIMER, TRC_PAR_LONG(delta),
- TRC_PAR_LONG(is_periodic ? period : 0),
- vlapic->pt.irq);
+ if ( is_periodic )
+ timer_period = period;
+
+ TRACE_TIME(TRC_HVM_EMUL_LAPIC_START_TIMER, delta, delta >> 32,
+ timer_period, timer_period >> 32,
+ vlapic->pt.irq);
create_periodic_time(current, &vlapic->pt, delta,
- is_periodic ? period : 0, vlapic->pt.irq,
+ timer_period, vlapic->pt.irq,
is_periodic ? vlapic_pt_cb : NULL,
&vlapic->timer_last_update, false);
As with Jan, I'd be OK with checking it in the way it is if you prefer, so:
Reviewed-by: George Dunlap <george.dun...@cloud.com>
