lkml.org 
[lkml]   [2007]   [Jun]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH 5/7] Introduce a means of compacting memory within a zone
    From
    On (18/06/07 10:18), Christoph Lameter didst pronounce:
    > On Mon, 18 Jun 2007, Mel Gorman wrote:
    >
    > > + /* Isolate free pages. This assumes the block is valid */
    > > + for (; blockpfn < end_pfn; blockpfn++) {
    > > + struct page *page;
    > > + int isolated, i;
    > > +
    > > + if (!pfn_valid_within(blockpfn))
    > > + continue;
    > > +
    > > + page = pfn_to_page(blockpfn);
    > > + if (!PageBuddy(page))
    > > + continue;
    >
    > The name PageBuddy is getting to be misleading. Maybe rename this to
    > PageFree or so?
    >

    That would be suprisingly ambiguous. per-cpu pages are free pages but are not
    PageBuddy pages. In this case, I really mean a PageBuddy page, not a free page.

    > > +
    > > + /* Found a free page, break it into order-0 pages */
    > > + isolated = split_free_page(page);
    > > + total_isolated += isolated;
    > > + for (i = 0; i < isolated; i++) {
    > > + list_add(&page->lru, freelist);
    > > + page++;
    > > + }
    >
    > Why do you need to break them all up? Easier to coalesce later?
    >

    They are broken up because migration currently works on order-0 pages.
    It is easier to break them up now for compaction_alloc() to give out one
    at a time than trying to figure out how to split them up later.

    > > +/* Returns 1 if the page is within a block suitable for migration to */
    > > +static int pageblock_migratable(struct page *page)
    > > +{
    > > + /* If the page is a large free page, then allow migration */
    > > + if (PageBuddy(page) && page_order(page) >= pageblock_order)
    > > + return 1;
    >
    > if (PageSlab(page) && page->slab->ops->kick) {
    > migratable slab
    > }
    >
    > if (page table page) {
    > migratable page table page?
    > }
    >
    > etc?
    >

    Not quite. pageblock_migratable() is telling if this block is suitable for
    taking free pages from so movable pages can be migrated there. Right now
    that means checking if there are enough free pages that the whole block
    becomes MOVABLE or if the block is already being used for movable pages.

    The block could become movable if the decision was made to kick out slab
    pages that are located towards the end of the zone. If page tables
    become movable, then they would need to be identified here but that is
    not the case.

    The pageblock_migratable() function is named so that this decision can
    be easily revisited in one place.

    > > + /* Try isolate the page */
    > > + if (locked_isolate_lru_page(zone, page, migratelist) == 0)
    > > + isolated++;
    >
    > Support for other ways of migrating a page?
    >

    When other mechanisms exist, they would be added here. Right now,
    isolate_lru_page() is the only one I am aware of.

    > > +static int compact_zone(struct zone *zone, struct compact_control *cc)
    > > +{
    > > + int ret = COMPACT_INCOMPLETE;
    > > +
    > > + /* Setup to move all movable pages to the end of the zone */
    > > + cc->migrate_pfn = zone->zone_start_pfn;
    > > + cc->free_pfn = cc->migrate_pfn + zone->spanned_pages;
    > > + cc->free_pfn &= ~(pageblock_nr_pages-1);
    > > +
    > > + for (; ret == COMPACT_INCOMPLETE; ret = compact_finished(zone, cc)) {
    > > + isolate_migratepages(zone, cc);
    > > +
    > > + if (!cc->nr_migratepages)
    > > + continue;
    > > +
    > > + /* Isolate free pages if necessary */
    > > + if (cc->nr_freepages < cc->nr_migratepages)
    > > + isolate_freepages(zone, cc);
    > > +
    > > + /* Stop compacting if we cannot get enough free pages */
    > > + if (cc->nr_freepages < cc->nr_migratepages)
    > > + break;
    > > +
    > > + migrate_pages(&cc->migratepages, compaction_alloc,
    > > + (unsigned long)cc);
    >
    > You do not need to check the result of migration? Page migration is a best
    > effort that may fail.
    >

    You're right. I used to check it for debugging purposes to make sure migration
    was actually occuring. It is not unusual still for a fair number of pages
    to fail to migrate. migration already uses a retry logic and I shouldn't
    be replicating it.

    More importantly, by leaving the pages on the migratelist, I potentially
    retry the same migrations over and over again wasting time and effort not
    to mention that I keep pages isolated for much longer than necessary and
    that could cause stalling problems. I should be calling putback_lru_pages()
    when migrate_pages() tells me it failed to migrate pages.

    I'll revisit this one. Thanks

    > Looks good otherwise.
    >
    > Acked-by: Christoph Lameter <clameter@sgi.com>

    --
    --
    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-06-19 18:39    [W:2.365 / U:0.072 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site