Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752216AbdGBK7C (ORCPT ); Sun, 2 Jul 2017 06:59:02 -0400 Received: from Galois.linutronix.de ([146.0.238.70]:39474 "EHLO Galois.linutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751810AbdGBK7A (ORCPT ); Sun, 2 Jul 2017 06:59:00 -0400 Date: Sun, 2 Jul 2017 12:58:43 +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 11/21] x86/intel_rdt/cqm: Add mkdir support for RDT monitoring In-Reply-To: <1498503368-20173-12-git-send-email-vikas.shivappa@linux.intel.com> Message-ID: References: <1498503368-20173-1-git-send-email-vikas.shivappa@linux.intel.com> <1498503368-20173-12-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: 4750 Lines: 174 On Mon, 26 Jun 2017, Vikas Shivappa wrote: > +/* > + * Common code for ctrl_mon and monitor group mkdir. > + * The caller needs to unlock the global mutex upon success. > + */ > +static int mkdir_rdt_common(struct kernfs_node *pkn, struct kernfs_node *prkn, pkn and prkn are horrible to distinguish. What's wrong with keeping *parent_kn and have *kn as the new thing? > + const char *name, umode_t mode, > + enum rdt_group_type rtype, struct rdtgroup **r) > { Can you please split out that mkdir_rdt_common() change into a separate patch? It can be done as a preparatory stand alone change just for the existing rdt group code. Then the monitoring add ons come on top of it. > - struct rdtgroup *parent, *rdtgrp; > + struct rdtgroup *prgrp, *rdtgrp; > struct kernfs_node *kn; > - int ret, closid; > - > - /* Only allow mkdir in the root directory */ > - if (parent_kn != rdtgroup_default.kn) > - return -EPERM; > - > - /* Do not accept '\n' to avoid unparsable situation. */ > - if (strchr(name, '\n')) > - return -EINVAL; > + uint fshift = 0; > + int ret; > > - parent = rdtgroup_kn_lock_live(parent_kn); > - if (!parent) { > + prgrp = rdtgroup_kn_lock_live(prkn); > + if (!prgrp) { > ret = -ENODEV; > goto out_unlock; > } > > - ret = closid_alloc(); > - if (ret < 0) > - goto out_unlock; > - closid = ret; > - > /* allocate the rdtgroup. */ > rdtgrp = kzalloc(sizeof(*rdtgrp), GFP_KERNEL); > if (!rdtgrp) { > ret = -ENOSPC; > - goto out_closid_free; > + goto out_unlock; > } > - rdtgrp->closid = closid; > - list_add(&rdtgrp->rdtgroup_list, &rdt_all_groups); > + *r = rdtgrp; > + rdtgrp->parent = prgrp; > + rdtgrp->type = rtype; > + INIT_LIST_HEAD(&rdtgrp->crdtgrp_list); > > /* kernfs creates the directory for rdtgrp */ > - kn = kernfs_create_dir(parent->kn, name, mode, rdtgrp); > + kn = kernfs_create_dir(pkn, name, mode, rdtgrp); > if (IS_ERR(kn)) { > ret = PTR_ERR(kn); > goto out_cancel_ref; > @@ -1138,27 +1166,138 @@ static int rdtgroup_mkdir(struct kernfs_node *parent_kn, const char *name, > if (ret) > goto out_destroy; > > - ret = rdtgroup_add_files(kn, RF_CTRL_BASE); > + fshift = 1 << (RF_CTRLSHIFT + rtype); > + ret = rdtgroup_add_files(kn, RFTYPE_BASE | fshift); I'd rather make this: files = RFTYPE_BASE | (1U << (RF_CTRLSHIFT + rtype)); ret = rdtgroup_add_files(kn, files); > if (ret) > goto out_destroy; > > + if (rdt_mon_features) { > + ret = alloc_rmid(); > + if (ret < 0) > + return ret; > + > + rdtgrp->rmid = ret; > + } > kernfs_activate(kn); > > - ret = 0; > - goto out_unlock; What unlocks prkn now? The caller, right? Please add a comment ... > + return 0; > > out_destroy: > kernfs_remove(rdtgrp->kn); > out_cancel_ref: > - list_del(&rdtgrp->rdtgroup_list); > kfree(rdtgrp); > -out_closid_free: > +out_unlock: > + rdtgroup_kn_unlock(prkn); > + return ret; > +} > + > +static void mkdir_rdt_common_clean(struct rdtgroup *rgrp) > +{ > + kernfs_remove(rgrp->kn); > + if (rgrp->rmid) > + free_rmid(rgrp->rmid); Please put that conditonal into free_rmid(). > + kfree(rgrp); > +} > +static int rdtgroup_mkdir(struct kernfs_node *pkn, const char *name, > + umode_t mode) > +{ > + /* Do not accept '\n' to avoid unparsable situation. */ > + if (strchr(name, '\n')) > + return -EINVAL; > + > + /* > + * We don't allow rdtgroup ctrl_mon directories to be created anywhere > + * except the root directory and dont allow rdtgroup monitor > + * directories to be created anywhere execept inside mon_groups > + * directory. > + */ > + if (rdt_alloc_enabled && pkn == rdtgroup_default.kn) > + return rdtgroup_mkdir_ctrl_mon(pkn, pkn, name, mode); > + else if (rdt_mon_features && > + !strcmp(pkn->name, "mon_groups")) > + return rdtgroup_mkdir_mon(pkn, pkn->parent, name, mode); > + else > + return -EPERM; TBH, this is really convoluted (including the comment). /* * If the parent directory is the root directory and RDT * allocation is supported, add a control and monitoring * subdirectory. */ if (rdt_alloc_capable && parent_kn == rdtgroup_default.kn) return rdtgroup_mkdir_ctrl_mon(...); /* * If the parent directory is a monitoring group and RDT * monitoring is supported, add a monitoring subdirectory. */ if (rdt_mon_capable && is_mon_group(parent_kn)) return rdtgroup_mkdir_mon(...); return -EPERM; Note, that I did not use strcmp(parent_kn->name) because that's simply not sufficient. What prevents a user from doing: # mkdir /sys/fs/resctrl/mon_group/mon_group # mkdir /sys/fs/resctrl/mon_group/mon_group/foo You need a better way to distignuish that than strcmp(). You probably want to prevent creating subdirectories named "mon_group" as well. Thanks, tglx