Re: [PATCH] Xen: credit2: avoid picking a spurious idle unit when caps are used

[Jan Beulich <jbeulich@xxxxxxxx>]

On 04.08.2021 15:28, Dario Faggioli wrote:
> On Wed, 2021-08-04 at 09:37 +0200, Jan Beulich wrote:
>> On 03.08.2021 19:36, Dario Faggioli wrote:
>>>
>>> Signed-off-by: Dario Faggioli <dfaggioli@xxxxxxxx>
>>> Suggested-by: George Dunlap <george.dunlap@xxxxxxxxxx>
>>
>> Minor remark: Generally I think the order of tags should follow the
>> timeline: Suggestions (or bug reports) come before patch creation,
>> which in turns comes before reviewing / acking of a patch.
>>
> Right. In fact, I agree, and I keep forgetting doing that.
> 
> Thanks, will fix.
> 
>>> Cc: George Dunlap <george.dunlap@xxxxxxxxxx>
>>> Cc: Jan Beulich <jbeulich@xxxxxxxx>
>>
>> Since George is on leave and since I was Cc-ed, I thought I'd make an
>> attempt at reviewing this. The more that ...
>>
> Yep. You were Cc-ed because of the request to backport and include in
> stable branches, but thank you very much for also taking the time to
> have a look at it!! :-)
> 
>>> It should, therefore, be backported and applied to all the branches
>>> to
>>> which that commit has been. About backports, it should be
>>> straigthforward to do that until 4.13.
>>
>> ... for 4.13.4 it would of course be nice to have it in. Things look
>> plausible overall, but I've got one question which - despite concerning
>> code you only move - may play into the underlying issue.
>>
> Ok.
> 
>>> For 4.12 and earlier, it's trickier, but the fix is still necessary.
>>> Actually, both 07b0eb5d0ef0 and this patch should be backported to
>>> that
>>> branch!
>>
>> Depends on what you target with this remark: For downstreams - yes. The
>> stable upstream branch, otoh, is out of general support, and since this
>> is not a security fix it is not going to be applied to that tree.
>>
> Yeah, I know. I decided to mention this (although, I probably could
> have made myself more clear) and provide a backport (of this and of the
> other, already committed patch) just for convenience of both users and
> downstreams that happens to use such codebases.
> 
>>>          /*
>>> -         * If an unit is meant to be picked up by another processor,
>>> and such
>>> -         * processor has not scheduled yet, leave it in the runqueue
>>> for him.
>>> +         * If the unit in the runqueue has more credit than current
>>> (or than
>>> +         * idle, if current is not runnable) or if current is
>>> yielding, we may
>>> +         * want to pick it up.
>>>           */
>>> -        if ( svc->tickled_cpu != -1 && svc->tickled_cpu != cpu &&
>>> -             cpumask_test_cpu(svc->tickled_cpu, &rqd->tickled) )
>>> +        if ( (yield || svc->credit > snext->credit) )
>>
>> The "credit" field is plain "int", i.e. signed. Idle domain's vCPU-s
>> don't get INT_MIN credit afaict (there's only one use of INT_MIN
>> throughout the entire file). Hence I can't see why in principle a
>> vCPU of an ordinary domain couldn't have equal or less credit than
>> the CPU's idle vCPU. 
>>
> So, if I understand what you mean, yes, we've had that issue, i.e.,
> vCPUs managing to get to credit values which were lower than the one of
> the idle vCPUs.
> 
> That was, in fact, even causing issue and it's what lead to
> 36f3662f27dec32d76c0edb4c6b62b9628d6869d "credit2: avoid vCPUs to ever
> reach lower credits than idle".
> 
> After that commit, idle vCPUs' credits are set to CSCHED2_CREDIT_MIN-1
> and, for regular vCPUs, whenever we subtract some value from their
> credits, we limit them to not go beyond CSCHED2_CREDIT_MIN (this
> happens in t2c_update(), called by burn_credit()).
> 
> Therefore, it should now not be possible any longer for regular vCPUs
> to fall behind idle vCPUs, in terms of amount of credits.
> 
> So, was it this you were asking about and, if yes, does this answer
> your concerns?

Yes, it does. I continue to think though that the "yield" variable
could do with either a comment along the lines of what you've
explained, or with it getting set to true in more cases (as
indicated), as the interaction with the credit comparisons isn't
very obvious right now (to me at least).

>> Otoh I'm having trouble seeing why all of this code movement / re-
>> indentation is necessary in the first place: If the initial if() was
>> inverted to
>>
>>         if ( !yield && svc->credit <= snext->credit )
>>             continue;
>>
> Actually, I am just realizing that if I, instead, use:
> 
>         if ( !yield && svc->credit <= snext->credit )
>             break;
> 
> It would be much better (even as compared to the current situation).
> 
> In fact, right now that the priority check is toward the end, we have
> do the checks that comes earlier in the loop (is it runnable? Can it
> run on this CPU? Is it worth migrating it? Etc) at least for one
> element of the runqueue.
> 
> With either my code or above the suggested form, we don't, but we may
> have to do at least the priority check for all the elements of the
> runqueue. This was something I knew, and as a matter of fact, it should
> be quick enough (and comparable with doing expensive checks even on
> just 1 vCPU). But still, it's ugly.
> 
> However, we know that the runqueue is sorted by credits!

Oh - I was first thinking it might be, but seeing all the logic I
(wrongly, as you now tell me) inferred it's unsorted.

Jan

> So, unless
> we're yielding, it is always the case that as soon as we find there an
> unit that has less credit than snext, we want to bail (and keep running
> snext).
> 
> This means that we neither scan all the runqueues, not even for doing
> just quick priority checks, nor we, in the case that snext is the
> actual highest priority unit need to do any check for the unit at the
> top of the runqueue.
> 
> So, I'm actually re-doing (and re-testing) the patch in this way.
> 
> Thanks again and Regards
>