Re: [PATCH 5/7] xen: Switch to new TRACE() API

[George Dunlap <george.dunlap@xxxxxxxxx>]

On Wed, Mar 20, 2024 at 1:45 PM Jan Beulich <jbeulich@xxxxxxxx> 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.dunlap@xxxxxxxxx>