From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758706AbaCUDqU (ORCPT ); Thu, 20 Mar 2014 23:46:20 -0400 Received: from e28smtp08.in.ibm.com ([122.248.162.8]:34305 "EHLO e28smtp08.in.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751613AbaCUDqS (ORCPT ); Thu, 20 Mar 2014 23:46:18 -0400 Message-ID: <532BB5AC.3000904@linux.vnet.ibm.com> Date: Fri, 21 Mar 2014 09:14:44 +0530 From: Anshuman Khandual User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/17.0 Thunderbird/17.0 MIME-Version: 1.0 To: Sudeep Holla CC: "linux-kernel@vger.kernel.org" , Benjamin Herrenschmidt , Paul Mackerras , "linuxppc-dev@lists.ozlabs.org" Subject: Re: [PATCH RFC/RFT v3 6/9] powerpc: move cacheinfo sysfs to generic cacheinfo infrastructure References: <1392825976-17633-1-git-send-email-sudeep.holla@arm.com> <1392825976-17633-7-git-send-email-sudeep.holla@arm.com> <531945D1.7010503@linux.vnet.ibm.com> <531963D9.5040701@linux.vnet.ibm.com> <531D9E3A.7040109@arm.com> In-Reply-To: <531D9E3A.7040109@arm.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-TM-AS-MML: disable X-Content-Scanned: Fidelis XPS MAILER x-cbid: 14032103-2000-0000-0000-00001048644B Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 >>>> >>>> This patch removes the redundant sysfs cacheinfo code by making use of >>>> the newly introduced generic cacheinfo infrastructure. >>>> >>>> Signed-off-by: Sudeep Holla >>>> Cc: Benjamin Herrenschmidt >>>> Cc: Paul Mackerras >>>> 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 >>>> #include >>>> -#include >>>> #include >>>> -#include >>>> -#include >>>> -#include >>>> #include >>>> -#include >>>> -#include >>>> -#include >>>> - >>>> -#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.