Re: [Xen-devel] [PATCH 1/3] mwait-idle: add support for using halt

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 25.02.19 at 21:23, <Brian.Woods@xxxxxxx> wrote:
> --- a/xen/arch/x86/cpu/mwait-idle.c
> +++ b/xen/arch/x86/cpu/mwait-idle.c
> @@ -103,6 +103,11 @@ static const struct cpuidle_state {
>  
>  #define CPUIDLE_FLAG_DISABLED                0x1
>  /*
> + * On certain AMD families that support mwait, only c1 can be reached by
> + * mwait and to reach c2, halt has to be used.
> + */
> +#define CPUIDLE_FLAG_USE_HALT                0x2

Could you point us at where in the manuals this behavior is described?
While PM Vol 2 has a chapter talking about P-states, I can't seem to
find any mention of C-states there.

> @@ -783,8 +788,23 @@ static void mwait_idle(void)
>  
>       update_last_cx_stat(power, cx, before);
>  
> -     if (cpu_is_haltable(cpu))
> -             mwait_idle_with_hints(eax, MWAIT_ECX_INTERRUPT_BREAK);
> +     if (cpu_is_haltable(cpu)) {
> +             struct cpu_info *info;
> +             switch (cx->entry_method) {

Blank line between declaration(s) and statement(s) please. And
it would seem better to move the declaration right here (inside
the switch()) anyway. Then again ...

> +             case ACPI_CSTATE_EM_FFH:
> +                     mwait_idle_with_hints(eax, MWAIT_ECX_INTERRUPT_BREAK);
> +                     break;
> +             case ACPI_CSTATE_EM_HALT:
> +                     info = get_cpu_info();
> +                     spec_ctrl_enter_idle(info);
> +                     safe_halt();
> +                     spec_ctrl_exit_idle(info);

... wouldn't it be better to avoid the redundancy with default_idle(),
by introducing a new helper function, e.g. spec_ctrl_safe_halt()?

> +                     local_irq_disable();
> +                     break;
> +             default:
> +                     printk(XENLOG_ERR PREFIX "unknown entry method %d\n", 
> cx->entry_method);
> +             }

Overly long line and missing break statement.

> @@ -1184,8 +1204,9 @@ static int mwait_idle_cpu_init(struct notifier_block 
> *nfb,
>       for (cstate = 0; cpuidle_state_table[cstate].target_residency; 
> ++cstate) {
>               unsigned int num_substates, hint, state;
>               struct acpi_processor_cx *cx;
> +             const unsigned int flags = cpuidle_state_table[cstate].flags;

May I suggest to name the variable slightly differently, e.g. cflags,
to avoid any risk of it being mistaken for what we commonly use
with e.g. spin_lock_irqsave()?

> @@ -1221,7 +1242,12 @@ static int mwait_idle_cpu_init(struct notifier_block 
> *nfb,
>               cx = dev->states + dev->count;
>               cx->type = state;
>               cx->address = hint;
> -             cx->entry_method = ACPI_CSTATE_EM_FFH;
> +
> +             if (flags & CPUIDLE_FLAG_USE_HALT)
> +                     cx->entry_method = ACPI_CSTATE_EM_HALT;
> +              else
> +                     cx->entry_method = ACPI_CSTATE_EM_FFH;

I'd prefer if you used a conditional expression here. One of the goals for
any changes to this file should be to limit the delta to its Linux original, in
order to increase the chances of patches coming from there to apply
reasonably cleanly here.

Doing so would also save me from complaining about the stray blank
ahead of "else".

Jan



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