Re: [Xen-devel] [PATCH] x86/idle: reduce contention on ACPI register accesses

[Keir Fraser <keir.xen@xxxxxxxxx>]

On 11/11/2013 08:06, "Jan Beulich" <JBeulich@xxxxxxxx> wrote:

> Other than when they're located in I/O port space, accessing them when
> in MMIO space (currently) implies usage of some sort of global lock: In
> -unstable this would be due to the use of vmap(), is older trees the
> necessary locking was introduced by 2ee9cbf9 ("ACPI: fix
> acpi_os_map_memory()"). This contention was observed to result in Dom0
> kernel soft lockups during the loading of the ACPI processor driver
> there on systems with very many CPU cores.
> 
> There are a couple of things being done for this:
> - re-order elements of an if() condition so that the register access
>   only happens when we really need it
> - turn off arbitration disabling only when the first CPU leaves C3
>   (paralleling how arbitration disabling gets turned on)
> - only set the (global) bus master reload flag once (when the first
>   target CPU gets processed)
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

Acked-by: Keir Fraser <keir@xxxxxxx>

> --- a/xen/arch/x86/acpi/cpu_idle.c
> +++ b/xen/arch/x86/acpi/cpu_idle.c
> @@ -439,8 +439,8 @@ static void acpi_processor_idle(void)
>           (next_state = cpuidle_current_governor->select(power)) > 0 )
>      {
>          cx = &power->states[next_state];
> -        if ( power->flags.bm_check && acpi_idle_bm_check()
> -             && cx->type == ACPI_STATE_C3 )
> +        if ( cx->type == ACPI_STATE_C3 && power->flags.bm_check &&
> +             acpi_idle_bm_check() )
>              cx = power->safe_state;
>          if ( cx->idx > max_cstate )
>              cx = &power->states[max_cstate];
> @@ -563,8 +563,8 @@ static void acpi_processor_idle(void)
>          {
>              /* Enable bus master arbitration */
>              spin_lock(&c3_cpu_status.lock);
> -            acpi_set_register(ACPI_BITREG_ARB_DISABLE, 0);
> -            c3_cpu_status.count--;
> +            if ( c3_cpu_status.count-- == num_online_cpus() )
> +                acpi_set_register(ACPI_BITREG_ARB_DISABLE, 0);
>              spin_unlock(&c3_cpu_status.lock);
>          }
>  
> @@ -821,12 +821,10 @@ static int check_cx(struct acpi_processo
>              return -EINVAL;
>  
>          /* All the logic here assumes flags.bm_check is same across all CPUs
> */
> -        if ( bm_check_flag == -1 )
> +        if ( bm_check_flag < 0 )
>          {
>              /* Determine whether bm_check is needed based on CPU  */
>              acpi_processor_power_init_bm_check(&(power->flags));
> -            bm_check_flag = power->flags.bm_check;
> -            bm_control_flag = power->flags.bm_control;
>          }
>          else
>          {
> @@ -853,14 +851,13 @@ static int check_cx(struct acpi_processo
>                  }
>              }
>              /*
> -             * On older chipsets, BM_RLD needs to be set
> -             * in order for Bus Master activity to wake the
> -             * system from C3.  Newer chipsets handle DMA
> -             * during C3 automatically and BM_RLD is a NOP.
> -             * In either case, the proper way to
> -             * handle BM_RLD is to set it and leave it set.
> +             * On older chipsets, BM_RLD needs to be set in order for Bus
> +             * Master activity to wake the system from C3, hence
> +             * acpi_set_register() is always being called once below.  Newer
> +             * chipsets handle DMA during C3 automatically and BM_RLD is a
> +             * NOP.  In either case, the proper way to handle BM_RLD is to
> +             * set it and leave it set.
>               */
> -            acpi_set_register(ACPI_BITREG_BUS_MASTER_RLD, 1);
>          }
>          else
>          {
> @@ -875,7 +872,13 @@ static int check_cx(struct acpi_processo
>                            " for C3 to be enabled on SMP systems\n"));
>                  return -EINVAL;
>              }
> -            acpi_set_register(ACPI_BITREG_BUS_MASTER_RLD, 0);
> +        }
> +
> +        if ( bm_check_flag < 0 )
> +        {
> +            bm_check_flag = power->flags.bm_check;
> +            bm_control_flag = power->flags.bm_control;
> +            acpi_set_register(ACPI_BITREG_BUS_MASTER_RLD, bm_check_flag);
>          }
>  
>          break;
> 
> 
> 



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