On 03/10/2014 04:42 PM, Sudeep Holla wrote:
> 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.ho...@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.ho...@arm.com>
>>>> Cc: Benjamin Herrenschmidt <b...@kernel.crashing.org>
>>>> Cc: Paul Mackerras <pau...@samba.org>
>>>> Cc: linuxppc-...@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.
Now that you got some of the problems to work on and resend the patches, I will
hold on to the clean up patches I had.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
