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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 20.03.2024 16:46, George Dunlap wrote:
> 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.

Neither am I, just that ...

>  Both are kind of "code smell"-y to me, but I think the second seems worse.

... it was the other way around to me.

> 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?

Yeah, why not.

Jan

> 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>