From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752217AbdGBKF5 (ORCPT ); Sun, 2 Jul 2017 06:05:57 -0400 Received: from Galois.linutronix.de ([146.0.238.70]:39425 "EHLO Galois.linutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751849AbdGBKF4 (ORCPT ); Sun, 2 Jul 2017 06:05:56 -0400 Date: Sun, 2 Jul 2017 12:05:40 +0200 (CEST) From: Thomas Gleixner To: Vikas Shivappa cc: x86@kernel.org, linux-kernel@vger.kernel.org, hpa@zytor.com, peterz@infradead.org, ravi.v.shankar@intel.com, vikas.shivappa@intel.com, tony.luck@intel.com, fenghua.yu@intel.com, andi.kleen@intel.com Subject: Re: [PATCH 08/21] x86/intel_rdt/cqm: Add RMID(Resource monitoring ID) management In-Reply-To: <1498503368-20173-9-git-send-email-vikas.shivappa@linux.intel.com> Message-ID: References: <1498503368-20173-1-git-send-email-vikas.shivappa@linux.intel.com> <1498503368-20173-9-git-send-email-vikas.shivappa@linux.intel.com> User-Agent: Alpine 2.20 (DEB 67 2015-01-07) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, 26 Jun 2017, Vikas Shivappa wrote: > +static u64 __rmid_read(u32 rmid, u32 eventid) > +{ > + u64 val; > + > + wrmsr(MSR_IA32_QM_EVTSEL, eventid, rmid); > + rdmsrl(MSR_IA32_QM_CTR, val); The calling convention of this function needs to be documented. It's obvious that it needs to be serialized .... > + > + /* > + * Aside from the ERROR and UNAVAIL bits, the return value is the > + * count for this @eventid tagged with @rmid. > + */ > + return val; > +} > + > +/* > + * Test whether an RMID is dirty(occupancy > threshold_occupancy) > + */ > +static void intel_cqm_stable(void *arg) > +{ > + struct rmid_entry *entry; > + u64 val; > + > + /* > + * Since we are in the IPI already lets mark all the RMIDs > + * that are dirty This comment is crap. It suggests: Let's do it while we are here anyway. But that's not true. The IPI is issued solely to figure out which RMIDs are dirty. > + */ > + list_for_each_entry(entry, &rmid_limbo_lru, list) { Since this is executed on multiple CPUs, that needs an explanation why that list is safe to iterate w/o explicit protection here. > + val = __rmid_read(entry->rmid, QOS_L3_OCCUP_EVENT_ID); > + if (val > intel_cqm_threshold) > + entry->state = RMID_DIRTY; > + } > +} > + > +/* > + * Scan the limbo list and move all entries that are below the > + * intel_cqm_threshold to the free list. > + * Return "true" if the limbo list is empty, "false" if there are > + * still some RMIDs there. > + */ > +static bool try_freeing_limbo_rmid(void) > +{ > + struct rmid_entry *entry, *tmp; > + struct rdt_resource *r; > + cpumask_var_t cpu_mask; > + struct rdt_domain *d; > + bool ret = true; > + > + if (list_empty(&rmid_limbo_lru)) > + return ret; > + > + if (!zalloc_cpumask_var(&cpu_mask, GFP_KERNEL)) > + return false; > + > + r = &rdt_resources_all[RDT_RESOURCE_L3]; > + > + list_for_each_entry(d, &r->domains, list) > + cpumask_set_cpu(cpumask_any(&d->cpu_mask), cpu_mask); > + > + /* > + * Test whether an RMID is free for each package. That wants a bit of explanation at some place why RMIDs have global scope. That's a pure implementation decision because from a hardware POV RMIDs have package scope. We could use the same RMID on different packages for different purposes. > + */ > + on_each_cpu_mask(cpu_mask, intel_cqm_stable, NULL, true); > + > + list_for_each_entry_safe(entry, tmp, &rmid_limbo_lru, list) { > + /* > + * Ignore the RMIDs that are marked dirty and reset the > + * state to check for being dirty again later. Ignore? -EMAKESNOSENSE > + */ > + if (entry->state == RMID_DIRTY) { > + entry->state = RMID_CHECK; > + ret = false; > + continue; > + } > + list_del(&entry->list); > + list_add_tail(&entry->list, &rmid_free_lru); > + } > + > + free_cpumask_var(cpu_mask); ... > +void free_rmid(u32 rmid) > +{ > + struct rmid_entry *entry; > + > + lockdep_assert_held(&rdtgroup_mutex); > + > + WARN_ON(!rmid); > + entry = __rmid_entry(rmid); > + > + entry->state = RMID_CHECK; > + > + if (rdt_mon_features & (1 << QOS_L3_OCCUP_EVENT_ID)) > + list_add_tail(&entry->list, &rmid_limbo_lru); > + else > + list_add_tail(&entry->list, &rmid_free_lru); Thinking a bit more about that limbo mechanics. In case that a RMID was never used on a particular package, the state check forces an IPI on all packages unconditionally. That's suboptimal at least. We know on which package a given RMID was used, so we could restrict the checks to exactly these packages, but I'm not sure it's worth the trouble. We might at least document that and explain why this is implemented in that way. Thanks, tglx