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

[George Dunlap <dunlapg@umich.edu>]

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