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

[Dario Faggioli <dfaggioli@xxxxxxxx>]

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?

> 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! 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
-- 
Dario Faggioli, Ph.D
http://about.me/dario.faggioli
Virtualization Software Engineer
SUSE Labs, SUSE https://www.suse.com/
-------------------------------------------------------------------
<<This happens because _I_ choose it to happen!>> (Raistlin Majere)

Attachment: signature.asc
Description: This is a digitally signed message part