From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755062AbbETRTi (ORCPT ); Wed, 20 May 2015 13:19:38 -0400 Received: from mga09.intel.com ([134.134.136.24]:19800 "EHLO mga09.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755023AbbETRTg (ORCPT ); Wed, 20 May 2015 13:19:36 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.13,465,1427785200"; d="scan'208";a="729132356" Date: Wed, 20 May 2015 10:18:11 -0700 (PDT) From: Vikas Shivappa X-X-Sender: vikas@vshiva-Udesk To: Thomas Gleixner cc: Vikas Shivappa , 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: 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.10 (DEB 1266 2009-07-14) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, 15 May 2015, Thomas Gleixner wrote: > 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? 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 >