Re: [PATCH v2 10/11] x86/vpt: remove vPT timers per-vCPU lists

[Jan Beulich <jbeulich@xxxxxxxx>]

On 30.09.2020 12:41, Roger Pau Monne wrote:
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -1964,7 +1964,7 @@ void context_switch(struct vcpu *prev, struct vcpu 
> *next)
>      vpmu_switch_from(prev);
>      np2m_schedule(NP2M_SCHEDLE_OUT);
>  
> -    if ( is_hvm_domain(prevd) && !list_empty(&prev->arch.hvm.tm_list) )
> +    if ( is_hvm_domain(prevd) )
>          pt_save_timer(prev);

While most of the function goes away, pt_freeze_time() now will get
called in cases it previously wasn't called - is this benign?

> @@ -195,50 +182,20 @@ static void pt_thaw_time(struct vcpu *v)
>  
>  void pt_save_timer(struct vcpu *v)
>  {
> -    struct list_head *head = &v->arch.hvm.tm_list;
> -    struct periodic_time *pt;
> -
> -    if ( v->pause_flags & VPF_blocked )
> -        return;
> -
> -    pt_vcpu_lock(v);
> -
> -    list_for_each_entry ( pt, head, list )
> -        if ( !pt->do_not_freeze )
> -            stop_timer(&pt->timer);
>  
>      pt_freeze_time(v);
> -
> -    pt_vcpu_unlock(v);
>  }
>  
>  void pt_restore_timer(struct vcpu *v)
>  {
> -    struct list_head *head = &v->arch.hvm.tm_list;
> -    struct periodic_time *pt;
> -
> -    pt_vcpu_lock(v);
> -
> -    list_for_each_entry ( pt, head, list )
> -        if ( pt->pending_intr_nr == 0 )
> -            set_timer(&pt->timer, pt->scheduled);
> -
>      pt_thaw_time(v);
> -
> -    pt_vcpu_unlock(v);
>  }

In both functions the single function called also is the only
time it is used anywhere, so I guess the extra layer could be
removed.

> @@ -402,8 +339,7 @@ void create_periodic_time(
>      write_lock(&v->domain->arch.hvm.pl_time->pt_migrate);
>  
>      pt->pending_intr_nr = 0;
> -    pt->do_not_freeze = 0;
> -    pt->irq_issued = 0;
> +    pt->masked = false;

I agree here, but ...

> @@ -479,10 +412,8 @@ void destroy_periodic_time(struct periodic_time *pt)
>          return;
>  
>      pt_lock(pt);
> -    if ( pt->on_list )
> -        list_del(&pt->list);
> -    pt->on_list = 0;
>      pt->pending_intr_nr = 0;
> +    pt->masked = false;

... why not "true" here, at the very least for pt_active()'s sake?

> --- a/xen/include/asm-x86/hvm/vpt.h
> +++ b/xen/include/asm-x86/hvm/vpt.h
> @@ -31,13 +31,10 @@
>  typedef void time_cb(struct vcpu *v, void *opaque);
>  
>  struct periodic_time {
> -    struct list_head list;
> -    bool on_list;
>      bool one_shot;
> -    bool do_not_freeze;
> -    bool irq_issued;
>      bool warned_timeout_too_short;
>      bool level;
> +    bool masked;

"masked" aiui doesn't say anything about the present state of a
timer, but about its state the last time an interrupt was
attempted to be injected. If this is right, either a name change
("last_seen_masked" is somewhat longish) might be helpful, but
at the very least I'd like to ask for a comment to this effect.

> @@ -158,7 +153,7 @@ void pt_adjust_global_vcpu_target(struct vcpu *v);
>  void pt_may_unmask_irq(struct domain *d, struct periodic_time *vlapic_pt);
>  
>  /* Is given periodic timer active? */
> -#define pt_active(pt) ((pt)->on_list || (pt)->pending_intr_nr)
> +#define pt_active(pt) !(pt)->masked

This wants parentheses around it. And why does the right side of the
|| go away?

Jan