lkml.org 
[lkml]   [2014]   [Mar]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH RFC/RFT v3 6/9] powerpc: move cacheinfo sysfs to generic cacheinfo infrastructure
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 <sudeep.holla@arm.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@arm.com>
>>> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>>> Cc: Paul Mackerras <paulus@samba.org>
>>> Cc: linuxppc-dev@lists.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.
>>

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



\
 
 \ /
  Last update: 2014-03-10 12:41    [W:0.129 / U:2.568 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site