Re: [PATCH 11/21] x86/intel_rdt/cqm: Add mkdir support for RDT monitoring

[Shivappa Vikas]

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