Re: [Xen-devel] [PATCH v2] sched: credit2: respect per-vcpu hard affinity

[Justin Weaver <jtweaver@xxxxxxxxxx>]

Dario,

>> Without the processor assignment
>> here the vcpu might go on being assigned to a processor it no longer
>> is allowed to run on.
>>
> Ok.
>
>> In that case, function runq_candidate may only
>> get called for the vcpu's old processor, and runq_candidate will no
>> longer let a vcpu run on a processor that it's not allowed to run on
>> (because of the hard affinity check first introduced in v1 of this
>> patch).
>>
> It mostly makes sense. Out of the top of my head, it still looks like
> there should be a pCPU that, when scheduling, would pick it up... I need
> to think more about this...
>
>> So in that condition the vcpu never gets to run. That's still
>> somewhat of a vague explanation, but I have observed that that is what
>> happens.
>>
> Do you mean you _actually_ saw this, with some debugging printk-s, or
> tracing, or something like this?

Yes, I saw the above behavior using printk-s. I commented out the line
that does the processor assignment if the run queues are the same. I
put some printks in key spots. For the test, I had only one guest vcpu
running; it had hard affinity with all cpus and I used xl vcpu-list to
see that it was on cpu 15 (two runqs 0-7,8-15). I then pinned it to 9
and it became unresponsive (vcpu-list showed ---), and printk output
only showed over and over and over again output from runq_candidate
saying something like 'vcpu can't run on cpu 15, not in hard affinity
mask'. A call to runq_candidate for cpu 9 never came up. Pinning it
back to 15 (or all) started it running again.


>> diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
>> index cf53770..de8fb5a 100644
>> --- a/xen/common/sched_credit2.c
>> +++ b/xen/common/sched_credit2.c
>
>> @@ -2127,16 +2212,25 @@ csched2_init(struct scheduler *ops)
>>
>>      prv->load_window_shift = opt_load_window_shift;
>>
>> +    cpumask = xzalloc_bytes(nr_cpu_ids * sizeof(cpumask_t *));
>> +    if ( cpumask == NULL )
>> +        return -ENOMEM;
>> +
> This can probably be xzalloc_array(), or even xmalloc_array(), if we
> don't care zeroing all elements (see below about why I actually don't
> think we need).

OK, I'll make this change. Yes, they don't have to be zeroed based on
what you point out below.

>>      return 0;
>>  }
>>
>>  static void
>>  csched2_deinit(const struct scheduler *ops)
>>  {
>> +    int i;
>>      struct csched2_private *prv;
>>
>>      prv = CSCHED2_PRIV(ops);
>>      xfree(prv);
>> +
>> +    for ( i = 0; i < nr_cpu_ids; i++ )
>> +        free_cpumask_var(cpumask[i]);
>> +    xfree(cpumask);
>>
> Do we need the loop? I mean, all the pcpus go through
> csched2_alloc_pdata(), when being activated under this scheduler, which
> allocates their scratch masks. They then go through
> csched2_free_pdata(), when deactivated from this scheduler, which frees
> their scratch masks.
>
> I would then expect that, when we get to here, all the elemtns of the
> scratch mask array that were allocated, have also been freed, and hence
> only freeing the array is necessary.
>
> Am I missing something?

I agree; what we allocate in csched2_init is what should be
deallocated in csched2_deinit. They should match. I'll make the
change.

Thanks,
Justin

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