Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751872AbdGBMoB (ORCPT ); Sun, 2 Jul 2017 08:44:01 -0400 Received: from Galois.linutronix.de ([146.0.238.70]:39625 "EHLO Galois.linutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751611AbdGBMoA (ORCPT ); Sun, 2 Jul 2017 08:44:00 -0400 Date: Sun, 2 Jul 2017 14:43:56 +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 14/21] x86/intel_rdt/cqm: Add mon_data In-Reply-To: <1498503368-20173-15-git-send-email-vikas.shivappa@linux.intel.com> Message-ID: References: <1498503368-20173-1-git-send-email-vikas.shivappa@linux.intel.com> <1498503368-20173-15-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 Content-Length: 1957 Lines: 79 On Mon, 26 Jun 2017, Vikas Shivappa wrote: > Add a mon_data directory for the root rdtgroup and all other rdtgroups. > The directory holds all of the monitored data for all domains and events > of all resources being monitored. Again. This does two things at once. Move the existing code to a new file and add the monitoring stuff. Please split it apart. > +static bool __mon_event_count(u32 rmid, struct rmid_read *rr) > +{ > + u64 tval; > + > + tval = __rmid_read(rmid, rr->evtid); > + if (tval & (RMID_VAL_ERROR | RMID_VAL_UNAVAIL)) { > + rr->val = tval; > + return false; > + } > + switch (rr->evtid) { > + case QOS_L3_OCCUP_EVENT_ID: > + rr->val += tval; > + return true; > + default: > + return false; I have no idea what that return code means. > + } > +} > + > +void mon_event_count(void *info) Some explanation why this is a void pointer and how that function is called (I assume it's via IPI) would be appreciated. > +{ > + struct rdtgroup *rdtgrp, *entry; > + struct rmid_read *rr = info; > + struct list_head *llist; *head; > + > + rdtgrp = rr->rgrp; > + > + if (!__mon_event_count(rdtgrp->rmid, rr)) > + return; > + > + /* > + * For Ctrl groups read data from child monitor groups. > + */ > + llist = &rdtgrp->crdtgrp_list; > + > + if (rdtgrp->type == RDTCTRL_GROUP) { > + list_for_each_entry(entry, llist, crdtgrp_list) { > + if (!__mon_event_count(entry->rmid, rr)) > + return; > + } > + } > +} > +static int get_rdt_resourceid(struct rdt_resource *r) > +{ > + if (r > (rdt_resources_all + RDT_NUM_RESOURCES - 1) || > + r < rdt_resources_all || > + ((r - rdt_resources_all) % sizeof(struct rdt_resource))) > + return -EINVAL; If that ever happens, then you have other problems than a wrong pointer. > + > + return ((r - rdt_resources_all) / sizeof(struct rdt_resource)); Moo. Can't you simply put an index field into struct rdt_resource, intialize it with the resource ID and use that? Thanks, tglx