Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752721AbdGFVVm (ORCPT ); Thu, 6 Jul 2017 17:21:42 -0400 Received: from mga05.intel.com ([192.55.52.43]:32433 "EHLO mga05.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751936AbdGFVVk (ORCPT ); Thu, 6 Jul 2017 17:21:40 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.40,319,1496127600"; d="scan'208";a="283393872" Date: Thu, 6 Jul 2017 14:23:14 -0700 (PDT) From: Shivappa Vikas X-X-Sender: vikas@vshiva-Udesk To: Thomas Gleixner cc: Vikas Shivappa , 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: 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.10 (DEB 1266 2009-07-14) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5592 Lines: 203 On Sun, 2 Jul 2017, Thomas Gleixner wrote: > 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? the prkn is always the kn for parent rdtgroup where as pkn is the parent kn. May be parent_kn parent_kn_rdtgrp ? Wanted to make it shorter. > >> + 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(). Will fix all above. > >> + 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; Will fix. > > 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 > This would fail because the parent rdtgrp when creating foo is NULL. This is because the parent rdtgrp is taken from the "resctrl/mon_group/mon_group" directory's parent which is the resctrl/mon_groups->priv. We always keep this NULL. So user can create a mon_groups under resctr/mon_groups but cant create a dir under that.. > You need a better way to distignuish that than strcmp(). You probably want > to prevent creating subdirectories named "mon_group" as well. > If creating a monitor group named mon_group is confusing then it can be checked. Thanks, Vikas > Thanks, > > tglx > >