lkml.org 
[lkml]   [2007]   [May]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [RFC] memory hotremove patch take 2 [02/10] (make page unused)
    On Wed, 9 May 2007, Yasunori Goto wrote:

    > This patch is for supporting making page unused.
    >

    Without reading the patch, this could also be interesting when trying to
    free a block of pages for a contiguous allocation without racing against
    other allocators.

    > Isolate pages by capturing freed pages before inserting free_area[],
    > buddy allocator.
    > If you have an idea for avoiding spin_lock(), please advise me.
    >

    Again, commenting on this before I read the patch. Grouping pages by
    mobility uses a bitmap to track flags affecting a block of pages. If you
    used a bit there and added a MIGRATE_ISOLATING type, the pages on free
    would get placed in those freelists. As long as MIGRATE_ISOLATING is not
    in fallbacks[] in page_alloc.c, the pages would not get allocated. This
    should avoid the need for a separate spinlock.

    That said, it increases the size of struct zone more than yours do and
    ties these patches to a part of grouping pages by mobility which you don't
    do currently.

    > Isolating pages in free_area[] is implemented in other patch.
    >

    I haven't seen that part yet but it sounds like it does something similar
    to move_freepages() so there may be code to be shared there.

    > Signed-Off-By: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
    > Signed-off-by: Yasunori Goto <y-goto@jp.fujitsu.com>
    >
    >
    > include/linux/mmzone.h | 8 +
    > include/linux/page_isolation.h | 52 +++++++++++
    > mm/Kconfig | 7 +
    > mm/page_alloc.c | 187 +++++++++++++++++++++++++++++++++++++++++
    > 4 files changed, 254 insertions(+)
    >
    > Index: current_test/include/linux/mmzone.h
    > ===================================================================
    > --- current_test.orig/include/linux/mmzone.h 2007-05-08 15:06:49.000000000 +0900
    > +++ current_test/include/linux/mmzone.h 2007-05-08 15:08:03.000000000 +0900
    > @@ -314,6 +314,14 @@ struct zone {
    > /* zone_start_pfn == zone_start_paddr >> PAGE_SHIFT */
    > unsigned long zone_start_pfn;
    >
    > +#ifdef CONFIG_PAGE_ISOLATION
    > + /*
    > + * For pages which are not used but not free.
    > + * See include/linux/page_isolation.h
    > + */
    > + spinlock_t isolation_lock;
    > + struct list_head isolation_list;
    > +#endif

    Using MIGRATE_ISOLATING instead of this approach does mean that there will
    be MAX_ORDER additional struct free_area added to the zone. That is more
    lists than this approach.

    I am somewhat suprised that CONFIG_PAGE_ISOLATION exists as a separate
    option. If it was a compile-time option at all, I would expect it to
    depend on memory hot-remove being selected.

    > /*
    > * zone_start_pfn, spanned_pages and present_pages are all
    > * protected by span_seqlock. It is a seqlock because it has
    > Index: current_test/mm/page_alloc.c
    > ===================================================================
    > --- current_test.orig/mm/page_alloc.c 2007-05-08 15:07:20.000000000 +0900
    > +++ current_test/mm/page_alloc.c 2007-05-08 15:08:34.000000000 +0900
    > @@ -41,6 +41,7 @@
    > #include <linux/pfn.h>
    > #include <linux/backing-dev.h>
    > #include <linux/fault-inject.h>
    > +#include <linux/page_isolation.h>
    >
    > #include <asm/tlbflush.h>
    > #include <asm/div64.h>
    > @@ -448,6 +449,9 @@ static inline void __free_one_page(struc
    > if (unlikely(PageCompound(page)))
    > destroy_compound_page(page, order);
    >
    > + if (page_under_isolation(zone, page, order))
    > + return;
    > +

    Using MIGRATE_ISOLATING would avoid a potential list search here.

    > page_idx = page_to_pfn(page) & ((1 << MAX_ORDER) - 1);
    >
    > VM_BUG_ON(page_idx & (order_size - 1));
    > @@ -3259,6 +3263,10 @@ static void __meminit free_area_init_cor
    > zone->nr_scan_inactive = 0;
    > zap_zone_vm_stats(zone);
    > atomic_set(&zone->reclaim_in_progress, 0);
    > +#ifdef CONFIG_PAGE_ISOLATION
    > + spin_lock_init(&zone->isolation_lock);
    > + INIT_LIST_HEAD(&zone->isolation_list);
    i> +#endif
    > if (!size)
    > continue;
    >
    > @@ -4214,3 +4222,182 @@ void set_pageblock_flags_group(struct pa
    > else
    > __clear_bit(bitidx + start_bitidx, bitmap);
    > }
    > +
    > +#ifdef CONFIG_PAGE_ISOLATION
    > +/*
    > + * Page Isolation.
    > + *
    > + * If a page is removed from usual free_list and will never be used,
    > + * It is linked to "struct isolation_info" and set Reserved, Private
    > + * bit. page->mapping points to isolation_info in it.
    > + * and page_count(page) is 0.
    > + *
    > + * This can be used for creating a chunk of contiguous *unused* memory.
    > + *
    > + * current user is Memory-Hot-Remove.
    > + * maybe move to some other file is better.

    page_isolation.c to match the header filename seems reasonable.
    page_alloc.c has a lot of multi-function stuff like memory initialisation
    in it.

    > + */
    > +static void
    > +isolate_page_nolock(struct isolation_info *info, struct page *page, int order)
    > +{
    > + int pagenum;
    > + pagenum = 1 << order;
    > + while (pagenum > 0) {
    > + SetPageReserved(page);
    > + SetPagePrivate(page);
    > + page->private = (unsigned long)info;
    > + list_add(&page->lru, &info->pages);
    > + page++;
    > + pagenum--;
    > + }
    > +}

    It's worth commenting somewhere that pages on the list in isolation_info
    are always order-0.

    > +
    > +/*
    > + * This function is called from page_under_isolation()
    > + */
    > +
    > +int __page_under_isolation(struct zone *zone, struct page *page, int order)
    > +{
    > + struct isolation_info *info;
    > + unsigned long pfn = page_to_pfn(page);
    > + unsigned long flags;
    > + int found = 0;
    > +
    > + spin_lock_irqsave(&zone->isolation_lock,flags);

    An unwritten convention seems to be that __ versions of same-named
    functions are the nolock version. i.e. I would expect
    page_under_isolation() to acquire and release the spinlock and
    __page_under_isolation() to do no additional locking.

    Locking outside of here might make the flow a little clearer as well if
    you had two returns and avoided the use of "found".

    > + list_for_each_entry(info, &zone->isolation_list, list) {
    > + if (info->start_pfn <= pfn && pfn < info->end_pfn) {
    > + found = 1;
    > + break;
    > + }
    > + }
    > + if (found) {
    > + isolate_page_nolock(info, page, order);
    > + }
    > + spin_unlock_irqrestore(&zone->isolation_lock, flags);
    > + return found;
    > +}
    > +
    > +/*
    > + * start and end must be in the same zone.
    > + *
    > + */
    > +struct isolation_info *
    > +register_isolation(unsigned long start, unsigned long end)
    > +{
    > + struct zone *zone;
    > + struct isolation_info *info = NULL, *tmp;
    > + unsigned long flags;
    > + unsigned long last_pfn = end - 1;
    > +
    > + if (!pfn_valid(start) || !pfn_valid(last_pfn) || (start >= end))
    > + return ERR_PTR(-EINVAL);
    > + /* check start and end is in the same zone */
    > + zone = page_zone(pfn_to_page(start));
    > +
    > + if (zone != page_zone(pfn_to_page(last_pfn)))
    > + return ERR_PTR(-EINVAL);
    > + /* target range has to match MAX_ORDER alignmet */
    > + if ((start & (MAX_ORDER_NR_PAGES - 1)) ||
    > + (end & (MAX_ORDER_NR_PAGES - 1)))
    > + return ERR_PTR(-EINVAL);

    Why does the range have to be MAX_ORDER alighned?

    > + info = kmalloc(sizeof(*info), GFP_KERNEL);
    > + if (!info)
    > + return ERR_PTR(-ENOMEM);
    > + spin_lock_irqsave(&zone->isolation_lock, flags);
    > + /* we don't allow overlap among isolation areas */
    > + if (!list_empty(&zone->isolation_list)) {
    > + list_for_each_entry(tmp, &zone->isolation_list, list) {
    > + if (start < tmp->end_pfn && end > tmp->start_pfn) {
    > + goto out_free;
    > + }
    > + }
    > + }

    Why not merge requests for overlapping isolations?

    > + info->start_pfn = start;
    > + info->end_pfn = end;
    > + info->zone = zone;
    > + INIT_LIST_HEAD(&info->list);
    > + INIT_LIST_HEAD(&info->pages);
    > + list_add(&info->list, &zone->isolation_list);
    > +out_unlock:
    > + spin_unlock_irqrestore(&zone->isolation_lock, flags);
    > + return info;
    > +out_free:
    > + kfree(info);
    > + info = ERR_PTR(-EBUSY);
    > + goto out_unlock;
    > +}
    > +/*
    > + * Remove IsolationInfo from zone.
    > + * After this, we can unuse memory in info or
    > + * free back to freelist.
    > + */
    > +
    > +void
    > +detach_isolation_info_zone(struct isolation_info *info)
    > +{
    > + unsigned long flags;
    > + struct zone *zone = info->zone;
    > + spin_lock_irqsave(&zone->isolation_lock,flags);
    > + list_del(&info->list);
    > + info->zone = NULL;
    > + spin_unlock_irqrestore(&zone->isolation_lock,flags);
    > +}
    > +
    > +/*
    > + * All pages in info->pages should be remvoed before calling this.
    > + * And info should be detached from zone.
    > + */
    > +void
    > +free_isolation_info(struct isolation_info *info)
    > +{
    > + BUG_ON(!list_empty(&info->pages));
    > + BUG_ON(info->zone);
    > + kfree(info);
    > + return;
    > +}
    > +
    > +/*
    > + * Mark All pages in the isolation_info to be Reserved.
    > + * When onlining these pages again, a user must check
    > + * which page is usable by IORESOURCE_RAM
    > + * please see memory_hotplug.c/online_pages() if unclear.
    > + *
    > + * info should be detached from zone before calling this.
    > + */
    > +void
    > +unuse_all_isolated_pages(struct isolation_info *info)
    > +{
    > + struct page *page, *n;
    > + BUG_ON(info->zone);
    > + list_for_each_entry_safe(page, n, &info->pages, lru) {
    > + SetPageReserved(page);
    > + page->private = 0;
    > + ClearPagePrivate(page);
    > + list_del(&page->lru);
    > + }
    > +}
    > +
    > +/*
    > + * Free all pages connected in isolation list.
    > + * pages are moved back to free_list.
    > + */
    > +void
    > +free_all_isolated_pages(struct isolation_info *info)
    > +{
    > + struct page *page, *n;
    > + BUG_ON(info->zone);
    > + list_for_each_entry_safe(page, n ,&info->pages, lru) {
    > + ClearPagePrivate(page);
    > + ClearPageReserved(page);
    > + page->private = 0;
    > + list_del(&page->lru);
    > + set_page_count(page, 0);
    > + set_page_refcounted(page);
    > + /* This is sage because info is detached from zone */

    s/sage/safe/

    > + __free_page(page);
    > + }
    > +}
    > +
    > +#endif /* CONFIG_PAGE_ISOLATION */
    > +
    > +
    > Index: current_test/mm/Kconfig
    > ===================================================================
    > --- current_test.orig/mm/Kconfig 2007-05-08 15:06:50.000000000 +0900
    > +++ current_test/mm/Kconfig 2007-05-08 15:08:31.000000000 +0900
    > @@ -225,3 +225,10 @@ config DEBUG_READAHEAD
    >
    > Say N for production servers.
    >
    > +config PAGE_ISOLATION
    > + bool "Page Isolation Framework"
    > + help
    > + This option adds page isolation framework to mm.
    > + This is used for isolate amount of contiguous pages from linux
    > + memory management.
    > + Say N if unsure.
    > Index: current_test/include/linux/page_isolation.h
    > ===================================================================
    > --- /dev/null 1970-01-01 00:00:00.000000000 +0000
    > +++ current_test/include/linux/page_isolation.h 2007-05-08 15:08:34.000000000 +0900
    > @@ -0,0 +1,52 @@
    > +#ifndef __LINIX_PAGE_ISOLATION_H
    > +#define __LINUX_PAGE_ISOLATION_H
    > +
    > +#ifdef CONFIG_PAGE_ISOLATION
    > +
    > +struct isolation_info {
    > + struct list_head list;
    > + unsigned long start_pfn;
    > + unsigned long end_pfn;
    > + struct zone *zone;
    > + struct list_head pages;
    > +};
    > +
    > +extern int
    > +__page_under_isolation(struct zone *zone, struct page *page, int order);
    > +
    > +static inline int
    > +page_under_isolation(struct zone *zone, struct page *page, int order)
    > +{
    > + if (likely(list_empty(&zone->isolation_list)))
    > + return 0;
    > + return __page_under_isolation(zone, page, order);
    > +}
    > +
    > +static inline int
    > +is_page_isolated(struct isolation_info *info, struct page *page)
    > +{
    > + if (PageReserved(page) && PagePrivate(page) &&
    > + page_count(page) == 0 &&
    > + page->private == (unsigned long)info)
    > + return 1;
    > + return 0;
    > +}
    > +
    > +extern struct isolation_info *
    > +register_isolation(unsigned long start, unsigned long end);
    > +
    > +extern void detach_isolation_info_zone(struct isolation_info *info);
    > +extern void free_isolation_info(struct isolation_info *info);
    > +extern void unuse_all_isolated_pages(struct isolation_info *info);
    > +extern void free_all_isolated_pages(struct isolation_info *info);
    > +
    > +#else
    > +
    > +static inline int
    > +page_under_isolation(struct zone *zone, struct page *page, int order)
    > +{
    > + return 0;
    > +}
    > +
    > +#endif
    > +#endif
    >
    > --
    > Yasunori Goto
    >
    >

    --
    Mel Gorman
    Part-time Phd Student Linux Technology Center
    University of Limerick IBM Dublin Software Lab
    -
    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/

    \
     
     \ /
      Last update: 2007-05-10 17:37    [W:0.048 / U:32.224 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site