Re: [Xen-devel] [PATCH 02/16] xen: sched: add .init_pdata hook to the scheduler interface

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 18/03/16 19:04, Dario Faggioli wrote:
> with the purpose of decoupling the allocation phase and
> the initialization one, for per-pCPU data of the schedulers.
> 
> This makes it possible to perform the initialization later
> in the pCPU bringup/assignement process, when more information
> (for instance, the host CPU topology) are available. This,
> for now, is important only for Credit2, but it can well be
> useful to other schedulers.
> 
> Signed-off-by: Dario Faggioli <dario.faggioli@xxxxxxxxxx>

Reviewed-by: George Dunlap <george.dunlap@xxxxxxxxxx>

Just one note -- I found this patch harder to review than necessary, I
think, because it implemented the callback but nobody was using it.  I
had to keep switching back and forth between the patches to find out
what was going on.  I personally would have folded patches 2 and 4 together.

(Just to be clear, no action necessary.)

> ---
> Cc: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
> Cc: Juergen Gross <jgross@xxxxxxxx>
> ---
> Changes from v1:
>  * in schedule_cpu_switch(), call to init_pdata() moved up,
>    close to the call to alloc_pdata() (for consistency with
>    other call sites) and prototype slightly changed.
> ---
> During v1 review, it was agreed to add ASSERTS() and comments
> to clarify the use of schedule_cpu_switch(). This can't be
> found here, but only because it has happened in another patch.
> ---
>  xen/common/schedule.c      |    7 +++++++
>  xen/include/xen/sched-if.h |    1 +
>  2 files changed, 8 insertions(+)
> 
> diff --git a/xen/common/schedule.c b/xen/common/schedule.c
> index e57b659..0627eb5 100644
> --- a/xen/common/schedule.c
> +++ b/xen/common/schedule.c
> @@ -1517,10 +1517,15 @@ static int cpu_schedule_callback(
>      struct notifier_block *nfb, unsigned long action, void *hcpu)
>  {
>      unsigned int cpu = (unsigned long)hcpu;
> +    struct scheduler *sched = per_cpu(scheduler, cpu);
> +    struct schedule_data *sd = &per_cpu(schedule_data, cpu);
>      int rc = 0;
>  
>      switch ( action )
>      {
> +    case CPU_STARTING:
> +        SCHED_OP(sched, init_pdata, sd->sched_priv, cpu);
> +        break;
>      case CPU_UP_PREPARE:
>          rc = cpu_schedule_up(cpu);
>          break;
> @@ -1597,6 +1602,7 @@ void __init scheduler_init(void)
>      if ( ops.alloc_pdata &&
>           !(this_cpu(schedule_data).sched_priv = ops.alloc_pdata(&ops, 0)) )
>          BUG();
> +    SCHED_OP(&ops, init_pdata, this_cpu(schedule_data).sched_priv, 0);
>  }
>  
>  /*
> @@ -1640,6 +1646,7 @@ int schedule_cpu_switch(unsigned int cpu, struct 
> cpupool *c)
>      ppriv = SCHED_OP(new_ops, alloc_pdata, cpu);
>      if ( ppriv == NULL )
>          return -ENOMEM;
> +    SCHED_OP(new_ops, init_pdata, ppriv, cpu);
>      vpriv = SCHED_OP(new_ops, alloc_vdata, idle, idle->domain->sched_priv);
>      if ( vpriv == NULL )
>      {
> diff --git a/xen/include/xen/sched-if.h b/xen/include/xen/sched-if.h
> index 825f1ad..70c08c6 100644
> --- a/xen/include/xen/sched-if.h
> +++ b/xen/include/xen/sched-if.h
> @@ -133,6 +133,7 @@ struct scheduler {
>                                      void *);
>      void         (*free_pdata)     (const struct scheduler *, void *, int);
>      void *       (*alloc_pdata)    (const struct scheduler *, int);
> +    void         (*init_pdata)     (const struct scheduler *, void *, int);
>      void         (*free_domdata)   (const struct scheduler *, void *);
>      void *       (*alloc_domdata)  (const struct scheduler *, struct domain 
> *);
>  
> 


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