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

[George Dunlap <george.dunlap@xxxxxxxxxx>]

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.

I think "will have no effect" would probably be more clear; it's also
probably worth mentioning that you're planning on providing a
backwards-compatibility option in a subsequent patch.

[snip]

> diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
> index 3669e32524..ab62ffafa9 100644
> --- a/xen/include/public/sysctl.h
> +++ b/xen/include/public/sysctl.h
> @@ -601,6 +601,9 @@ struct xen_sysctl_credit_schedule {
>  #define XEN_SYSCTL_CSCHED_TSLICE_MIN 1
>      unsigned tslice_ms;
>      unsigned ratelimit_us;
> +    /* How long to consider a vCPU cache-hot on the CPU where it is
running */
> +#define XEN_SYSCTL_CSCHED_MGR_DLY_MAX (100 * 1000) /* 100us, in
millisecs */

Is this supposed to be "100ms in microseconds"?  Unless I'm confused
there are some serious unit conversion mix-ups in this patch; I think it
would be better to add the '_US' suffix to this to help clarify things.

[snip]

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

Is there really a point to checking to see if it's zero, just to make
sure that it's not greater than XEN_SYSCTL_CSCHED_MGR_DLY_MAX?  If it's
zero it certainly won't be greater than that value, assuming that value
isn't negative.

Also, if both vcpu_migr_delay_us and XEN_SYSCTL_CSCHED_MGR_DLY_MAX are
in microseconds, why are you doing any conversion at all (much less
converting only one of them)?

> +    if ( !vcpu_migration_delay && vcpu_migration_delay > MILLISECS(100) )

Why is this hardcoded, rather than using MGR_DLY_MAX?  Also...

> +    {
> +        vcpu_migration_delay = 0;
> +        printk("WARNING: vcpu_migration_delay outside of valid range 
> [%d,%d]us.\n"
> +               "Resetting to default: %u\n",
> +               0, XEN_SYSCTL_CSCHED_MGR_DLY_MAX, vcpu_migration_delay);
> +    }
> +    prv->vcpu_migr_delay = MICROSECS(vcpu_migration_delay);

Something here can't be right -- what unit is vcpu_migration_delay in?
If it's in microseconds, why are you comparing it to MILLISECS(100)
above (which would be in nanoseconds)?  If it's already in nanoseconds,
why are you converting it from us to ns here?

 -George

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