Message-ID: <alpine.DEB.2.20.1610171546410.9580@nanos>
Date:   Mon, 17 Oct 2016 16:44:53 +0200 (CEST)
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Fenghua Yu <fenghua.yu@...el.com>
cc:     "H. Peter Anvin" <h.peter.anvin@...el.com>,
        Ingo Molnar <mingo@...e.hu>, Tony Luck <tony.luck@...el.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Stephane Eranian <eranian@...gle.com>,
        Borislav Petkov <bp@...e.de>,
        Dave Hansen <dave.hansen@...el.com>,
        Nilay Vaish <nilayvaish@...il.com>, Shaohua Li <shli@...com>,
        David Carrillo-Cisneros <davidcc@...gle.com>,
        Ravi V Shankar <ravi.v.shankar@...el.com>,
        Sai Prakhya <sai.praneeth.prakhya@...el.com>,
        Vikas Shivappa <vikas.shivappa@...ux.intel.com>,
        linux-kernel <linux-kernel@...r.kernel.org>, x86 <x86@...nel.org>
Subject: Re: [PATCH v4 10/18] x86/intel_rdt: Build structures for each resource
 based on cache topology

On Fri, 14 Oct 2016, Fenghua Yu wrote:
>  
> +static int get_cache_id(int cpu, int level)
> +{
> +	struct cpu_cacheinfo *ci = get_cpu_cacheinfo(cpu);
> +	int i;
> +
> +	for (i = 0; i < ci->num_leaves; i++)
> +		if (ci->info_list[i].level == level)
> +			return ci->info_list[i].id;

Multi line statements need curly braces.

> +	return -1;
> +}
> +
> +void rdt_cbm_update(void *arg)
> +{
> +	struct msr_param *m = (struct msr_param *)arg;
> +	struct rdt_resource *r = m->res;
> +	struct rdt_domain *d;
> +	struct list_head *l;
> +	int i, cpu = smp_processor_id();
> +
> +	list_for_each(l, &r->domains) {
> +		d = list_entry(l, struct rdt_domain, list);

We have list_for_each_entry() ....

> +		if (cpumask_test_cpu(cpu, &d->cpu_mask))
> +			goto found;
> +	}
> +	pr_info_once("cpu %d not found in any domain for resource %s\n",
> +		     cpu, r->name);

If the list is empty, then 'd' is not initialized and dereferenced. If the
list is not empty then you use the last domain in the list. Neither one is
the right thing to do....

> +
> +found:
> +	for (i = m->low; i < m->high; i++)
> +		wrmsrl(r->msr_base + i, d->cbm[i]);
> +}
> +
> +static void update_domain(int cpu, struct rdt_resource *r, int add)
> +{
> +	struct list_head *l;
> +	struct rdt_domain *d;
> +	int i, cache_id;

+	struct rdt_domain *d;
+	struct list_head *l;
+	int i, cache_id;

Can you see how this is simpler to read?

> +
> +	cache_id = get_cache_id(cpu, r->cache_level);

> +	if (cache_id == -1) {
> +		pr_info_once("Could't find cache id for cpu %d\n", cpu);
> +		return;
> +	}

Please seperate this by a empty line.

> +	list_for_each(l, &r->domains) {
> +		d = list_entry(l, struct rdt_domain, list);

list_for_each_entry() once more.

> +		if (cache_id == d->id)
> +			goto found;
> +		if (cache_id < d->id)
> +			break;
> +	}
> +	if (!add) {
> +		pr_info_once("removed unknown cpu %d\n", cpu);

_once? Why should this happen at all?

> +		return;
> +	}
> +	d = kzalloc(sizeof(*d), GFP_KERNEL);

Shouldn't this be kzalloc_node() ?

> +	if (!d)
> +		return;
> +
> +	d->id = cache_id;
> +	d->cbm = kmalloc_array(r->max_closid, sizeof(*d->cbm), GFP_KERNEL);
> +	if (!d->cbm) {
> +		pr_info("Failed to alloc CBM array for cpu %d\n", cpu);
> +		kfree(d);
> +		return;
> +	}
> +	cpumask_set_cpu(cpu, &d->cpu_mask);
> +	for (i = 0; i < r->max_closid; i++) {
> +		d->cbm[i] = r->max_cbm;
> +		wrmsrl(r->msr_base + i, d->cbm[i]);
> +	}
> +	list_add_tail(&d->list, l);
> +	r->num_domains++;
> +	return;
> +
> +found:
> +	if (add) {
> +		cpumask_set_cpu(cpu, &d->cpu_mask);

Gah. Reusing this function for add and del is just a silly optimization
which makes the code harder to read.

All you share is the find thing. So you can split that out into a helper:

static struct rdt_domain *rdt_find_domain(struct rdt_resource *r,
					  unsigned int cpu, int id)
{
	if (id < 0)
		return ERR_PTR(id);

	list_for_each_entry(d, &r->domains, list) {
		if (d->id == cache_id)
			return d;
	}
	return NULL;
}

and use it for seperate add/del functions.

static void domain_add_cpu(unsigned int cpu, struct rdt_resource *r)
{
	int id = get_cache_id(cpu, r->cache_level);
	struct rdt_domain *d;

	d = rdt_find_domain(r, cpu, id);
	if (IS_ERR(d)) {
		pr_warn("Print some useful information\n", r->cache_level, cpu, ....);
		return;
	}

	if (d) {
		cpumask_set_cpu(cpu, &d->cpu_mask);
		return;
	}

   	/* Do the allocaction stuff */
}

static void domain_remove_cpu(unsigned int cpu, struct rdt_resource *r)
{
	int id = get_cache_id(cpu, r->cache_level);
	struct rdt_domain *d;

	d = rdt_find_domain(r, cpu, id);
	if (IS_ERR_OR_NULL(d)) {
		pr_warn("Print some useful information\n", r->cache_level, cpu, ...);
		return;
	}

	cpumask_clear_cpu(cpu, &d->cpu_mask);
	....	
}

Hmm?

>  static int __init intel_rdt_late_init(void)
>  {
>  	struct rdt_resource *r;
> +	int state;
>  
>  	if (!get_rdt_resources())
>  		return -ENODEV;
>  
> +	state = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
> +				"AP_INTEL_RDT_ONLINE",

Please use: "x86/rdt/cat:online:" or similar. We messed that up in a few
places when adding the names, but we don't want to add more of this.

> +				intel_rdt_online_cpu, intel_rdt_offline_cpu);

Thanks,

	tglx

Powered by blists - more mailing lists