From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753575Ab2GBGiY (ORCPT ); Mon, 2 Jul 2012 02:38:24 -0400 Received: from mx1.redhat.com ([209.132.183.28]:10626 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750878Ab2GBGiW (ORCPT ); Mon, 2 Jul 2012 02:38:22 -0400 Message-ID: <4FF14196.6040106@redhat.com> Date: Mon, 02 Jul 2012 02:37:10 -0400 From: Rik van Riel User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:12.0) Gecko/20120430 Thunderbird/12.0.1 MIME-Version: 1.0 To: Andrea Arcangeli CC: linux-kernel@vger.kernel.org, linux-mm@kvack.org, Hillf Danton , Dan Smith , Peter Zijlstra , Linus Torvalds , Andrew Morton , Thomas Gleixner , Ingo Molnar , Paul Turner , Suresh Siddha , Mike Galbraith , "Paul E. McKenney" , Lai Jiangshan , Bharata B Rao , Lee Schermerhorn , Johannes Weiner , Srivatsa Vaddagiri , Christoph Lameter , Alex Shi , Mauricio Faria de Oliveira , Konrad Rzeszutek Wilk , Don Morris , Benjamin Herrenschmidt Subject: Re: [PATCH 36/40] autonuma: page_autonuma References: <1340888180-15355-1-git-send-email-aarcange@redhat.com> <1340888180-15355-37-git-send-email-aarcange@redhat.com> In-Reply-To: <1340888180-15355-37-git-send-email-aarcange@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 06/28/2012 08:56 AM, Andrea Arcangeli wrote: > +++ b/include/linux/autonuma_flags.h > @@ -15,6 +15,12 @@ enum autonuma_flag { > > extern unsigned long autonuma_flags; > > +static inline bool autonuma_impossible(void) > +{ > + return num_possible_nodes()<= 1 || > + test_bit(AUTONUMA_IMPOSSIBLE_FLAG,&autonuma_flags); > +} When you fix the name of this function, could you also put it in the right spot, in the patch where it is originally introduced? Moving stuff around for no reason in a patch series is not very reviewer friendly. > diff --git a/include/linux/autonuma_types.h b/include/linux/autonuma_types.h > index 9e697e3..1e860f6 100644 > --- a/include/linux/autonuma_types.h > +++ b/include/linux/autonuma_types.h > @@ -39,6 +39,61 @@ struct task_autonuma { > unsigned long task_numa_fault[0]; > }; > > +/* > + * Per page (or per-pageblock) structure dynamically allocated only if > + * autonuma is not impossible. > + */ Double negatives are not easy to read. s/not impossible/enabled/ > +struct page_autonuma { > + /* > + * To modify autonuma_last_nid lockless the architecture, > + * needs SMP atomic granularity< sizeof(long), not all archs > + * have that, notably some ancient alpha (but none of those > + * should run in NUMA systems). Archs without that requires > + * autonuma_last_nid to be a long. > + */ If only all your data structures were documented like this. I guess that will give you something to do, when addressing the comments on the other patches :) > diff --git a/mm/huge_memory.c b/mm/huge_memory.c > index bcaa8ac..c5e47bc 100644 > --- a/mm/huge_memory.c > +++ b/mm/huge_memory.c > #ifdef CONFIG_AUTONUMA > - /* pick the last one, better than nothing */ > - autonuma_last_nid = > - ACCESS_ONCE(src_page->autonuma_last_nid); > - if (autonuma_last_nid>= 0) > - ACCESS_ONCE(page->autonuma_last_nid) = > - autonuma_last_nid; > + if (!autonuma_impossible()) { > + int autonuma_last_nid; > + src_page_an = lookup_page_autonuma(src_page); > + /* pick the last one, better than nothing */ > + autonuma_last_nid = > + ACCESS_ONCE(src_page_an->autonuma_last_nid); > + if (autonuma_last_nid>= 0) > + ACCESS_ONCE(page_an->autonuma_last_nid) = > + autonuma_last_nid; > + } Remembering the last page the loop went through, and then looking up the autonuma struct after you exit the loop could be better. > diff --git a/mm/page_autonuma.c b/mm/page_autonuma.c > new file mode 100644 > index 0000000..bace9b8 > --- /dev/null > +++ b/mm/page_autonuma.c > @@ -0,0 +1,234 @@ > +#include > +#include > +#include This should be There is absolutely no good reason why that one-liner change is a separate patch. > +struct page_autonuma *lookup_page_autonuma(struct page *page) > +{ > + offset = pfn - NODE_DATA(page_to_nid(page))->node_start_pfn; > + return base + offset; > +} Doing this and the reverse allows you to drop the page pointer in struct autonuma. It would make sense to do that either in this patch, or in a new one, but either way pulling it forward out of patch 40 would make the series easier to review for the next round. > +fail: > + printk(KERN_CRIT "allocation of page_autonuma failed.\n"); > + printk(KERN_CRIT "please try the 'noautonuma' boot option\n"); > + panic("Out of memory"); > +} The system can run just fine without autonuma. Would it make sense to simply disable autonuma at this point, but to try continue running? > @@ -700,8 +780,14 @@ static void free_section_usemap(struct page *memmap, unsigned long *usemap) > */ > if (PageSlab(usemap_page)) { > kfree(usemap); > - if (memmap) > + if (memmap) { > __kfree_section_memmap(memmap, PAGES_PER_SECTION); > + if (!autonuma_impossible()) > + __kfree_section_page_autonuma(page_autonuma, > + PAGES_PER_SECTION); > + else > + BUG_ON(page_autonuma); VM_BUG_ON ? > + if (!autonuma_impossible()) { > + struct page *page_autonuma_page; > + page_autonuma_page = virt_to_page(page_autonuma); > + free_map_bootmem(page_autonuma_page, nr_pages); > + } else > + BUG_ON(page_autonuma); ditto > pgdat_resize_unlock(pgdat,&flags); > if (ret<= 0) { > + if (!autonuma_impossible()) > + __kfree_section_page_autonuma(page_autonuma, nr_pages); > + else > + BUG_ON(page_autonuma); VM_BUG_ON ? -- All rights reversed