Re: [Xen-devel] [PATCH v2 2/5] xen: sched/credit1: make vcpu_migration_delay per-cpupool

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 02/28/2018 02:23 PM, George Dunlap wrote:
> On 02/23/2018 04:41 PM, Dario Faggioli wrote:
>> Right now, vCPU migration delay is controlled by
>> the vcpu_migration_delay boot parameter. This means
>> the same value will always be used for every instance
>> of Credit1, in any cpupool that will be created.
>>
>> Also, in order to get and set such value, a special
>> purpose libxc interface is defined, and used by the
>> xenpm tool. And this is problematic if Xen is built
>> without Credit1 support.
>>
>> This commit adds a vcpu_migr_delay field inside
>> struct csched_private, so that we can get/set the
>> migration delay indepently for each Credit1 instance,
>> in different cpupools.
>>
>> Getting and setting now happens via XEN_SYSCTL_SCHEDOP_*,
>> which is much better suited for this parameter.
>>
>> The value of the boot time parameter is used for
>> initializing the vcpu_migr_delay field of the private
>> structure of all the scheduler instances, when they're
>> created.
>>
>> While there, save reading NOW() and doing any s_time_t
>> operation, when the migration delay of a scheduler is
>> zero (as it is, by default), in
>> __csched_vcpu_is_cache_hot().
>>
>> Finally, note that, from this commit on, using `xenpm
>> {set,get}-vcpu-migration-delay' is not effective any
>> longer.
>>
>> Signed-off-by: Dario Faggioli <dfaggioli@xxxxxxxx>
>> ---
>> Cc: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
>> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> ---
>>  xen/common/sched_credit.c   |   38 +++++++++++++++++++++++++++-----------
>>  xen/include/public/sysctl.h |    3 +++
>>  2 files changed, 30 insertions(+), 11 deletions(-)
>>
>> diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c
>> index a2c5d43e33..fd6dbd02aa 100644
>> --- a/xen/common/sched_credit.c
>> +++ b/xen/common/sched_credit.c
>> @@ -214,7 +214,7 @@ struct csched_private {
>>  
>>      /* Period of master and tick in milliseconds */
>>      unsigned int tick_period_us, ticks_per_tslice;
>> -    s_time_t ratelimit, tslice;
>> +    s_time_t ratelimit, tslice, vcpu_migr_delay;
>>  
>>      struct list_head active_sdom;
>>      uint32_t weight;
>> @@ -690,11 +690,11 @@ unsigned int get_vcpu_migration_delay(void)
>>      return vcpu_migration_delay;
>>  }
>>  
>> -static inline int
>> -__csched_vcpu_is_cache_hot(struct vcpu *v)
>> +static inline bool
>> +__csched_vcpu_is_cache_hot(const struct csched_private *prv, struct vcpu *v)
>>  {
>> -    int hot = ((NOW() - v->last_run_time) <
>> -               ((uint64_t)vcpu_migration_delay * 1000u));
>> +    bool hot = prv->vcpu_migr_delay &&
>> +               (NOW() - v->last_run_time) < prv->vcpu_migr_delay;
>>  
>>      if ( hot )
>>          SCHED_STAT_CRANK(vcpu_hot);
>> @@ -703,7 +703,8 @@ __csched_vcpu_is_cache_hot(struct vcpu *v)
>>  }
>>  
>>  static inline int
>> -__csched_vcpu_is_migrateable(struct vcpu *vc, int dest_cpu, cpumask_t *mask)
>> +__csched_vcpu_is_migrateable(const struct csched_private *prv, struct vcpu 
>> *vc,
>> +                             int dest_cpu, cpumask_t *mask)
>>  {
>>      /*
>>       * Don't pick up work that's hot on peer PCPU, or that can't (or
>> @@ -714,7 +715,7 @@ __csched_vcpu_is_migrateable(struct vcpu *vc, int 
>> dest_cpu, cpumask_t *mask)
>>       */
>>      ASSERT(!vc->is_running);
>>  
>> -    return !__csched_vcpu_is_cache_hot(vc) &&
>> +    return !__csched_vcpu_is_cache_hot(prv, vc) &&
>>             cpumask_test_cpu(dest_cpu, mask);
>>  }
>>  
>> @@ -1251,7 +1252,9 @@ csched_sys_cntl(const struct scheduler *ops,
>>               || (params->ratelimit_us
>>                   && (params->ratelimit_us > XEN_SYSCTL_SCHED_RATELIMIT_MAX
>>                       || params->ratelimit_us < 
>> XEN_SYSCTL_SCHED_RATELIMIT_MIN))
>> -             || MICROSECS(params->ratelimit_us) > 
>> MILLISECS(params->tslice_ms) )
>> +             || MICROSECS(params->ratelimit_us) > 
>> MILLISECS(params->tslice_ms)
>> +             || (params->vcpu_migr_delay_us
>> +                 && MICROSECS(params->vcpu_migr_delay_us) >= 
>> XEN_SYSCTL_CSCHED_MGR_DLY_MAX) )
>>                  goto out;
>>  
>>          spin_lock_irqsave(&prv->lock, flags);
>> @@ -1261,12 +1264,14 @@ csched_sys_cntl(const struct scheduler *ops,
>>          else if ( prv->ratelimit && !params->ratelimit_us )
>>              printk(XENLOG_INFO "Disabling context switch rate limiting\n");
>>          prv->ratelimit = MICROSECS(params->ratelimit_us);
>> +        prv->vcpu_migr_delay = MICROSECS(params->vcpu_migr_delay_us);
>>          spin_unlock_irqrestore(&prv->lock, flags);
>>  
>>          /* FALLTHRU */
>>      case XEN_SYSCTL_SCHEDOP_getinfo:
>>          params->tslice_ms = prv->tslice / MILLISECS(1);
>>          params->ratelimit_us = prv->ratelimit / MICROSECS(1);
>> +        params->vcpu_migr_delay_us = prv->vcpu_migr_delay / MICROSECS(1);
>>          rc = 0;
>>          break;
>>      }
>> @@ -1608,6 +1613,7 @@ csched_tick(void *_cpu)
>>  static struct csched_vcpu *
>>  csched_runq_steal(int peer_cpu, int cpu, int pri, int balance_step)
>>  {
>> +    const struct csched_private * const prv = 
>> CSCHED_PRIV(per_cpu(scheduler, cpu));
>>      const struct csched_pcpu * const peer_pcpu = CSCHED_PCPU(peer_cpu);
>>      struct csched_vcpu *speer;
>>      struct list_head *iter;
>> @@ -1657,7 +1663,7 @@ csched_runq_steal(int peer_cpu, int cpu, int pri, int 
>> balance_step)
>>              continue;
>>  
>>          affinity_balance_cpumask(vc, balance_step, cpumask_scratch);
>> -        if ( __csched_vcpu_is_migrateable(vc, cpu, cpumask_scratch) )
>> +        if ( __csched_vcpu_is_migrateable(prv, vc, cpu, cpumask_scratch) )
>>          {
>>              /* We got a candidate. Grab it! */
>>              TRACE_3D(TRC_CSCHED_STOLEN_VCPU, peer_cpu,
>> @@ -2114,7 +2120,7 @@ csched_dump(const struct scheduler *ops)
>>             "\tratelimit          = %"PRI_stime"us\n"
>>             "\tcredits per msec   = %d\n"
>>             "\tticks per tslice   = %d\n"
>> -           "\tmigration delay    = %uus\n",
>> +           "\tmigration delay    = %"PRI_stime"us\n",
>>             prv->ncpus,
>>             prv->master,
>>             prv->credit,
>> @@ -2126,7 +2132,7 @@ csched_dump(const struct scheduler *ops)
>>             prv->ratelimit / MICROSECS(1),
>>             CSCHED_CREDITS_PER_MSEC,
>>             prv->ticks_per_tslice,
>> -           vcpu_migration_delay);
>> +           prv->vcpu_migr_delay/ MICROSECS(1));
>>  
>>      cpumask_scnprintf(idlers_buf, sizeof(idlers_buf), prv->idlers);
>>      printk("idlers: %s\n", idlers_buf);
>> @@ -2209,6 +2215,16 @@ csched_init(struct scheduler *ops)
>>      }
>>      else
>>          prv->ratelimit = MICROSECS(sched_ratelimit_us);
>> +
>> +    if ( !vcpu_migration_delay && vcpu_migration_delay > MILLISECS(100) )
> 
> Also, this expression can never evaluate to true; the first element
> requires 0, the second requires non-zero.  I think you want to get rid
> of the `!`.

Or, by the same argument, get rid of the first clause altogether, as if
it's > 100 then it cannot possibly be zero. :-)

 -George

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