| Message ID | 158524252335.30595.3422322089286433323.stgit@Palanthas (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen: credit2: document that min_rqd is valid and ok to use | expand |
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
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
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
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);
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(+)