Re: [Xen-devel] [PATCH v2 2/3] xen: add hypercall option to temporarily pin a vcpu

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 01.03.16 at 10:02, <JGross@xxxxxxxx> wrote:
> @@ -752,14 +766,20 @@ static int vcpu_set_affinity(
>      struct vcpu *v, const cpumask_t *affinity, cpumask_t *which)
>  {
>      spinlock_t *lock;
> +    int ret = 0;
>  
>      lock = vcpu_schedule_lock_irq(v);
>  
> -    cpumask_copy(which, affinity);
> +    if ( v->affinity_broken )
> +        ret = -EBUSY;
> +    else
> +    {
> +        cpumask_copy(which, affinity);
>  
> -    /* Always ask the scheduler to re-evaluate placement
> -     * when changing the affinity */
> -    set_bit(_VPF_migrating, &v->pause_flags);
> +        /* Always ask the scheduler to re-evaluate placement
> +         * when changing the affinity */
> +        set_bit(_VPF_migrating, &v->pause_flags);

When you touch code like this, would it be possible to at once fix
the coding style issues it (the comment in this case) has?

> @@ -978,6 +998,51 @@ void watchdog_domain_destroy(struct domain *d)
>          kill_timer(&d->watchdog_timer[i]);
>  }
>  
> +static long do_pin_temp(int cpu)

As expressed before, throughout this patch I dislike the "temp"
naming, when the temporary nature of this operation isn't being
enforced by anything.

Apart from that I (vaguely) recall there having been previous
suggestions in the direction of (temporary), which have got
rejected.

On both points I think we need to have input from the scheduler
maintainers.

> +{
> +    struct vcpu *v = current;
> +    spinlock_t *lock;
> +    long ret = -EINVAL;

"int" seems completely sufficient for both the variable and the
function return type.

> @@ -1087,6 +1152,23 @@ ret_t do_sched_op(int cmd, 
> XEN_GUEST_HANDLE_PARAM(void) arg)
>          break;
>      }
>  
> +    case SCHEDOP_pin_temp:
> +    {
> +        struct sched_pin_temp sched_pin_temp;
> +
> +        ret = -EFAULT;
> +        if ( copy_from_guest(&sched_pin_temp, arg, 1) )
> +            break;
> +
> +        ret = -EPERM;
> +        if ( !is_hardware_domain(current->domain) )
> +            break;

I'd generally suggest swapping these two.

> --- a/xen/include/public/sched.h
> +++ b/xen/include/public/sched.h
> @@ -118,6 +118,17 @@
>   * With id != 0 and timeout != 0, poke watchdog timer and set new timeout.
>   */
>  #define SCHEDOP_watchdog    6
> +
> +/*
> + * Temporarily pin the current vcpu to one physical cpu or undo that pinning.
> + * @arg == pointer to sched_pin_temp_t structure.
> + *
> + * Setting pcpu to -1 will undo a previous temporary pinning and restore the
> + * previous cpu affinity. The temporary aspect of the pinning isn't enforced
> + * by the hypervisor.

This comment is now out of sync with the code, since you now
accept any negative CPU number as "undo" request.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel