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

[George Dunlap <dunlapg@xxxxxxxxx>]

On Thu, Mar 26, 2020 at 5:09 PM Dario Faggioli <dfaggioli@xxxxxxxx> 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@xxxxxxxx>
> Signed-off-by: Dario Faggioli <dfaggioli@xxxxxxxx>
> ---
> Cc: Jürgen Groß <jgross@xxxxxxxx>
> ---
>  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