From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S935206AbcJQOrq (ORCPT ); Mon, 17 Oct 2016 10:47:46 -0400 Received: from Galois.linutronix.de ([146.0.238.70]:37404 "EHLO Galois.linutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933390AbcJQOri (ORCPT ); Mon, 17 Oct 2016 10:47:38 -0400 Date: Mon, 17 Oct 2016 16:44:53 +0200 (CEST) From: Thomas Gleixner To: Fenghua Yu cc: "H. Peter Anvin" , Ingo Molnar , Tony Luck , Peter Zijlstra , Stephane Eranian , Borislav Petkov , Dave Hansen , Nilay Vaish , Shaohua Li , David Carrillo-Cisneros , Ravi V Shankar , Sai Prakhya , Vikas Shivappa , linux-kernel , x86 Subject: Re: [PATCH v4 10/18] x86/intel_rdt: Build structures for each resource based on cache topology In-Reply-To: <1476497548-11169-11-git-send-email-fenghua.yu@intel.com> Message-ID: References: <1476497548-11169-1-git-send-email-fenghua.yu@intel.com> <1476497548-11169-11-git-send-email-fenghua.yu@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 Fri, 14 Oct 2016, Fenghua Yu wrote: > > +static int get_cache_id(int cpu, int level) > +{ > + struct cpu_cacheinfo *ci = get_cpu_cacheinfo(cpu); > + int i; > + > + for (i = 0; i < ci->num_leaves; i++) > + if (ci->info_list[i].level == level) > + return ci->info_list[i].id; Multi line statements need curly braces. > + return -1; > +} > + > +void rdt_cbm_update(void *arg) > +{ > + struct msr_param *m = (struct msr_param *)arg; > + struct rdt_resource *r = m->res; > + struct rdt_domain *d; > + struct list_head *l; > + int i, cpu = smp_processor_id(); > + > + list_for_each(l, &r->domains) { > + d = list_entry(l, struct rdt_domain, list); We have list_for_each_entry() .... > + if (cpumask_test_cpu(cpu, &d->cpu_mask)) > + goto found; > + } > + pr_info_once("cpu %d not found in any domain for resource %s\n", > + cpu, r->name); If the list is empty, then 'd' is not initialized and dereferenced. If the list is not empty then you use the last domain in the list. Neither one is the right thing to do.... > + > +found: > + for (i = m->low; i < m->high; i++) > + wrmsrl(r->msr_base + i, d->cbm[i]); > +} > + > +static void update_domain(int cpu, struct rdt_resource *r, int add) > +{ > + struct list_head *l; > + struct rdt_domain *d; > + int i, cache_id; + struct rdt_domain *d; + struct list_head *l; + int i, cache_id; Can you see how this is simpler to read? > + > + cache_id = get_cache_id(cpu, r->cache_level); > + if (cache_id == -1) { > + pr_info_once("Could't find cache id for cpu %d\n", cpu); > + return; > + } Please seperate this by a empty line. > + list_for_each(l, &r->domains) { > + d = list_entry(l, struct rdt_domain, list); list_for_each_entry() once more. > + if (cache_id == d->id) > + goto found; > + if (cache_id < d->id) > + break; > + } > + if (!add) { > + pr_info_once("removed unknown cpu %d\n", cpu); _once? Why should this happen at all? > + return; > + } > + d = kzalloc(sizeof(*d), GFP_KERNEL); Shouldn't this be kzalloc_node() ? > + if (!d) > + return; > + > + d->id = cache_id; > + d->cbm = kmalloc_array(r->max_closid, sizeof(*d->cbm), GFP_KERNEL); > + if (!d->cbm) { > + pr_info("Failed to alloc CBM array for cpu %d\n", cpu); > + kfree(d); > + return; > + } > + cpumask_set_cpu(cpu, &d->cpu_mask); > + for (i = 0; i < r->max_closid; i++) { > + d->cbm[i] = r->max_cbm; > + wrmsrl(r->msr_base + i, d->cbm[i]); > + } > + list_add_tail(&d->list, l); > + r->num_domains++; > + return; > + > +found: > + if (add) { > + cpumask_set_cpu(cpu, &d->cpu_mask); Gah. Reusing this function for add and del is just a silly optimization which makes the code harder to read. All you share is the find thing. So you can split that out into a helper: static struct rdt_domain *rdt_find_domain(struct rdt_resource *r, unsigned int cpu, int id) { if (id < 0) return ERR_PTR(id); list_for_each_entry(d, &r->domains, list) { if (d->id == cache_id) return d; } return NULL; } and use it for seperate add/del functions. static void domain_add_cpu(unsigned int cpu, struct rdt_resource *r) { int id = get_cache_id(cpu, r->cache_level); struct rdt_domain *d; d = rdt_find_domain(r, cpu, id); if (IS_ERR(d)) { pr_warn("Print some useful information\n", r->cache_level, cpu, ....); return; } if (d) { cpumask_set_cpu(cpu, &d->cpu_mask); return; } /* Do the allocaction stuff */ } static void domain_remove_cpu(unsigned int cpu, struct rdt_resource *r) { int id = get_cache_id(cpu, r->cache_level); struct rdt_domain *d; d = rdt_find_domain(r, cpu, id); if (IS_ERR_OR_NULL(d)) { pr_warn("Print some useful information\n", r->cache_level, cpu, ...); return; } cpumask_clear_cpu(cpu, &d->cpu_mask); .... } Hmm? > static int __init intel_rdt_late_init(void) > { > struct rdt_resource *r; > + int state; > > if (!get_rdt_resources()) > return -ENODEV; > > + state = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, > + "AP_INTEL_RDT_ONLINE", Please use: "x86/rdt/cat:online:" or similar. We messed that up in a few places when adding the names, but we don't want to add more of this. > + intel_rdt_online_cpu, intel_rdt_offline_cpu); Thanks, tglx