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

[Juergen Gross <jgross@xxxxxxxx>]

On 01/03/16 12:27, Jan Beulich wrote:
>>>> 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?

Sure, NP.

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

Okay. I don't mind changing the name. We should just agree on one.

> 
>> +{
>> +    struct vcpu *v = current;
>> +    spinlock_t *lock;
>> +    long ret = -EINVAL;
> 
> "int" seems completely sufficient for both the variable and the
> function return type.

Hmm, yes.

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

Will do.

> 
>> --- 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.

Will change it.


Juergen

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