Re: [Xen-devel] [PATCH v2 13/48] xen/sched: add is_running indicator to struct sched_unit

[Jan Beulich <jbeulich@suse.com>]

On 09.08.2019 16:57, Juergen Gross wrote:
> --- a/xen/common/schedule.c
> +++ b/xen/common/schedule.c
> @@ -407,6 +407,8 @@ int sched_init_vcpu(struct vcpu *v, unsigned int processor)
>      {
>          get_sched_res(v->processor)->curr = unit;
>          v->is_running = 1;
> +        unit->is_running = 1;
> +        unit->state_entry_time = NOW();
>      }
>      else
>      {

Are idle vCPU-s also going to get grouped into units (I'm sorry,
I don't recall)? If so, just like further down I'd be putting
under question the multiple writing of the field. I'd kind of
expect the unit and all vCPU-s within it to get an identical
state entry time stored.

Also both here and further down I'm puzzled to see that the
unit's field doesn't get set at the same place in code where
the vCPU's field gets set.

> @@ -1663,8 +1666,10 @@ static void schedule(void)
>       * switch, else lost_records resume will not work properly.
>       */
>  
> -    ASSERT(!next->is_running);
> +    ASSERT(!next->sched_unit->is_running);
>      next->is_running = 1;
> +    next->sched_unit->is_running = 1;
> +    next->sched_unit->state_entry_time = now;

Isn't the ASSERT() you delete still supposed to be true? In which case
wouldn't it be worthwhile to retain it?

> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -272,6 +272,11 @@ struct sched_unit {
>      struct sched_resource *res;
>      int                    unit_id;
>  
> +    /* Last time unit got (de-)scheduled. */
> +    uint64_t               state_entry_time;
> +
> +    /* Currently running on a CPU? */
> +    bool                   is_running;
>      /* Does soft affinity actually play a role (given hard affinity)? */
>      bool                   soft_aff_effective;
>      /* Bitmask of CPUs on which this VCPU may run. */

I'm noticing this here, but it may well have been an issue earlier
already (and there may well be later adjustments invalidating my
remark, and of course it's the end result of this series which
matters in the long run): Could you see about adding/removing
fields to this struct (and generally of course also others)
minimizing the number / overall size of holes?

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel