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
