Message-ID: <alpine.DEB.2.10.1707061415330.3798@vshiva-Udesk>
Date:   Thu, 6 Jul 2017 14:23:14 -0700 (PDT)
From:   Shivappa Vikas <vikas.shivappa@...el.com>
To:     Thomas Gleixner <tglx@...utronix.de>
cc:     Vikas Shivappa <vikas.shivappa@...ux.intel.com>, x86@...nel.org,
        linux-kernel@...r.kernel.org, hpa@...or.com, peterz@...radead.org,
        ravi.v.shankar@...el.com, vikas.shivappa@...el.com,
        tony.luck@...el.com, fenghua.yu@...el.com, andi.kleen@...el.com
Subject: Re: [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
>
>

Powered by blists - more mailing lists