Re: [PATCH 3/4] xen: sched: improve checking soft-affinity

[Dario Faggioli <raistlin@linux.it>]

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

On Wed, 2017-10-04 at 14:23 +0100, George Dunlap wrote:
> On 09/15/2017 06:35 PM, Dario Faggioli wrote:
> > 
> > diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c
> > index 3efbfc8..35d0c98 100644
> > --- a/xen/common/sched_credit.c
> > +++ b/xen/common/sched_credit.c
> > @@ -410,8 +410,7 @@ static inline void __runq_tickle(struct
> > csched_vcpu *new)
> >              int new_idlers_empty;
> >  
> >              if ( balance_step == BALANCE_SOFT_AFFINITY
> > -                 && !has_soft_affinity(new->vcpu,
> > -                                       new->vcpu-
> > >cpu_hard_affinity) )
> > +                 && !has_soft_affinity(new->vcpu) )
> >                  continue;
> >  
> >              /* Are there idlers suitable for new (for this balance
> > step)? */
> > @@ -743,50 +742,42 @@ __csched_vcpu_is_migrateable(struct vcpu *vc,
> > int dest_cpu, cpumask_t *mask)
> >  static int
> >  _csched_cpu_pick(const struct scheduler *ops, struct vcpu *vc,
> > bool_t commit)
> >  {
> > -    cpumask_t cpus;
> 
> Is there a reason you couldn't use cpumask_t
> *cpus=cpumask_scratch_cpu()?
> 
I was about to say "yes, of course". But while double checking that, I
realized that the patch is actually wrong.

So, even if not 100% intentional... good catch! :-P

In fact, in this function (_csched_cpu_pick()), cpu potentially changes
here:

    cpu = cpumask_test_cpu(vc->processor, cpumask_scratch_cpu(cpu))
            ? vc->processor
            : cpumask_cycle(vc->processor, cpumask_scratch_cpu(cpu));

and here:

    if ( !cpumask_test_cpu(cpu, cpumask_scratch_cpu(cpu)) &&
         !cpumask_empty(cpumask_scratch_cpu(cpu)) )
        cpu = cpumask_cycle(cpu, cpumask_scratch_cpu(cpu));

And it's therefore not ok to continue to use cpumask_scratch_cpu(cpu).

I now have fixed this, but then, while testing, I discovered more, and
much more serious issues.

I'm not actually sure what happened, but it's quite clear I've made a
mess while testing this series (likely, I must mistakenly have tested a
different version of the series, wrt to that I sent).

So I'll send a v3, with the issues I've found fixed, and this time
properly tested.

Sorry everyone, George in particular. :-(

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: 833 bytes --]

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

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