Re: [Xen-devel] [PATCH] xen: idle_loop: either deal with tasklets or go idle

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 14.06.17 at 18:53, <dario.faggioli@xxxxxxxxxx> wrote:
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -112,12 +112,18 @@ static void play_dead(void)
>  
>  static void idle_loop(void)
>  {
> +    unsigned int cpu = smp_processor_id();
> +
>      for ( ; ; )
>      {
> -        if ( cpu_is_offline(smp_processor_id()) )
> +        if ( cpu_is_offline(cpu) )
>              play_dead();
> -        (*pm_idle)();
> -        do_tasklet();
> +
> +        /* Are we here for running vcpu context tasklets, or for idling? */
> +        if ( unlikely(tasklet_work_to_do(cpu)) )

I'm not really sure about the "unlikely()" here.

> +            do_tasklet(cpu);
> +        else
> +            (*pm_idle)();

Please take the opportunity and drop the pointless parentheses
and indirection.

> --- a/xen/common/tasklet.c
> +++ b/xen/common/tasklet.c
> @@ -104,19 +104,11 @@ static void do_tasklet_work(unsigned int cpu, struct 
> list_head *list)
>  }
>  
>  /* VCPU context work */
> -void do_tasklet(void)
> +void do_tasklet(unsigned int cpu)
>  {
> -    unsigned int cpu = smp_processor_id();

I'm not convinced it is a good idea to have the caller pass in the CPU
number. In any event, if you do it this way, we will want an ASSERT()
to replace the initialization above.

>      unsigned long *work_to_do = &per_cpu(tasklet_work_to_do, cpu);
>      struct list_head *list = &per_cpu(tasklet_list, cpu);
>  
> -    /*
> -     * Work must be enqueued *and* scheduled. Otherwise there is no work to
> -     * do, and/or scheduler needs to run to update idle vcpu priority.
> -     */
> -    if ( likely(*work_to_do != (TASKLET_enqueued|TASKLET_scheduled)) )
> -        return;

Perhaps it also wouldn't hurt to convert this to an ASSERT() too.

> --- a/xen/include/xen/tasklet.h
> +++ b/xen/include/xen/tasklet.h
> @@ -40,9 +40,19 @@ DECLARE_PER_CPU(unsigned long, tasklet_work_to_do);
>  #define TASKLET_enqueued   (1ul << _TASKLET_enqueued)
>  #define TASKLET_scheduled  (1ul << _TASKLET_scheduled)
>  
> +static inline bool tasklet_work_to_do(unsigned int cpu)
> +{
> +    /*
> +     * Work must be enqueued *and* scheduled. Otherwise there is no work to
> +     * do, and/or scheduler needs to run to update idle vcpu priority.
> +     */
> +    return per_cpu(tasklet_work_to_do, cpu) == (TASKLET_enqueued|
> +                                                TASKLET_scheduled);
> +}

Wouldn't cpu_is_haltable() then also better use this new function?

Jan


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