From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933734AbcKJPVq (ORCPT ); Thu, 10 Nov 2016 10:21:46 -0500 Received: from Galois.linutronix.de ([146.0.238.70]:33437 "EHLO Galois.linutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933297AbcKJPVo (ORCPT ); Thu, 10 Nov 2016 10:21:44 -0500 Date: Thu, 10 Nov 2016 16:19:06 +0100 (CET) From: Thomas Gleixner To: David Carrillo-Cisneros cc: linux-kernel@vger.kernel.org, "x86@kernel.org" , Ingo Molnar , Andi Kleen , Kan Liang , Peter Zijlstra , Vegard Nossum , Marcelo Tosatti , Nilay Vaish , Borislav Petkov , Vikas Shivappa , Ravi V Shankar , Fenghua Yu , Paul Turner , Stephane Eranian Subject: Re: [PATCH v3 04/46] perf/x86/intel/cmt: add device initialization and CPU hotplug support In-Reply-To: <1477787923-61185-5-git-send-email-davidcc@google.com> Message-ID: References: <1477787923-61185-1-git-send-email-davidcc@google.com> <1477787923-61185-5-git-send-email-davidcc@google.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 Sat, 29 Oct 2016, David Carrillo-Cisneros wrote: > +static void free_pkg_data(struct pkg_data *pkg_data) > +{ > + kfree(pkg_data); > +} So this is called from __terminate_pkg_data() which itself is called from some random other place. Is this a code chasing game or is there a technical reason why functions which belong together are not grouped together? > +/* Init pkg_data for @cpu 's package. */ > +static struct pkg_data *alloc_pkg_data(int cpu) > +{ > + struct cpuinfo_x86 *c = &cpu_data(cpu); > + struct pkg_data *pkgd; > + int numa_node = cpu_to_node(cpu); > + u16 pkgid = topology_logical_package_id(cpu); Can you please sort the variables in reverse fir tree order? u16 pkgid = topology_logical_package_id(cpu); struct cpuinfo_x86 *c = &cpu_data(cpu); int numa_node = cpu_to_node(cpu); struct pkg_data *pkgd; That's way simpler to parse than this random ordering. > + > + if (c->x86_cache_occ_scale != cmt_l3_scale) { And why would c->x86_cache_occ_scale be intialized already when you really hotplug a CPU? It cannot be initialized because it is done from identify_cpu() when the cpu actually starts. You just never noticed because the driver initialized _AFTER_ all the cpus are brought up and you never bothered to limit the number of cpus which are brought up at boot time to a single node and then bring up the other node _after_ loading the driver. > + /* 0 scale must have been converted to 1 automatically. */ > + if (c->x86_cache_occ_scale || cmt_l3_scale != 1) { This check will just explode in your face when you do the above because c->x86_cache_occ_scale is 0. So IOW. This is broken and the wrong place to do this. > + pr_err("Multiple LLC scale values, disabling CMT support.\n"); Interesting. You disable CMT support. That's true for init(). In the real hotplug case you prevent bringing the cpu up, so the message is misleading because CMT for the already online cpus is already working and keeps so. > + return ERR_PTR(-ENXIO); > + } > + } > + > + pkgd = kzalloc_node(sizeof(*pkgd), GFP_KERNEL, numa_node); > + if (!pkgd) > + return ERR_PTR(-ENOMEM); > + > + pkgd->max_rmid = c->x86_cache_max_rmid; > + > + pkgd->work_cpu = cpu; This is wrong. This want's to be -1 or something invalid. We now can stop the hotplug process of a CPU at some random state. So if we stop right after this callback then this not yet online cpu is set as work cpu and if we then bring up another cpu in the package then it operates with a stale work cpu. Please make stuff symmetric. The pre online prep stage is just there to prepare data and pre initialize it. Anything which is related to operational state has to be done at the point where things become operational and undone at the same state when going down. > + pkgd->pkgid = pkgid; > + > + __min_max_rmid = min(__min_max_rmid, pkgd->max_rmid); What protects against the case where rmids are in use already and this cuts __min_max_rmid short during hotplug? > +static int init_pkg_data(int cpu) > +{ > + struct pkg_data *pkgd; > + u16 pkgid = topology_logical_package_id(cpu); > + > + lockdep_assert_held(&cmt_mutex); > + > + /* Verify that this pkgid isn't already initialized. */ > + if (WARN_ON_ONCE(cmt_pkgs_data[pkgid])) > + return -EPERM; For one this is a direct dereference of something which claims to be rcu protected. That's inconsistent. Further this check is completely pointless. This function is called from intel_cmt_prep_up() after detecting that there is no package data for this particular package id. I'm all for defensive programming, but this is just beyond silly. Aside of that why are you looking up pkgid in three functions in a row instead of simply handing it from one to the other? cmt_prep_up() -> init_pkg_data() -> alloc_pkg_data() > + pkgd = alloc_pkg_data(cpu); > + if (IS_ERR(pkgd)) > + return PTR_ERR(pkgd); > + > + rcu_assign_pointer(cmt_pkgs_data[pkgid], pkgd); > + synchronize_rcu(); And this synchronize_rcu() is required because of what? This is the first CPU of a package being brought up and you are holding the cmt_mutex. I might be missing something, but if so, then this is missing a comment. > +static int intel_cmt_prep_down(unsigned int cpu) > +{ > + struct pkg_data *pkgd; > + u16 pkgid = topology_logical_package_id(cpu); > + > + mutex_lock(&cmt_mutex); > + pkgd = rcu_dereference_protected(cmt_pkgs_data[pkgid], > + lockdep_is_held(&cmt_mutex)); > + if (pkgd->work_cpu >= nr_cpu_ids) { > + /* will destroy pkgd */ > + __terminate_pkg_data(pkgd); Oh right. You free data _BEFORE_ setting the pointer to NULL and synchronizing RCU. So anything which is merily using rcu_read_lock() can get a valid reference and fiddle with freed data. Well done. > + RCU_INIT_POINTER(cmt_pkgs_data[pkgid], NULL); > + synchronize_rcu(); > +static int __init cmt_start(void) > +{ > + char *str, scale[20]; > + int err; > + > + /* will be modified by init_pkg_data() in intel_cmt_prep_up(). */ > + __min_max_rmid = UINT_MAX; > + err = cpuhp_setup_state(CPUHP_PERF_X86_CMT_PREP, > + "PERF_X86_CMT_PREP", > + intel_cmt_prep_up, > + intel_cmt_prep_down); > + if (err) > + return err; > + > + err = cpuhp_setup_state(CPUHP_AP_PERF_X86_CMT_ONLINE, > + "AP_PERF_X86_CMT_ONLINE", > + intel_cmt_hp_online_enter, > + intel_cmt_hp_online_exit); > + if (err) > + goto rm_prep; > + > + snprintf(scale, sizeof(scale), "%u", cmt_l3_scale); > + str = kstrdup(scale, GFP_KERNEL); That string is duplicated for memory leak detector testing purposes or what? > + if (!str) { > + err = -ENOMEM; > + goto rm_online; > + } > + > + return 0; > + > +rm_online: > + cpuhp_remove_state(CPUHP_AP_PERF_X86_CMT_ONLINE); > +rm_prep: > + cpuhp_remove_state(CPUHP_PERF_X86_CMT_PREP); > + > + return err; > +} > + > +static int __init intel_cmt_init(void) > +{ > + struct pkg_data *pkgd = NULL; > + int err = 0; > + > + if (!x86_match_cpu(intel_cmt_match)) { > + err = -ENODEV; > + goto err_exit; This is crap. If a CPU does NOT support this then printing the registration failed error is just confusing. If it's not supported, return -ENODEV and be done with it. > + } > + > + err = cmt_alloc(); > + if (err) > + goto err_exit; > + > + err = cmt_start(); > + if (err) > + goto err_dealloc; > + > + pr_info("Intel CMT enabled with "); > + rcu_read_lock(); > + while ((pkgd = cmt_pkgs_data_next_rcu(pkgd))) { > + pr_cont("%d RMIDs for pkg %d, ", > + pkgd->max_rmid + 1, pkgd->pkgid); And this is useful because? Because it's so nice to have random stuff printed. The only valuable information is that this is enabled with the detected possible number of rmids (__min_max_rmid or whatever incomprehensible variable name you came up with). > + } > + rcu_read_unlock(); > + pr_cont("and l3 scale of %d KBs.\n", cmt_l3_scale); i.e this should be: pr_info("Intel CMT enabled. %d RMIDs, L3 scale %d KBs", ....); Am I missing something? If you really want to print out the per package rmdis, then the only reason to do so is when they atually differ and that can be done where you do that min() check. > + > +device_initcall(intel_cmt_init); Oh no. This wants to be a module from the very beginning. No point on forcing this as builtin for no reason. Thanks, tglx