Re: [Xen-devel] [PATCH] xen: credit2: document that min_rqd is valid and ok to use

[Dario Faggioli <dfaggioli@suse.com>]

[-- Attachment #1: Type: text/plain, Size: 2804 bytes --]

On Mon, 2020-10-26 at 10:43 +0000, George Dunlap wrote:
> On Thu, Mar 26, 2020 at 5:09 PM Dario Faggioli <dfaggioli@suse.com>
> wrote:
> > diff --git a/xen/common/sched/credit2.c
> > b/xen/common/sched/credit2.c
> > index c7241944a8..9da51e624b 100644
> > --- a/xen/common/sched/credit2.c
> > +++ b/xen/common/sched/credit2.c
> > @@ -2387,6 +2387,13 @@ csched2_res_pick(const struct scheduler
> > *ops, const struct sched_unit *unit)
> >          goto out_up;
> >      }
> > 
> > +    /*
> > +     * If we're here, min_rqd must be valid. In fact, either we
> > picked a
> > +     * runqueue in the "list_for_each" (as min_avgload is
> > initialized to
> > +     * MAX_LOAD) or we just did that (in the "else" branch) above.
> > +     */
> 
> 
> Sorry it's taken so long to get back to you on this.
> 
> The problem with this is that there are actually *three* alternate
> clauses above:
> 
> 1. (has_soft && min_s_rqd)
> 2. min_rqd
> 3. <none of the above>
> 
Yes, indeed.

However, one of the three is "if (min_rqs)", and I think it is clear
that in that case (which would be 2 in the list above) min_rqd is
valid.

Therefore, this part of the comment "In fact, either we picked a
runqueue in the "list_for_each" (as min_avgload is initialized to
MAX_LOAD)", was referring to 1.

And this other part "or we just did that (in the "else" branch) above",
was referring to 3.

> It's obvious that if we hit #2 or #3, that min_rqd will be set.  But
> it's not immediately obvious why the condition in #1 guarantees that
> min_rqd will be set.
> 
That's what I tried to explain with this: "we picked a runqueue in the
"list_for_each" (as min_avgload is initialized to MAX_LOAD)"

> Is it because if we get to the point in the above loop where
> min_s_rqd is set, then min_rqd will always be set if it hasn't been
> set already?  Or to put it a different way -- the only way for
> min_rqd *not* to be set is if it always bailed before min_s_rqd was
> set?
> 
The point is really that the "list_for_each" loop scans all the
runqueues. If we do at least one step of the loop, min_rqd is ok,
because min_avgload is initialized to MAX_LOAD, and hence we have done
at least one assignment of min_rq=rqd (in the body of the very last if
of the loop itself).

min_s_rqd may or may not have been set to point to any runqueue. But if
it is valid, it means we have done at least one step of the loop, and
hence min_rqd is valid too.

Makes sense? :-)

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 #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]