Re: [Xen-devel] [Help] Trigger Watchdog when adding an IPI in vcpu_wake

[Wei Yang <richard.weiyang@xxxxxxxxxx>]

On Tue, Sep 20, 2016 at 01:24:17AM +0200, Dario Faggioli wrote:
>On Sat, 2016-09-17 at 00:31 +0000, Wei Yang wrote:
>> On Fri, Sep 16, 2016 at 06:07:08PM +0200, Dario Faggioli wrote:
>> > But then again, if the system is not oversubscribed, I'd tend to
>> > think
>> > it to be tolerable, and I'd expect the biggest problem to be the
>> > work-
>> > stealing logic (considering the high number of pcpus), rather than
>> > the
>> > duration of the critical sections within vcpu_wake().
>> > 
>> Yes, we are trying to improve the stealing part too.
>> 
>Great.
>
>> Sounds reasonable, vcpu_wake() is O(1) while "stealing" is O(N) in
>> terms of
>> #PCPUs.
>> 
>Exactly!
>
>> > I also think current upstream is a bit better, especially because
>> > it's
>> > mostly me that made it look the way it does. :-D
>> 
>> Ah, not intended to flattering you.
>> 
>EhEh! Sure, I was joking myself. :-)
>

Ah, really enjoy talking with you :-)
You got some special sense of humor, and I like it~

Have a good day!

>> > 
>> > 
>> > But I was actually describing how 4.1 works. In fact, in 4.1, if
>> > pcpu 6
>> > is idle (se the '//xxx xxx xxx' comments I'm adding to the code
>> > excerpts:
>> > 
>> >     if ( new->pri > cur->pri )  //is true, so we put pcpu 6 in mask
>> >     {
>> >         cpu_set(cpu, mask);
>> >     }
>> >     if ( cur->pri > CSCHED_PRI_IDLE )  //is false!!
>> >     {
>> >        ....
>> >     }
>> >     if ( !cpus_empty(mask) ) //the mask contains pcpu 6 only
>> >         cpumask_raise_softirq(mask, SCHEDULE_SOFTIRQ);
>> > 
>> 
>> Hmm... don't have the code at hand, while looks you are right. I
>> misunderstood
>> the code.
>
>> So only one idler would be tickled even there would be several idlers
>> in the
>> system. I thought we would tickle several idlers, which may introduce
>> some
>> contention between them.
>> 
>You can ask Xen to tickle more than one idle, by means of that
>parameter. But again, tickling idlers --either one or many-- would only
>happen if there's actual need for it.
>
>> BTW, any idea behind the cycle_cpu()? If this is about the locality,
>> how about
>> cycle within the node? and cycle within the node where v->processor
>> is?
>> 
>Cycle is there as a form of load spreading, i.e., we don't want to risk
>tickling always the same set of pcpus, or always the ones with the
>lower cpu IDs.
>
>You're right that taking locality into account could be a good thing in
>big systems. No, that is not done, not in 4.1 nor in upstream, and it
>may be something actually worth trying (although, again, it's probably
>unrelated to your issue).
>
>> > And again, I'd personally spend some more time --even by
>> > temporarily
>> > and hackishly instrumenting the code-- to understand better where
>> > the
>> > bottleneck is.
>> > 
>> 
>> Hmm... usually we use xentrace and lock profile to identify the
>> bottleneck,
>> other method you would like to suggest? and instrumenting the code
>> means to
>> add some log in code?
>> 
>xentrace and lock profile is all I use too, and there's not much else,
>without touching the code. And in fact, yes, with "instrumenting the
>code" I means temporary changing the code to display the information
>you need.
>
>In this specific case, rather than adding printks here and there
>(which, e.g., is what you usually do for debugging crashes or alike),
>I'd see about modifying a little bit either xentrace or lock profiling
>code (or both!) to make them provide the info you need.
>
>Sorry, but I don't think I can be much more specific without going
>looking at the code and actually trying to do that...
>
>> > E.g., something that has proven effective for others (and for which
>> > I've got an unfinished and never submitted patch to upstream), is
>> > keeping track of what pcpus actually have at least 1 vcpu in their
>> > runqueues (i.e., at least 1 vcpus that is runnable and not running)
>> > and, inside balance_load(), only consider those when looking for
>> > work
>> > to steal.
>> 
>> Looks like we keep a cpumask with those who have 1 more vcpus and
>> during
>> stealing process, just scan those instead of the online cpumask.
>> 
>Yep, that sounds like a plan, and was along the lines of what I was
>doing. If you want to give it a try, patches (for upstream, of course
>:-D) are welcome. :-P
>
>Regards,
>Dario
>-- 
><<This happens because I choose it to happen!>> (Raistlin Majere)
>-----------------------------------------------------------------
>Dario Faggioli, Ph.D, http://about.me/dario.faggioli
>Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)


-- 
Richard Yang\nHelp you, Help me

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel