Re: [PATCH v5 13/18] x86/intel_rdt: Add mkdir to resctrl file system

[Thomas Gleixner <tglx@linutronix.de>]

On Sat, 22 Oct 2016, Fenghua Yu wrote:
> +/*
> + * Trivial allocator for CLOSIDs. Since h/w only supports a small number,
> + * we can keep a bitmap of free CLOSIDs in a single integer.
> + *
> + * Using a global CLOSID across all resources has some advantages and
> + * some drawbacks:
> + * + We can simply set "current->closid" to assign a task to a resource
> + *   group.
> + * + Context switch code can avoid extra memory references deciding which
> + *   CLOSID to load into the PQR_ASSOC MSR
> + * - We give up some options in configuring resource groups across multi-socket
> + *   systems.
> + * - Our choices on how to configure each resource become progressively more
> + *   limited as the number of resources grows.
> + */
> +static int closid_free_map;
> +
> +static void closid_init(void)
> +{
> +	struct rdt_resource *r;
> +	int rdt_max_closid;
> +
> +	/* Compute rdt_max_closid across all resources */
> +	rdt_max_closid = 0;
> +	for_each_enabled_rdt_resource(r)
> +		rdt_max_closid = max(rdt_max_closid, r->num_closid);

So you decided to silently ignore my objections against this approach. Fine
with me, but that does not solve the problem at all.

Once more:

On a system with L2 and L3 CAT it does not make any sense at all to expose
the closids which exceed the L2 space. Simply because using them wreckages
any L2 partitioning done in the valid L2 space.

If you really want to allow that, then:

   1) It must be a opt-in at mount time

   2) It must be documented clearly along with the mount option

> +	/*
> +	 * CDP is "special". Because we share the L3 CBM MSR array
> +	 * between L3DATA and L3CODE, we must not use a CLOSID larger
> +	 * than they support. Just check against L3DATA because it
> +	 * is the same as L3CODE.
> +	 */
> +	r = &rdt_resources_all[RDT_RESOURCE_L3DATA];
> +	if (r->enabled)
> +		rdt_max_closid = min(rdt_max_closid, r->num_closid);

This explicit special casing is crap, really.

	for_each_enabled_rdt_resource(r)
		rdt_max_closid = max(rdt_max_closid, r->num_closid);

	for_each_enabled_rdt_resource(r) {
		if (!relaxed_max_closid || r->force_min_closid)
			rdt_max_closid = min(rdt_max_closid, r->num_closid);
	}

Handles all cases without 'CDP is special' and whatever nonsense intel will
come up with in future. All you need to do is to add that force_min_closid
field into the resource struct and set it for l3data and l3code.

relaxed_max_closid is set at mount time by an appropriate mount option.

Thanks,

	tglx