Re: [Xen-devel] [PATCH RFC V2 45/45] xen/sched: add scheduling granularity enum

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 06.05.19 at 08:56, <jgross@xxxxxxxx> wrote:
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -1701,6 +1701,8 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>          printk(XENLOG_INFO "Parked %u CPUs\n", num_parked);
>      smp_cpus_done();
>  
> +    scheduler_smp_init();
> +
>      do_initcalls();

This placement and the actual implementation of the function make
me wonder: Why didn't you make this an initcall, thus taking care of
Arm (at least in an abstract way) at the same time?

>  void scheduler_percpu_init(unsigned int cpu)
>  {
>      struct scheduler *sched = per_cpu(scheduler, cpu);
>      struct sched_resource *sd = per_cpu(sched_res, cpu);
> +    const cpumask_t *mask;
> +    unsigned int master_cpu;
> +    spinlock_t *lock;
> +    struct sched_item *old_item, *master_item;
> +
> +    if ( system_state == SYS_STATE_resume )
> +        return;
> +
> +    switch ( opt_sched_granularity )
> +    {
> +    case SCHED_GRAN_cpu:
> +        mask = cpumask_of(cpu);
> +        break;
> +    case SCHED_GRAN_core:
> +        mask = per_cpu(cpu_sibling_mask, cpu);
> +        break;
> +    case SCHED_GRAN_socket:
> +        mask = per_cpu(cpu_core_mask, cpu);
> +        break;
> +    default:
> +        ASSERT_UNREACHABLE();
> +        return;
> +    }
>  
> -    if ( system_state != SYS_STATE_resume )
> +    if ( cpu == 0 || cpumask_weight(mask) == 1 )

At least outside of x86 specific code I think we should avoid
introducing (further?) assumptions that seeing CPU 0 on a
CPU initialization path implies this being while booting the
system. I wonder anyway whether the right side of the ||
doesn't render the left side redundant.

> +static unsigned int __init sched_check_granularity(void)
> +{
> +    unsigned int cpu;
> +    unsigned int siblings, gran = 0;
> +
> +    for_each_online_cpu( cpu )

You want to decide for one of two possible styles, but not a mixture
of both:

    for_each_online_cpu ( cpu )

or

    for_each_online_cpu(cpu)

. Yet then I'm a little puzzled by its use here in the first place.
Generally I think for_each_cpu() uses in __init functions are
problematic, as they then require further code elsewhere to
deal with hot-onlining. A pre-SMP-initcall plus use of CPU
notifiers is typically more appropriate.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel