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

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@suse.com>

--- 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@lists.xen.org
http://lists.xen.org/xen-devel

On 11/11/2013 08:06, "Jan Beulich" <JBeulich@suse.com> 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@suse.com>
Acked-by: Keir Fraser <keir@xen.org>
> --- 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;
> 
> 
>