Re: [PATCH 5/7] x86/intel_rdt: Software Cache for IA32_PQR_MSR

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

On Fri, 15 May 2015, Thomas Gleixner wrote:

> On Mon, 11 May 2015, Vikas Shivappa wrote:
>
>> Signed-off-by: Vikas Shivappa <vikas.shivappa@linux.intel.com>
>>
>> Conflicts:
>> 	arch/x86/kernel/cpu/perf_event_intel_cqm.c
>
> And that's interesting for what?

Will remove this, fixed some conflicts as this code changes both cqm and cache 
allocation.

>
>> --- /dev/null
>> +++ b/arch/x86/include/asm/rdt_common.h
>
>> @@ -0,0 +1,13 @@
>> +#ifndef _X86_RDT_H_
>> +#define _X86_RDT_H_
>> +
>> +#define MSR_IA32_PQR_ASSOC	0x0c8f
>> +
>> +struct intel_pqr_state {
>> +	raw_spinlock_t	lock;
>> +	int		rmid;
>> +	int		clos;
>
> Those want to be u32. We have no type checkes there, but u32 is the
> proper data type for wrmsr.

will fix.

>
>> +	int		cnt;
>> +};
>> +
>
> What's wrong with having this in intel_rdt.h?

intel_rdt.h is specific for only cache allocation and rdt features in the 
cgroup. well, thats a criteria  to seperate them , isnt it ? cqm wont 
use most of the things (and growing) in intel_rdt.h.

It seems you're a big
> fan of sprinkling stuff all over the place so reading becomes a
> chasing game.
>
>>  {
>>  	struct task_struct *task = current;
>>  	struct intel_rdt *ir;
>> +	struct intel_pqr_state *state = this_cpu_ptr(&pqr_state);
>> +	unsigned long flags;
>>
>> +	raw_spin_lock_irqsave(&state->lock, flags);
>
> finish_arch_switch() is called with interrupts disabled already ...

Ok , I somehow could not locate this cli when I thought about it. So if this is 
interrupt disabled and no preempt ,
then we dont need both the spin lock and rcu 
lock (since rcu lock sync would obviously wait for the preempt disable..).

will remove both of them.

>
>>  	rcu_read_lock();
>
> So we now have a spin_lock() and rcu_read_lock() and no explanation
> what is protecting what.
>
>>  	ir = task_rdt(task);
>> -	if (ir->clos == clos) {
>> +	if (ir->clos == state->clos) {
>
> And of course taking the spin lock for checking state->clos is
> complete and utter nonsense.
>
> state->clos can only be changed by this code and the only reason why
> we need the lock is to protect against concurrent modification of
> state->rmid.
>
> So the check for ir->clos == state->clos can be done lockless.
>
> And I seriously doubt, that state->lock is necessary at all.
>
> Q: What is it protecting?
> A: state->clos, state->rmid, state->cnt
>
> Q: What's the context?
> A: Per cpu context. The per cpu pqr state is NEVER modified from a
>   remote cpu.
>
> Q: What is the lock for?
> A: Nothing.
>
> Q: Why?
> A: Because interrupt disabled regions protect per cpu state perfectly
>   fine and there is is no memory ordering issue which would require a
>   lock or barrier either.
>
> Peter explained it to you several times already that context switch is
> one the most sensitive hot pathes where we care about every cycle. But
> you just go ahead and mindlessly create pointless overhead.
>
>> +	/*
>> +	 * PQR has closid in high 32 bits and CQM-RMID
>> +	 * in low 10 bits. Rewrite the exsting rmid from
>> +	 * software cache.
>
> This comment wouldnt be necessary if you would have proper documented
> struct pqr_state.

Will fix, that would be lot better.

>
>> +	 */
>> +	wrmsr(MSR_IA32_PQR_ASSOC, state->rmid, ir->clos);
>> +	state->clos = ir->clos;
>>  	rcu_read_unlock();
>> +	raw_spin_unlock_irqrestore(&state->lock, flags);
>> +
>>  }
>
>> -static DEFINE_PER_CPU(struct intel_cqm_state, cqm_state);
>> +DEFINE_PER_CPU(struct intel_pqr_state, pqr_state);
>
> With CONFIG_PERF=n and CONFIG_CGROUP_RDT=y the linker will fail.

copy from Makefile below -
obj-$(CONFIG_CPU_SUP_INTEL)             += perf_event_intel_rapl.o 
perf_event_intel_cqm.o

should work with CONFIG_PERF_EVENTS=n and CGROUP_RDT=y ?

Thanks,
Vikas

>
> Thanks,
>
> 	tglx
>