Message-ID: <20160701102143.GB4749@pd.tnic>
Date:	Fri, 1 Jul 2016 12:21:43 +0200
From:	Borislav Petkov <bp@...e.de>
To:	Fenghua Yu <fenghua.yu@...el.com>
Cc:	Thomas Gleixner <tglx@...utronix.de>, Ingo Molnar <mingo@...e.hu>,
	"H. Peter Anvin" <h.peter.anvin@...el.com>,
	Tony Luck <tony.luck@...el.com>,
	Peter Zijlstra <peterz@...radead.org>,
	Stephane Eranian <eranian@...gle.com>,
	Ravi V Shankar <ravi.v.shankar@...el.com>,
	Vikas Shivappa <vikas.shivappa@...ux.intel.com>,
	linux-kernel <linux-kernel@...r.kernel.org>, x86 <x86@...nel.org>
Subject: Re: [PATCH] cacheinfo: Introduce cache id

On Wed, Jun 29, 2016 at 06:56:10PM -0700, Fenghua Yu wrote:
> From: Fenghua Yu <fenghua.yu@...el.com>
> 
> Each cache node is described by cacheinfo and is a unique node across

What is a cache node?

> the platform. But there is no id for a cache node. We introduce cache ID
> to identify a cache node.
> 
> Intel Cache Allocation Technology (CAT) allows some control on the
> allocation policy within each cache that it controls. We need a unique
> cache ID for each cache level to allow the user to specify which
> controls are applied to which cache.
> 
> The cache id is first enabled on x86. It can be enabled on other platforms
> as well. The cache id is not necessary contiguous.
> 
> Add an "id" entry to /sys/devices/system/cpu/cpu*/cache/index*/
> 
> Signed-off-by: Fenghua Yu <fenghua.yu@...el.com>
> ---
>  arch/x86/kernel/cpu/intel_cacheinfo.c | 21 +++++++++++++++++++++
>  drivers/base/cacheinfo.c              |  5 +++++
>  include/linux/cacheinfo.h             |  3 +++
>  3 files changed, 29 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/intel_cacheinfo.c b/arch/x86/kernel/cpu/intel_cacheinfo.c
> index de6626c..aefc1e5 100644
> --- a/arch/x86/kernel/cpu/intel_cacheinfo.c
> +++ b/arch/x86/kernel/cpu/intel_cacheinfo.c
> @@ -153,6 +153,7 @@ struct _cpuid4_info_regs {
>  	union _cpuid4_leaf_eax eax;
>  	union _cpuid4_leaf_ebx ebx;
>  	union _cpuid4_leaf_ecx ecx;
> +	unsigned int id;
>  	unsigned long size;
>  	struct amd_northbridge *nb;
>  };
> @@ -894,6 +895,9 @@ static void __cache_cpumap_setup(unsigned int cpu, int index,
>  static void ci_leaf_init(struct cacheinfo *this_leaf,
>  			 struct _cpuid4_info_regs *base)
>  {
> +	this_leaf->id = base->id;
> +	/* Set CACHE_ID bit in attributes to enable cache id in x86. */

No need for that comment.

> +	this_leaf->attributes = CACHE_ID;
>  	this_leaf->level = base->eax.split.level;
>  	this_leaf->type = cache_type_map[base->eax.split.type];
>  	this_leaf->coherency_line_size =
> @@ -920,6 +924,22 @@ static int __init_cache_level(unsigned int cpu)
>  	return 0;
>  }
>  
> +/*
> + * The max shared threads number comes from CPUID.4:EAX[25-14] with input
> + * ECX as cache index. Then right shift apicid by the number's order to get
> + * cache id for this cache node.
> + */
> +static void get_cache_id(int cpu, struct _cpuid4_info_regs *id4_regs)
> +{
> +	struct cpuinfo_x86 *c = &cpu_data(cpu);
> +	unsigned long num_threads_sharing;
> +	int index_msb;
> +
> +	num_threads_sharing = 1 + id4_regs->eax.split.num_threads_sharing;
> +	index_msb = get_count_order(num_threads_sharing);
> +	id4_regs->id = c->apicid >> index_msb;
> +}
> +
>  static int __populate_cache_leaves(unsigned int cpu)
>  {
>  	unsigned int idx, ret;
> @@ -931,6 +951,7 @@ static int __populate_cache_leaves(unsigned int cpu)
>  		ret = cpuid4_cache_lookup_regs(idx, &id4_regs);
>  		if (ret)
>  			return ret;
> +		get_cache_id(cpu, &id4_regs);
>  		ci_leaf_init(this_leaf++, &id4_regs);
>  		__cache_cpumap_setup(cpu, idx, &id4_regs);
>  	}
> diff --git a/drivers/base/cacheinfo.c b/drivers/base/cacheinfo.c
> index e9fd32e..2a21c15 100644
> --- a/drivers/base/cacheinfo.c
> +++ b/drivers/base/cacheinfo.c
> @@ -233,6 +233,7 @@ static ssize_t file_name##_show(struct device *dev,		\
>  	return sprintf(buf, "%u\n", this_leaf->object);		\
>  }
>  
> +show_one(id, id);
>  show_one(level, level);
>  show_one(coherency_line_size, coherency_line_size);
>  show_one(number_of_sets, number_of_sets);
> @@ -314,6 +315,7 @@ static ssize_t write_policy_show(struct device *dev,
>  	return n;
>  }
>  
> +static DEVICE_ATTR_RO(id);
>  static DEVICE_ATTR_RO(level);
>  static DEVICE_ATTR_RO(type);
>  static DEVICE_ATTR_RO(coherency_line_size);
> @@ -327,6 +329,7 @@ static DEVICE_ATTR_RO(shared_cpu_list);
>  static DEVICE_ATTR_RO(physical_line_partition);
>  
>  static struct attribute *cache_default_attrs[] = {
> +	&dev_attr_id.attr,
>  	&dev_attr_type.attr,
>  	&dev_attr_level.attr,
>  	&dev_attr_shared_cpu_map.attr,
> @@ -350,6 +353,8 @@ cache_default_attrs_is_visible(struct kobject *kobj,
>  	const struct cpumask *mask = &this_leaf->shared_cpu_map;
>  	umode_t mode = attr->mode;
>  
> +	if ((attr == &dev_attr_id.attr) && this_leaf->attributes & CACHE_ID)
> +		return mode;
>  	if ((attr == &dev_attr_type.attr) && this_leaf->type)
>  		return mode;
>  	if ((attr == &dev_attr_level.attr) && this_leaf->level)
> diff --git a/include/linux/cacheinfo.h b/include/linux/cacheinfo.h
> index 2189935..16e5573 100644
> --- a/include/linux/cacheinfo.h
> +++ b/include/linux/cacheinfo.h
> @@ -18,6 +18,7 @@ enum cache_type {
>  
>  /**
>   * struct cacheinfo - represent a cache leaf node
> + * @id: id of this cache node. This is unique id across the platform.

So you need to explain what a "cache node" is. Especially since this is
generic code and other arches might not necessarily have a notion of a
"cache node" - whatever that is.

And since this patch adds the generic functionality and then enables it
on x86, it should be split into two patches.

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

Powered by blists - more mailing lists