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

[Thomas Gleixner <tglx@linutronix.de>]

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?

> +			    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().

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

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

You need a better way to distignuish that than strcmp(). You probably want
to prevent creating subdirectories named "mon_group" as well.

Thanks,

	tglx