Re: [PATCH] x86/intel_rdt.c: Disable preempt for intel_rdt_sched_in() in move_myself()

[Thomas Gleixner]

On Wed, 30 Nov 2016, Fenghua Yu wrote:
> From: Fenghua Yu <fenghua.yu@xxxxxxxxx>

Subject: x86/intel_rdt.c: Disable preempt for intel_rdt_sched_in() in move_myself()

- The prefix is crap. git log arch/x86/kernel/cpu/intel_rdt.c should tell
  you what's the proper prefix is.

It's preemption not preempt and this wants to be a proper sentence:

     x86/intel_rdt: Call intel_rdt_sched_in() with preemption disabled

> intel_rdt_sched_in() calls this_cpu_ptr() read and write pqr_state and
> update PQR_ASSOC on current cpu. If execution of the function is preempted
> and switched to another CPU, the results are wrong. If 
> CONFIG_DEBUG_PREEMPT is turned on, the issue is reported as
> "BUG: smp_processor_id() running in preemptible code." when moving a task
> to a rdtgroup in move_myself().

When will you finally start to describe problems proper instead of
telling fairy tales about how you debugged the symptoms?

  intel_rdt_sched_in() must be called with preemption disabled because the
  function accesses percpu variables (pqr_state and closid).

  If a task moves itself via move_myself() preemption is enabled, which
  violates the calling convention and can result in incorrect closid
  selection when the task gets preempted or migrated. 

  Add the required protection and a comment about the calling convention.

That's concise and precise.

> Disable preempt before intel_rdt_sched_in() and eanble preempt after

s/preempt/preemtion/
s/eanble/enable/

Oh well.

> the function in move_myself() to solve the issue.

And next time you write: Add one before foo() and subtract one after the
function ....

Changelogs need to tell the WHY and not the WHAT. We can see the WHAT from
looking at the patch.

> Signed-off-by: Fenghua Yu <fenghua.yu@xxxxxxxxx>
> ---
>  arch/x86/kernel/cpu/intel_rdt_rdtgroup.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/arch/x86/include/asm/intel_rdt.h b/arch/x86/include/asm/intel_rdt.h
> index 6e90e87..eaaa765 100644
> --- a/arch/x86/include/asm/intel_rdt.h
> +++ b/arch/x86/include/asm/intel_rdt.h
> @@ -192,6 +192,9 @@ int rdtgroup_schemata_show(struct kernfs_open_file *of,
>   *   resctrl file system.
>   * - Caches the per cpu CLOSid values and does the MSR write only
>   *   when a task with a different CLOSid is scheduled in.
> + * - Caller needs to disable preempt before calling this function.

  s/preempt/preemtion/

  * - Must be called with preemption disabled.

> + *   The function doesn't check preemptible. Scheduler hot path
> + *   disables preempt already.

This part makes no sense whatsoever. -ENOPARSE

The important information is the calling convention, i.e.:

  * - Must be called with preemption disabled.

and that's enough.

>   */
>  static inline void intel_rdt_sched_in(void)
>  {
> diff --git a/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c b/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
> index fb8e03e..9c6f732 100644
> --- a/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
> +++ b/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
> @@ -326,8 +326,10 @@ static void move_myself(struct callback_head *head)
>  		kfree(rdtgrp);
>  	}
>  
> +	get_cpu();

Bah! What's wrong with preempt_disable()?

>  	/* update PQR_ASSOC MSR to make resource group go into effect */
>  	intel_rdt_sched_in();
> +	put_cpu();
>  
>  	kfree(callback);
>  }

Thanks,

	tglx