From: Sudeep Holla <sudeep.holla@arm.com>
To: Anshuman Khandual <khandual@linux.vnet.ibm.com>
Cc: Sudeep Holla <sudeep.holla@arm.com>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
Benjamin Herrenschmidt <benh@kernel.crashing.org>,
Paul Mackerras <paulus@samba.org>,
"linuxppc-dev@lists.ozlabs.org" <linuxppc-dev@lists.ozlabs.org>
Subject: Re: [PATCH RFC/RFT v3 6/9] powerpc: move cacheinfo sysfs to generic cacheinfo infrastructure
Date: Mon, 10 Mar 2014 11:12:58 +0000 [thread overview]
Message-ID: <531D9E3A.7040109@arm.com> (raw)
In-Reply-To: <531963D9.5040701@linux.vnet.ibm.com>
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
next prev parent reply other threads:[~2014-03-10 11:12 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-02-19 16:06 [PATCH RFC/RFT v3 0/9] drivers: cacheinfo support Sudeep Holla
2014-02-19 16:06 ` [PATCH RFC/RFT v3 1/9] drivers: base: add new class "cpu" to group cpu devices Sudeep Holla
2014-03-01 0:42 ` Greg Kroah-Hartman
2014-03-03 7:28 ` Sudeep Holla
2014-02-19 16:06 ` [PATCH RFC/RFT v3 2/9] drivers: base: support cpu cache information interface to userspace via sysfs Sudeep Holla
2014-03-01 0:42 ` Greg Kroah-Hartman
2014-03-03 7:39 ` Sudeep Holla
2014-02-19 16:06 ` [PATCH RFC/RFT v3 3/9] ia64: move cacheinfo sysfs to generic cacheinfo infrastructure Sudeep Holla
2014-02-19 16:06 ` [PATCH RFC/RFT v3 4/9] s390: " Sudeep Holla
2014-02-20 8:38 ` Heiko Carstens
2014-02-20 13:33 ` Sudeep Holla
2014-02-20 14:07 ` Heiko Carstens
2014-02-20 14:37 ` Sudeep Holla
2014-02-19 16:06 ` [PATCH RFC/RFT v3 5/9] x86: " Sudeep Holla
2014-02-19 16:06 ` [PATCH RFC/RFT v3 6/9] powerpc: " Sudeep Holla
2014-03-07 4:06 ` Anshuman Khandual
2014-03-07 6:14 ` Anshuman Khandual
2014-03-10 11:12 ` Sudeep Holla [this message]
2014-03-21 3:44 ` Anshuman Khandual
2014-03-21 12:04 ` Sudeep Holla
2014-02-19 16:06 ` [PATCH RFC/RFT v3 7/9] ARM64: kernel: add support for cpu cache information Sudeep Holla
2014-02-19 16:06 ` [PATCH RFC/RFT v3 8/9] ARM: " Sudeep Holla
2014-02-19 16:06 ` [PATCH RFC/RFT v3 9/9] ARM: kernel: add outer cache support for cacheinfo implementation Sudeep Holla
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=531D9E3A.7040109@arm.com \
--to=sudeep.holla@arm.com \
--cc=benh@kernel.crashing.org \
--cc=khandual@linux.vnet.ibm.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=paulus@samba.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).