[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH] xen: sched: Credit2: during scheduling, update the idle mask before using it
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 _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxxxxxxxxx https://lists.xenproject.org/mailman/listinfo/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |