Re: [PATCH 4/7] x86/intel_rdt: Implement scheduling support for Intel RDT

[Vikas Shivappa <vikas.shivappa@intel.com>]

On Sat, 2 May 2015, Peter Zijlstra wrote:

> On Fri, May 01, 2015 at 06:36:38PM -0700, Vikas Shivappa wrote:
>> Adds support for IA32_PQR_ASSOC MSR writes during task scheduling.
>>
>> The high 32 bits in the per processor MSR IA32_PQR_ASSOC represents the
>> CLOSid. During context switch kernel implements this by writing the
>> CLOSid of the cgroup to which the task belongs to the CPU's
>> IA32_PQR_ASSOC MSR.
>>
>> For Cache Allocation, this would let the task fill in the cache 'subset'
>> represented by the cgroup's Cache bit mask(CBM).
>>
>
> Are you guys for real? Have you even looked at the trainwreck this
> makes?
>
>> +static inline bool rdt_enabled(void)
>> +{
>> +	return static_key_false(&rdt_enable_key);
>> +}
>
>> +/*
>> + * rdt_sched_in() - Writes the task's CLOSid to IA32_PQR_MSR
>> + * if the current Closid is different than the new one.
>> + */
>> +static inline void rdt_sched_in(struct task_struct *task)
>> +{
>> +	struct intel_rdt *ir;
>> +	unsigned int clos;
>> +
>> +	if (!rdt_enabled())
>> +		return;
>> +
>> +	/*
>> +	 * This needs to be fixed
>> +	 * to cache the whole PQR instead of just CLOSid.
>> +	 * PQR has closid in high 32 bits and CQM-RMID in low 10 bits.
>> +	 * Should not write a 0 to the low 10 bits of PQR
>> +	 * and corrupt RMID.
>> +	 */
>> +	clos = this_cpu_read(x86_cpu_clos);
>> +
>> +	rcu_read_lock();
>> +	ir = task_rdt(task);
>> +	if (ir->clos == clos) {
>> +		rcu_read_unlock();
>> +		return;
>> +	}
>> +
>> +	wrmsr(MSR_IA32_PQR_ASSOC, 0, ir->clos);
>> +	this_cpu_write(x86_cpu_clos, ir->clos);
>> +	rcu_read_unlock();
>> +}
>
> You inject _ALL_ that into the scheduler hot path. Insane much?

At some point I had a #ifdef for the rdt_sched_in , will fix this.

>
>> +
>> +#else
>> +
>> +static inline void rdt_sched_in(struct task_struct *task) {}
>> +
>>  #endif
>>  #endif
>> diff --git a/arch/x86/include/asm/switch_to.h b/arch/x86/include/asm/switch_to.h
>> index 751bf4b..82ef4b3 100644
>> --- a/arch/x86/include/asm/switch_to.h
>> +++ b/arch/x86/include/asm/switch_to.h
>> @@ -8,6 +8,9 @@ struct tss_struct;
>>  void __switch_to_xtra(struct task_struct *prev_p, struct task_struct *next_p,
>>  		      struct tss_struct *tss);
>>
>> +#include <asm/intel_rdt.h>
>> +#define post_arch_switch(current)	rdt_sched_in(current)
>> +
>>  #ifdef CONFIG_X86_32
>>
>>  #ifdef CONFIG_CC_STACKPROTECTOR
>
>> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
>> index f9123a8..cacb490 100644
>> --- a/kernel/sched/core.c
>> +++ b/kernel/sched/core.c
>> @@ -2241,6 +2241,7 @@ static struct rq *finish_task_switch(struct task_struct *prev)
>>  	prev_state = prev->state;
>>  	vtime_task_switch(prev);
>>  	finish_arch_switch(prev);
>> +	post_arch_switch(current);
>>  	perf_event_task_sched_in(prev, current);
>>  	finish_lock_switch(rq, prev);
>>  	finish_arch_post_lock_switch();
>
> Not a word in the Changelog on this hook; that's double fail.

will add the changelog. we want the current task which no other existing hook 
provides.

Thanks,
Vikas

>
> Please _THINK_ before writing code.
>