[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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: signature.asc
Description: This is a digitally signed message part

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.