| 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 linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <531D9E3A.7040109@arm.com>
Date: Mon, 10 Mar 2014 11:12:58 +0000
From: Sudeep Holla <sudeep.holla@....com>
To: Anshuman Khandual <khandual@...ux.vnet.ibm.com>
CC: Sudeep Holla <sudeep.holla@....com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Benjamin Herrenschmidt <benh@...nel.crashing.org>,
Paul Mackerras <paulus@...ba.org>,
"linuxppc-dev@...ts.ozlabs.org" <linuxppc-dev@...ts.ozlabs.org>
Subject: Re: [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@....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.
>>
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
--
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