Re: [PATCH 3/7] x86/intel_rdt: Support cache bit mask for Intel CAT

[Peter Zijlstra]

There's CAT in your subject, make up your minds already on what you want
to call this stuff.

On Fri, May 01, 2015 at 06:36:37PM -0700, Vikas Shivappa wrote:
> +static void rdt_free_closid(unsigned int clos)
> +{
> +

superfluous whitespace

> +	lockdep_assert_held(&rdt_group_mutex);
> +
> +	clear_bit(clos, rdtss_info.closmap);
> +}

> +static inline bool cbm_is_contiguous(unsigned long var)
> +{
> +	unsigned long first_bit, zero_bit;
> +	unsigned long maxcbm = MAX_CBM_LENGTH;

flip these two lines

> +
> +	if (!var)
> +		return false;
> +
> +	first_bit = find_next_bit(&var, maxcbm, 0);

What was wrong with find_first_bit() ?

> +	zero_bit = find_next_zero_bit(&var, maxcbm, first_bit);
> +
> +	if (find_next_bit(&var, maxcbm, zero_bit) < maxcbm)
> +		return false;
> +
> +	return true;
> +}
> +
> +static int cat_cbm_read(struct seq_file *m, void *v)
> +{
> +	struct intel_rdt *ir = css_rdt(seq_css(m));
> +
> +	seq_printf(m, "%08lx\n", ccmap[ir->clos].cache_mask);

inconsistent spacing, you mostly have a whilespace before the return
statement, but here you have not.

> +	return 0;
> +}
> +
> +static int validate_cbm(struct intel_rdt *ir, unsigned long cbmvalue)
> +{
> +	struct intel_rdt *par, *c;
> +	struct cgroup_subsys_state *css;
> +	unsigned long *cbm_tmp;

No reason no to order these on line length is there?

> +
> +	if (!cbm_is_contiguous(cbmvalue)) {
> +		pr_err("bitmask should have >= 1 bits and be contiguous\n");
> +		return -EINVAL;
> +	}
> +
> +	par = parent_rdt(ir);
> +	cbm_tmp = &ccmap[par->clos].cache_mask;
> +	if (!bitmap_subset(&cbmvalue, cbm_tmp, MAX_CBM_LENGTH))
> +		return -EINVAL;
> +
> +	rcu_read_lock();
> +	rdt_for_each_child(css, ir) {
> +		c = css_rdt(css);
> +		cbm_tmp = &ccmap[c->clos].cache_mask;
> +		if (!bitmap_subset(cbm_tmp, &cbmvalue, MAX_CBM_LENGTH)) {
> +			rcu_read_unlock();
> +			pr_err("Children's mask not a subset\n");
> +			return -EINVAL;
> +		}
> +	}
> +
> +	rcu_read_unlock();

Daft whitespace again.

> +	return 0;
> +}
> +
> +static bool cbm_search(unsigned long cbm, int *closid)
> +{
> +	int maxid = boot_cpu_data.x86_cat_closs;
> +	unsigned int i;
> +
> +	for (i = 0; i < maxid; i++) {
> +		if (bitmap_equal(&cbm, &ccmap[i].cache_mask, MAX_CBM_LENGTH)) {
> +			*closid = i;
> +			return true;
> +		}
> +	}

and again

> +	return false;
> +}
> +
> +static void cbmmap_dump(void)
> +{
> +	int i;
> +
> +	pr_debug("CBMMAP\n");
> +	for (i = 0; i < boot_cpu_data.x86_cat_closs; i++)
> +		pr_debug("cache_mask: 0x%x,clos_refcnt: %u\n",
> +		 (unsigned int)ccmap[i].cache_mask, ccmap[i].clos_refcnt);

This is missing {}

> +}
> +
> +static void __cpu_cbm_update(void *info)
> +{
> +	unsigned int closid = *((unsigned int *)info);
> +
> +	wrmsrl(CBM_FROM_INDEX(closid), ccmap[closid].cache_mask);
> +}

