Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933237Ab3HNUre (ORCPT ); Wed, 14 Aug 2013 16:47:34 -0400 Received: from www.sr71.net ([198.145.64.142]:42658 "EHLO blackbird.sr71.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933004Ab3HNUrc (ORCPT ); Wed, 14 Aug 2013 16:47:32 -0400 Message-ID: <520BECDF.8060501@sr71.net> Date: Wed, 14 Aug 2013 13:47:27 -0700 From: Dave Hansen User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130803 Thunderbird/17.0.8 MIME-Version: 1.0 To: Seth Jennings CC: Greg Kroah-Hartman , Nathan Fontenot , Cody P Schafer , Andrew Morton , Lai Jiangshan , "Rafael J. Wysocki" , linux-kernel@vger.kernel.org, linux-mm@kvack.org Subject: Re: [RFC][PATCH] drivers: base: dynamic memory block creation References: <1376508705-3188-1-git-send-email-sjenning@linux.vnet.ibm.com> In-Reply-To: <1376508705-3188-1-git-send-email-sjenning@linux.vnet.ibm.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4291 Lines: 127 On 08/14/2013 12:31 PM, Seth Jennings wrote: > There was a significant amount of refactoring to allow for this but > IMHO, the code is much easier to understand now. ... > drivers/base/memory.c | 248 +++++++++++++++++++++++++++++++++++++------------ > include/linux/memory.h | 1 - > 2 files changed, 188 insertions(+), 61 deletions(-) Adding 120 lines of code made it easier to understand? ;) > diff --git a/drivers/base/memory.c b/drivers/base/memory.c > index 2b7813e..392ccd3 100644 > --- a/drivers/base/memory.c > +++ b/drivers/base/memory.c > @@ -30,7 +30,7 @@ static DEFINE_MUTEX(mem_sysfs_mutex); > > #define MEMORY_CLASS_NAME "memory" > > -static int sections_per_block; > +static int sections_per_block __read_mostly; > > static inline int base_memory_block_id(int section_nr) > { > @@ -47,6 +47,9 @@ static struct bus_type memory_subsys = { > .offline = memory_subsys_offline, > }; > > +static unsigned long *memblock_present; > +static bool largememory_enable __read_mostly; How would you see this getting used in practice? Are you just going to set this by default on ppc? Or, would you ask the distros to put it on the command-line by default? Would it only affect machines larger than a certain size? This approach breaks the ABI, right?. An existing tool would not work with this patch (plus boot option) since it would not know how to show/hide things. It lets _part_ of those existing tools get reused since they only have to be taught how to show/hide things. I'd find this really intriguing if you found a way to keep even the old tools working. Instead of having an explicit show/hide, why couldn't you just create the entries on open(), for instance? > int register_memory_notifier(struct notifier_block *nb) > @@ -565,16 +568,13 @@ static const struct attribute_group *memory_memblk_attr_groups[] = { > static > int register_memory(struct memory_block *memory) > { > - int error; > - > memory->dev.bus = &memory_subsys; > memory->dev.id = memory->start_section_nr / sections_per_block; > memory->dev.release = memory_block_release; > memory->dev.groups = memory_memblk_attr_groups; > memory->dev.offline = memory->state == MEM_OFFLINE; > > - error = device_register(&memory->dev); > - return error; > + return device_register(&memory->dev); > } This kind of simplification could surely stand in its own patch. > static int init_memory_block(struct memory_block **memory, > @@ -582,67 +582,72 @@ static int init_memory_block(struct memory_block **memory, > { > struct memory_block *mem; > unsigned long start_pfn; > - int scn_nr; > - int ret = 0; > + int scn_nr, ret, memblock_id; > > + *memory = NULL; > mem = kzalloc(sizeof(*mem), GFP_KERNEL); > if (!mem) > return -ENOMEM; > > scn_nr = __section_nr(section); > + memblock_id = base_memory_block_id(scn_nr); > mem->start_section_nr = > base_memory_block_id(scn_nr) * sections_per_block; > mem->end_section_nr = mem->start_section_nr + sections_per_block - 1; > mem->state = state; > - mem->section_count++; > mutex_init(&mem->state_mutex); > start_pfn = section_nr_to_pfn(mem->start_section_nr); > mem->phys_device = arch_get_memory_phys_device(start_pfn); > > ret = register_memory(mem); > + if (ret) { > + kfree(mem); > + return ret; > + } > > *memory = mem; > - return ret; > + return 0; > } memblock_id doesn't appear to ever get used. This also appears to change the conventions about where the 'memory_block' is allocated and freed. It isn't immediately clear why it needed to be changed. Looking at the rest, this _really_ needs to get refactored before it's reviewable. > +static ssize_t memory_present_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + int n_bits, ret; > + > + n_bits = NR_MEM_SECTIONS / sections_per_block; > + ret = bitmap_scnlistprintf(buf, PAGE_SIZE - 2, > + memblock_present, n_bits); > + buf[ret++] = '\n'; > + buf[ret] = '\0'; > + > + return ret; > +} Doesn't this break the one-value-per-file rule? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/