Re: [PATCH 2/6] xen: credit: (micro) optimize csched_runq_steal().

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

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

On Fri, 2017-03-03 at 09:48 +0000, anshul makkar wrote:
> On 02/03/17 10:38, Dario Faggioli wrote:
> > --- a/xen/common/sched_credit.c
> > +++ b/xen/common/sched_credit.c
> > @@ -708,12 +708,10 @@ static inline int
> >   __csched_vcpu_is_migrateable(struct vcpu *vc, int dest_cpu,
> > cpumask_t *mask)
> >   {
> >       /*
> > -     * Don't pick up work that's in the peer's scheduling tail or
> > hot on
> > -     * peer PCPU. Only pick up work that prefers and/or is allowed
> > to run
> > -     * on our CPU.
> > +     * Don't pick up work that's or hot on peer PCPU, or that
> > can't (or
> Not clear.
>
Well, there's actually a typo (redundant 'or'). Good catch. :-)
> > 
> > +     * would prefer not to) run on cpu.
> >        */
> > -    return !vc->is_running &&
> > -           !__csched_vcpu_is_cache_hot(vc) &&
> > +    return !__csched_vcpu_is_cache_hot(vc) &&
> >              cpumask_test_cpu(dest_cpu, mask);
> !vc->is_running doesn't ease the complexity and doesn't save much on
> cpu 
> cycles. Infact, I think (!vc->is_running) makes the intention to
> check 
> for migration much more clear to understand.
> 
But the point is not saving the overhead of a !vc->is_running check
here, it is actually to pull it out from within this function and check
that before. And that's ok because the value won't change, and is good
thing because what we save is a call to

  __vcpu_has_soft_affinity()

and, potentially, to

  csched_balance_cpumask()

i.e., more specifically...
            *
> > @@ -1633,8 +1633,9 @@ csched_runq_steal(int peer_cpu, int cpu, int
> > pri, int balance_step)
> >            * vCPUs with useful soft affinities in some sort of
> > bitmap
> >            * or counter.
> >            */
> > -        if ( balance_step == CSCHED_BALANCE_SOFT_AFFINITY
> > -             && !__vcpu_has_soft_affinity(vc, vc-
> > >cpu_hard_affinity) )
> > +        if ( vc->is_running ||
> > +             (balance_step == CSCHED_BALANCE_SOFT_AFFINITY
> > +              && !__vcpu_has_soft_affinity(vc, vc-
> > >cpu_hard_affinity)) )
> >               continue;
> >   
> >           csched_balance_cpumask(vc, balance_step,
> > cpumask_scratch);
> > 
...these ones here.

I agree that the check was a good fit for that function, but --with the
updated comments-- I don't think it's too terrible to have it outside.

Or were you suggesting to have it in _both_ places? If that's the case,
no... I agree it's cheap, but that would look confusing to me (I
totally see myself, in 3 months, sending a patch to remove the
redundant is_running check! :-P).

Thanks and 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)

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

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

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