> +static int cat_cbm_write(struct cgroup_subsys_state *css,
> +				 struct cftype *cft, u64 cbmvalue)
> +{
> +	struct intel_rdt *ir = css_rdt(css);
> +	ssize_t err = 0;
> +	unsigned long cache_mask, max_mask;
> +	unsigned long *cbm_tmp;
> +	unsigned int closid;
> +	u32 max_cbm = boot_cpu_data.x86_cat_cbmlength;

That's just a right mess isn't it?

> +
> +	if (ir == &rdt_root_group)
> +		return -EPERM;
> +	bitmap_set(&max_mask, 0, max_cbm);
> +
> +	/*
> +	 * Need global mutex as cbm write may allocate a closid.
> +	 */
> +	mutex_lock(&rdt_group_mutex);
> +	bitmap_and(&cache_mask, (unsigned long *)&cbmvalue, &max_mask, max_cbm);
> +	cbm_tmp = &ccmap[ir->clos].cache_mask;
> +
> +	if (bitmap_equal(&cache_mask, cbm_tmp, MAX_CBM_LENGTH))
> +		goto out;
> +
> +	err = validate_cbm(ir, cache_mask);
> +	if (err)
> +		goto out;
> +
> +	/*
> +	 * At this point we are sure to change the cache_mask.Hence release the
> +	 * reference to the current CLOSid and try to get a reference for
> +	 * a different CLOSid.
> +	 */
> +	__clos_put(ir->clos);
> +
> +	if (cbm_search(cache_mask, &closid)) {
> +		ir->clos = closid;
> +		__clos_get(closid);
> +	} else {
> +		err = rdt_alloc_closid(ir);
> +		if (err)
> +			goto out;
> +
> +		ccmap[ir->clos].cache_mask = cache_mask;
> +		cbm_update_all(ir->clos);
> +	}
> +
> +	cbmmap_dump();
> +out:
> +

Daft whitespace again.. Also inconsistent return paradigm, here you use
an out label, where in validate_cbm() you did rcu_read_unlock() and
return from the middle.

> +	mutex_unlock(&rdt_group_mutex);
> +	return err;
> +}
> +
> +static inline bool rdt_update_cpumask(int cpu)
> +{
> +	int phys_id = topology_physical_package_id(cpu);
> +	struct cpumask *mask = &rdt_cpumask;
> +	int i;
> +
> +	for_each_cpu(i, mask) {
> +		if (phys_id == topology_physical_package_id(i))
> +			return false;
> +	}
> +
> +	cpumask_set_cpu(cpu, mask);

More daft whitespace

> +	return true;
> +}
> +
> +/*
> + * rdt_cpu_start() - If a new package has come up, update all
> + * the Cache bitmasks on the package.
> + */
> +static inline void rdt_cpu_start(int cpu)
> +{
> +	mutex_lock(&rdt_group_mutex);
> +	if (rdt_update_cpumask(cpu))
> +		cbm_update_msrs(cpu);
> +	mutex_unlock(&rdt_group_mutex);
> +}
> +
> +static void rdt_cpu_exit(unsigned int cpu)
> +{
> +	int phys_id = topology_physical_package_id(cpu);
> +	int i;
> +
> +	mutex_lock(&rdt_group_mutex);
> +	if (!cpumask_test_and_clear_cpu(cpu, &rdt_cpumask)) {
> +		mutex_unlock(&rdt_group_mutex);
> +		return;

And here we return from the middle again..

> +	}
> +
> +	for_each_online_cpu(i) {
> +		if (i == cpu)
> +			continue;
> +
> +		if (phys_id == topology_physical_package_id(i)) {
> +			cpumask_set_cpu(i, &rdt_cpumask);
> +			break;
> +		}
> +	}
> +	mutex_unlock(&rdt_group_mutex);
> +}

/me tired and gives up..
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/