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

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

[Dario Faggioli]

Code is a bit involved, and it is not easy to tell that min_rqd, inside
csched2_res_pick() is actually pointing to a runqueue, when it is
dereferenced.

Add a comment and an ASSERT() for that.

Suggested-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Dario Faggioli <dfaggioli@suse.com>
---
Cc: Jürgen Groß <jgross@suse.com>
---
 xen/common/sched/credit2.c |    7 +++++++
 1 file changed, 7 insertions(+)

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.
+     */
+    ASSERT(min_rqd);
+
     new_cpu = cpumask_cycle(min_rqd->pick_bias, cpumask_scratch_cpu(cpu));
     min_rqd->pick_bias = new_cpu;
     BUG_ON(new_cpu >= nr_cpu_ids);

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

[Dario Faggioli]

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

On Thu, 2020-03-26 at 18:08 +0100, Dario Faggioli wrote:
> Code is a bit involved, and it is not easy to tell that min_rqd,
> inside
> csched2_res_pick() is actually pointing to a runqueue, when it is
> dereferenced.
> 
> Add a comment and an ASSERT() for that.
> 
> Suggested-by: Jan Beulich <jbeulich@suse.com>
> Signed-off-by: Dario Faggioli <dfaggioli@suse.com>
>
Ping.

Actually, for George, this is more a:

<<Hey, it seems I forgot to Cc you when I sent this. Apologies for
that!>> :-)

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 --]

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

[George Dunlap]

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

On Thu, Mar 26, 2020 at 5:09 PM Dario Faggioli <dfaggioli@suse.com> wrote:
>
> Code is a bit involved, and it is not easy to tell that min_rqd, inside
> csched2_res_pick() is actually pointing to a runqueue, when it is
> dereferenced.
>
> Add a comment and an ASSERT() for that.
>
> Suggested-by: Jan Beulich <jbeulich@suse.com>
> Signed-off-by: Dario Faggioli <dfaggioli@suse.com>
> ---
> Cc: Jürgen Groß <jgross@suse.com>
> ---
>  xen/common/sched/credit2.c |    7 +++++++
>  1 file changed, 7 insertions(+)
>
> 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>

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.

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?

 -George

[-- Attachment #2: Type: text/html, Size: 2330 bytes --]

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

[Dario Faggioli]

[-- 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 --]