Re: [Xen-devel] [PATCH v2 2/2] sched_credit2.c: runqueue_per_core code

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

On Wed, 2015-03-18 at 07:56 +0000, Jan Beulich wrote:
> >>> On 17.03.15 at 19:18, <dario.faggioli@xxxxxxxxxx> wrote:

> > --- a/xen/common/sched_credit2.c
> > +++ b/xen/common/sched_credit2.c
> > @@ -1936,12 +1936,8 @@ static void init_pcpu(const struct scheduler *ops, 
> > int cpu)
> >      }
> >  
> >      /* Figure out which runqueue to put it in */
> > -    rqi = 0;
> > -
> > -    /* Figure out which runqueue to put it in */
> > -    /* NB: cpu 0 doesn't get a STARTING callback, so we hard-code it to 
> > runqueue 0. */
> > -    if ( cpu == 0 )
> > -        rqi = 0;
> > +    if ( system_state == SYS_STATE_boot )
> 
> ... I'd suggest using <= SYS_STATE_boot or < SYS_STATE_smp_boot.
> 
Ok.

> > @@ -1986,9 +1982,13 @@ static void init_pcpu(const struct scheduler *ops, 
> > int cpu)
> >  static void *
> >  csched2_alloc_pdata(const struct scheduler *ops, int cpu)
> >  {
> > -    /* Check to see if the cpu is online yet */
> > -    /* Note: cpu 0 doesn't get a STARTING callback */
> > -    if ( cpu == 0 || cpu_to_socket(cpu) >= 0 )
> > +    /*
> > +     * Actual initialization is deferred to when the pCPU will be
> > +     * online, via a STARTING callback. The only exception is
> > +     * the boot cpu, which does not get such a notification, and
> > +     * hence needs to be taken care of here.
> > +     */
> > +    if ( system_state == SYS_STATE_boot )
> >          init_pcpu(ops, cpu);
> 
> Same here, plus the new condition at the first glance isn't matching
> the old one, but perhaps that's intentional.
> 
It is intentional, and that is why we're changing it! :-) Let me try to
explain:

The idea, in both old and new code, is to call init_pcpu() either:
 a) on the boot cpu, during boot
 b) on non-boot cpu, *only* if they are online.

The issue is that, for assessing b), it checks whether
cpu_to_socket(cpu) returns >=0 and, if it does, it assumes the cpu is
online. That is not true, as cpu_data.phys_proc_id (which is what
cpu_to_socket() go reading) is 0 even before any onlining and topolog
identification has happened (it's a global).

Therefore, all the pcpus are initialized right away, and all are
assigned to runqueue 0, as returned by cpu_to_socket() at this stage,
which is clearly wrong.

In fact, this is the reason why, previous versions of this took the
approach of initializing things such as cpu_to_socket() returned -1
before topology identification.

In the new version, as you suggested, I'm using system_state to figure
out whether we are dealing with the boot cpu, and that's it. :-)

Hope this clarifies things... If yes, I'll make sure to put a similar
explanation in the changelog, when sending the patch officially.

Regards,
Dario

Attachment: signature.asc
Description: This is a digitally signed message part

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