Re: [Xen-devel] [PATCH v2 5/7] xen: credit1: increase efficiency and scalability of load balancing.

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 07/04/17 15:49, Dario Faggioli wrote:
> On Fri, 2017-04-07 at 15:38 +0100, George Dunlap wrote:
>> On 06/04/17 09:16, Dario Faggioli wrote:
>>> --- a/xen/common/sched_credit.c
>>> +++ b/xen/common/sched_credit.c
>>> @@ -1738,14 +1800,13 @@ csched_load_balance(struct csched_private
>>> *prv, int cpu,
>>>                   * could cause a deadlock if the peer CPU is also
>>> load
>>>                   * balancing and trying to lock this CPU.
>>>                   */
>>> -                spinlock_t *lock =
>>> pcpu_schedule_trylock(peer_cpu);
>>> +                lock = pcpu_schedule_trylock(peer_cpu);
>>>                  SCHED_STAT_CRANK(steal_trylock);
>>>                  if ( !lock )
>>>                  {
>>>                      SCHED_STAT_CRANK(steal_trylock_failed);
>>>                      TRACE_2D(TRC_CSCHED_STEAL_CHECK, peer_cpu, /*
>>> skipp'n */ 0);
>>> -                    peer_cpu = cpumask_cycle(peer_cpu, &workers);
>>> -                    continue;
>>> +                    goto next_cpu;
>>
>> Wait -- does this mean that before this patch, this effectively
>> busy-waited until peer_cpu was actually free (since peer_cpu was
>> never
>> incremented)?
>>
> Err, what do you mean? peer_cpu is updated here, with the result of
> cpumask_cycle(), run on workers, starting from the the current value of
> peer_cpu itself.
> 
> So, no, we don't busy wait try-locking on the same pcpu, we try all of
> them, one after the other (in a node-wise fashion, thanks to the outer
> loop).

Oh, right -- sorry, missed that. :-)

>> I like the idea in general, but I'm not a fan of the current way of
>> doing the accounting -- it seems like too many special cases.  Let me
>> have a think about this and come back to it.
>>
> Ok. I'm not sure I see what you mean with 'accounting' in this context,
> but, yeah, go ahead and let me know. :-)

Well I don't like having the increase_* and decrease_* all over the
place, *almost* corresponding with __runq_insert() and __runq_remove(),
but not quite.  It seems like trying to sort out most of the refcounting
inside fo those two functions (perhaps with runq_insert() which did
reference counting, and __runq_insert() that didn't, or something  like
that) would be a better approach.

If you're going to re-send the series, maybe it would be best if you
sent this patch last, so we can commit the rest?

 -George

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