Subject: Re: [PATCH RFC/RFT v3 6/9] powerpc: move cacheinfo sysfs to generic cacheinfo infrastructure

From: Sudeep Holla <[email-masked]>
Date: Mon, 10 Mar 2014 11:12:58 +0000

Hi Anshuman,

On 07/03/14 06:14, Anshuman Khandual wrote:
> On 03/07/2014 09:36 AM, Anshuman Khandual wrote:
>> On 02/19/2014 09:36 PM, Sudeep Holla wrote:
>>> From: Sudeep Holla <[email-masked]>
>>>
>>> This patch removes the redundant sysfs cacheinfo code by making use of
>>> the newly introduced generic cacheinfo infrastructure.
>>>
>>> Signed-off-by: Sudeep Holla <[email-masked]>
>>> Cc: Benjamin Herrenschmidt <[email-masked]>
>>> Cc: Paul Mackerras <[email-masked]>
>>> Cc: [email-masked]
>>> ---
>>>   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.
>>

Ah, that's silly mistake on my side, will fix it.

>> 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 !

Thanks for taking time for testing and reviewing these patches.

>
> 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.
>

Yes this is on my list, I need to avoid populating the sysfs files with 
"Unknown" as value, will do that in next version.

> 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.
>

Yes, based on the logs you have provided, I will check for the root 
cause of these issues. I will get back with questions if I need 
clarifications.

Regards,
Sudeep