lkml.org 
[lkml]   [2017]   [Jul]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 11/21] x86/intel_rdt/cqm: Add mkdir support for RDT monitoring


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
>
>

\
 
 \ /
  Last update: 2017-07-06 23:22    [W:0.105 / U:2.040 seconds]
©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site