lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Fri, 07 Mar 2014 11:44:49 +0530
From:	Anshuman Khandual <khandual@...ux.vnet.ibm.com>
To:	Sudeep Holla <sudeep.holla@....com>
CC:	linux-kernel@...r.kernel.org,
	Benjamin Herrenschmidt <benh@...nel.crashing.org>,
	Paul Mackerras <paulus@...ba.org>,
	linuxppc-dev@...ts.ozlabs.org
Subject: Re: [PATCH RFC/RFT v3 6/9] powerpc: move cacheinfo sysfs to generic
 cacheinfo infrastructure

On 03/07/2014 09:36 AM, Anshuman Khandual wrote:
> On 02/19/2014 09:36 PM, Sudeep Holla wrote:
>> From: Sudeep Holla <sudeep.holla@....com>
>>
>> This patch removes the redundant sysfs cacheinfo code by making use of
>> the newly introduced generic cacheinfo infrastructure.
>>
>> Signed-off-by: Sudeep Holla <sudeep.holla@....com>
>> Cc: Benjamin Herrenschmidt <benh@...nel.crashing.org>
>> Cc: Paul Mackerras <paulus@...ba.org>
>> Cc: linuxppc-dev@...ts.ozlabs.org
>> ---
>>  arch/powerpc/kernel/cacheinfo.c | 831 ++++++----------------------------------
>>  arch/powerpc/kernel/cacheinfo.h |   8 -
>>  arch/powerpc/kernel/sysfs.c     |   4 -
>>  3 files changed, 109 insertions(+), 734 deletions(-)
>>  delete mode 100644 arch/powerpc/kernel/cacheinfo.h
>>
>> diff --git a/arch/powerpc/kernel/cacheinfo.c b/arch/powerpc/kernel/cacheinfo.c
>> index 2912b87..05b7580 100644
>> --- a/arch/powerpc/kernel/cacheinfo.c
>> +++ b/arch/powerpc/kernel/cacheinfo.c
>> @@ -10,38 +10,10 @@
>>   * 2 as published by the Free Software Foundation.
>>   */
>>
>> +#include <linux/cacheinfo.h>
>>  #include <linux/cpu.h>
>> -#include <linux/cpumask.h>
>>  #include <linux/kernel.h>
>> -#include <linux/kobject.h>
>> -#include <linux/list.h>
>> -#include <linux/notifier.h>
>>  #include <linux/of.h>
>> -#include <linux/percpu.h>
>> -#include <linux/slab.h>
>> -#include <asm/prom.h>
>> -
>> -#include "cacheinfo.h"
>> -
>> -/* per-cpu object for tracking:
>> - * - a "cache" kobject for the top-level directory
>> - * - a list of "index" objects representing the cpu's local cache hierarchy
>> - */
>> -struct cache_dir {
>> -	struct kobject *kobj; /* bare (not embedded) kobject for cache
>> -			       * directory */
>> -	struct cache_index_dir *index; /* list of index objects */
>> -};
>> -
>> -/* "index" object: each cpu's cache directory has an index
>> - * subdirectory corresponding to a cache object associated with the
>> - * cpu.  This object's lifetime is managed via the embedded kobject.
>> - */
>> -struct cache_index_dir {
>> -	struct kobject kobj;
>> -	struct cache_index_dir *next; /* next index in parent directory */
>> -	struct cache *cache;
>> -};
>>
>>  /* Template for determining which OF properties to query for a given
>>   * cache type */
>> @@ -60,11 +32,6 @@ struct cache_type_info {
>>  	const char *nr_sets_prop;
>>  };
>>
>> -/* These are used to index the cache_type_info array. */
>> -#define CACHE_TYPE_UNIFIED     0
>> -#define CACHE_TYPE_INSTRUCTION 1
>> -#define CACHE_TYPE_DATA        2
>> -
>>  static const struct cache_type_info cache_type_info[] = {
>>  	{
>>  		/* PowerPC Processor binding says the [di]-cache-*
>> @@ -77,246 +44,115 @@ static const struct cache_type_info cache_type_info[] = {
>>  		.nr_sets_prop    = "d-cache-sets",
>>  	},
>>  	{
>> -		.name            = "Instruction",
>> -		.size_prop       = "i-cache-size",
>> -		.line_size_props = { "i-cache-line-size",
>> -				     "i-cache-block-size", },
>> -		.nr_sets_prop    = "i-cache-sets",
>> -	},
>> -	{
>>  		.name            = "Data",
>>  		.size_prop       = "d-cache-size",
>>  		.line_size_props = { "d-cache-line-size",
>>  				     "d-cache-block-size", },
>>  		.nr_sets_prop    = "d-cache-sets",
>>  	},
>> +	{
>> +		.name            = "Instruction",
>> +		.size_prop       = "i-cache-size",
>> +		.line_size_props = { "i-cache-line-size",
>> +				     "i-cache-block-size", },
>> +		.nr_sets_prop    = "i-cache-sets",
>> +	},
>>  };
> 
> 
> Hey Sudeep,
> 
> After applying this patch, the cache_type_info array looks like this.
> 
> static const struct cache_type_info cache_type_info[] = {
>         {
>                 /* 
>                  * PowerPC Processor binding says the [di]-cache-*
>                  * must be equal on unified caches, so just use
>                  * d-cache properties.
>                  */
>                 .name            = "Unified",
>                 .size_prop       = "d-cache-size",
>                 .line_size_props = { "d-cache-line-size",
>                                      "d-cache-block-size", },
>                 .nr_sets_prop    = "d-cache-sets",
>         },
>         {
>                 .name            = "Data",
>                 .size_prop       = "d-cache-size",
>                 .line_size_props = { "d-cache-line-size",
>                                      "d-cache-block-size", },
>                 .nr_sets_prop    = "d-cache-sets",
>         },
>         {
>                 .name            = "Instruction",
>                 .size_prop       = "i-cache-size",
>                 .line_size_props = { "i-cache-line-size",
>                                      "i-cache-block-size", },
>                 .nr_sets_prop    = "i-cache-sets",
>         },
> };
> 
> and this function computes the the array index for any given cache type
> define for PowerPC.
> 
> static inline int get_cacheinfo_idx(enum cache_type type)
> {
>         if (type == CACHE_TYPE_UNIFIED)
>                 return 0;
>         else
>                 return type;
> }
> 
> These types are define in include/linux/cacheinfo.h as
> 
> enum cache_type {
>         CACHE_TYPE_NOCACHE = 0,
>         CACHE_TYPE_INST = BIT(0),		---> 1
>         CACHE_TYPE_DATA = BIT(1),		---> 2
>         CACHE_TYPE_SEPARATE = CACHE_TYPE_INST | CACHE_TYPE_DATA,
>         CACHE_TYPE_UNIFIED = BIT(2),
> };
> 
> When it is UNIFIED we return index 0, which is correct. But the index
> for instruction and data cache seems to be swapped which wrong. This
> will fetch invalid properties for any given cache type.
> 
> I have done some initial review and testing for this patch's impact on
> PowerPC (ppc64 POWER specifically). I am trying to do some code clean-up
> and re-arrangements. Will post out soon. Thanks !

It does not work correctly on POWER.

The new patchset adds some more attributes for every cache entry apart from
what we used to have on PowerPC before. From the ABI perspective, the old ones
should reflect the correct value in the same manner as before. Looks like
the generic code will make any attribute as "Unknown" if the arch code does
not populate them in the respective callback.

Here are some problems found on a POWER7 system

(1) L1 instruction cache (cpu<N>/cache/index1/)

	====== Before patch ======

	coherency_line_size: 	128
	level:			1
	shared_cpu_map:		00000000,00000000,00000000,00000000,00000000,00000000,00000000,00000000,00000000,
        			00000000,00000000,00000000,00000000,00000000,00000000,00000000,00000000,00000000,
				00000000,00000000,00000000,00000000,00000000,00000000,00000000,00000000,00000000,
				00000000,00000000,00000000,00000000,00000f00
	size:			32K
	type:			Instruction 

	===== After patch ========

	coherency_line_size:	Unknown						----> Wrong
	level:			1
	shared_cpu_map:		00000000,00000000,00000000,00000000,00000000,00000000,00000000,00000000,00000000,
        			00000000,00000000,00000000,00000000,00000000,00000000,00000000,00000000,00000000,
				00000000,00000000,00000000,00000000,00000000,00000000,00000000,00000000,00000000,
				00000000,00000000,00000000,00000000,00ffffff	----> Wrong
	size:			0K						----> Wrong
	type:			Instruction	

(2) L3 cache (cpu<N>/cache/index3/)

	====== Before patch ======

	number_of_sets:		1
	size:			4096K
	ways_of_associativity:	0

	===== After patch ========

	number_of_sets:		1
	size:			4096K
	ways_of_associativity:	Unknown		----> Wrong

Need to revisit this implementation on PowerPC and figure out the cause of these problems.

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

Powered by blists - more mailing lists