Re: [Xen-devel] [PATCH] xen: sched: Credit2: during scheduling, update the idle mask before using it

[Dario Faggioli <dfaggioli@xxxxxxxx>]

On Thu, 2018-10-11 at 15:19 +0100, Jan Beulich wrote:
> > > > On 11.10.18 at 15:44, <dfaggioli@xxxxxxxx> wrote:
> > Not doing that, in fact, may cause runq_tickle() to think that the
> > cpu
> > is still idle, and tickle it to go pick up a vcpu from the
> > runqueue,
> > which might be wrong/unideal.
> 
> Makes sense to me; I seem to vaguely recall that something
> along these lines was done years ago for credit1 as well.
> 
Mmm... yes, there have been a few, not identical, but vaguely similar
(or, at least, related to making sure that we actually tickle idle
cpus, etc).

> > Backporting: this does not fix a system crash. However, it fixes
> > the
> > behavior of the scheduler --which I'd call wrong more than just
> > suboptimal.
> > 
> > Therefore, I'd be inclined to ask for this to be backported. It
> > should
> > be fairly straightforward, but as usual, I'm up for helping with
> > that.
> 
> I'll try to remember to pick it up when I've seen it go in.
>
Thanks. I'll try myself keeping an eye on the staging trees at relevant
times.

> > --- a/xen/common/sched_credit2.c
> > +++ b/xen/common/sched_credit2.c
> > @@ -3554,6 +3554,13 @@ csched2_schedule(
> >              __set_bit(__CSFLAG_scheduled, &snext->flags);
> >          }
> >  
> > +        /* Clear the idle mask if necessary */
> > +        if ( cpumask_test_cpu(cpu, &rqd->idle) )
> > +        {
> > +            __cpumask_clear_cpu(cpu, &rqd->idle);
> > +            smt_idle_mask_clear(cpu, &rqd->smt_idle);
> > +        }
> 
> I realize you're merely moving code, but is there a reason to do
> the test-and-clear in two steps rather than one. It being the
> non-atomic variant, it can't be shared memory, and hence the
> cache line ping-pong consideration applicable in other cases is
> irrelevant here afaict.
> 
Indeed it's not that, IMO. In fact, access to this mask is serialized
via a spinlock, so there can't be two CPUs trying to read/update it
concurrently.

I don't know, I guess the original structure of the code is inherited
from Credit1 (where things are _not_ serialized, so trying to avoid
ping-pong makes sense).

Then, when I did 222234f2ad1 "xen: credit2: use non-atomic cpumask and
bit operations", I didn't go as far as I should have. It's weird
because ISTR having thought about it, but I may be confusing this with
something else.

In any event, I think we very well can turn these in
__test_and_clear/set(). But I'd do that in a separate patch.

Thanks and Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Software Engineer @ SUSE https://www.suse.com/

Attachment: signature.asc
Description: This is a digitally signed message part

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel