From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934804AbbEOUPW (ORCPT ); Fri, 15 May 2015 16:15:22 -0400 Received: from www.linutronix.de ([62.245.132.108]:42941 "EHLO Galois.linutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933913AbbEOUPQ (ORCPT ); Fri, 15 May 2015 16:15:16 -0400 Date: Fri, 15 May 2015 22:15:25 +0200 (CEST) From: Thomas Gleixner To: Vikas Shivappa cc: vikas.shivappa@intel.com, x86@kernel.org, linux-kernel@vger.kernel.org, hpa@zytor.com, mingo@kernel.org, tj@kernel.org, peterz@infradead.org, matt.fleming@intel.com, will.auld@intel.com, peter.zijlstra@intel.com, h.peter.anvin@intel.com, kanaka.d.juvva@intel.com, mtosatti@redhat.com Subject: Re: [PATCH 5/7] x86/intel_rdt: Software Cache for IA32_PQR_MSR In-Reply-To: <1431370976-31115-6-git-send-email-vikas.shivappa@linux.intel.com> Message-ID: References: <1431370976-31115-1-git-send-email-vikas.shivappa@linux.intel.com> <1431370976-31115-6-git-send-email-vikas.shivappa@linux.intel.com> User-Agent: Alpine 2.11 (DEB 23 2013-08-11) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII X-Linutronix-Spam-Score: -1.0 X-Linutronix-Spam-Level: - X-Linutronix-Spam-Status: No , -1.0 points, 5.0 required, ALL_TRUSTED=-1,SHORTCIRCUIT=-0.0001,URIBL_BLOCKED=0.001 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, 11 May 2015, Vikas Shivappa wrote: > Signed-off-by: Vikas Shivappa > > Conflicts: > arch/x86/kernel/cpu/perf_event_intel_cqm.c And that's interesting for what? > --- /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. > + int cnt; > +}; > + What's wrong with having this 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 ... > 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. > + */ > + 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. Thanks, tglx