Re: [PATCH v2] sched: credit2: respect per-vcpu hard affinity

[Dario Faggioli <dario.faggioli@citrix.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3574 bytes --]

On Sun, 2015-03-08 at 21:11 -1000, Justin Weaver wrote:
> Thanks to all for the comments! I've implemented most of the changes
> recommended here in the v2 review. I should have a new patch set ready
> this week (with an updated soft affinity patch as well). 
>
Great! :-)

> > Oh, and this is what was causing you troubles, in case source and
> > destination runqueue were the same... Help me understand, which call to
> > sched_move_irqs() in schedule.c were we missing? I'd say it is the one
> > in vcpu_migrate(), but that does not seem to care about vc->processor
> > (much rater about new_cpu)... what am I missing?
> >
> > However, if they are not the same, the call to migrate() will override
> > this right away, won't it? What I mean to say is, if this is required
> > only in case trqd and svc->rqd are equal, can we tweak this part of
> > csched2_vcpu_migrate() as follows?
> >
> >     if ( trqd != svc->rqd )
> >         migrate(ops, svc, trqd, NOW());
> >     else
> >         vc->processor = new_cpu;
> 
> You are right; it does not have anything to do with sched_move_irqs()
> not being called (like you said it doesn't care about vc->processor).
>
Ah, ok. :-)

> You are never going to believe any of my explanations now! :) 
>
EhEh... If I'd do that to people who fail to understand how things works
at the first or second attempt, I would stop believing myself! :-D :-D

> Without the processor assignment
> here the vcpu might go on being assigned to a processor it no longer
> is allowed to run on. 
>
Ok.

> In that case, function runq_candidate may only
> get called for the vcpu's old processor, and runq_candidate will no
> longer let a vcpu run on a processor that it's not allowed to run on
> (because of the hard affinity check first introduced in v1 of this
> patch). 
>
It mostly makes sense. Out of the top of my head, it still looks like
there should be a pCPU that, when scheduling, would pick it up... I need
to think more about this...

> So in that condition the vcpu never gets to run. That's still
> somewhat of a vague explanation, but I have observed that that is what
> happens. 
>
Do you mean you _actually_ saw this, with some debugging printk-s, or
tracing, or something like this?

> Anyway I think everyone agrees that the processor assignment
> needs to be here, and I did move it to an else block for v3 like you
> recommended above.
>
Yes, that's the point, the assignement above is correct, IMO, so it
should be there, whether or not it is the cause of the issue :-)

> > I don't like the "_safe_" part, but that is not a big deal, I certainly
> > can live with it.
> 
> I changed it to _choose_valid_pcpu ... discuss! 
>
I'm fine with what Goerge proposes in his email.

> (Also, I can send out
> a pre-patch to change the double underscores in the whole file)
> 
For static symbols, yes. As Jan says, it's George's call. If you're up
for it, I think it would be an improvement.

> >> > VCPU2ONLINE(svc->vcpu) would make the line shorter.
> 
> I agree. VCPU2ONLINE is defined in schedule.c ... do you want me to
> move it to a common header along with the other parts we discussed
> (__vcpu_has_soft_affinity, etc.)? 
>
Either that or, if you only need it once, just open code it.

> Okay to move them to sched-if.h, or
> should I put them in a new header file?
> 
sched-if.h is ok for the step-wise load balancing macros, and it would
be the proper place where to move this too, if we go for moving it.

Thanks and Regards,
Dario

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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