Re: [PATCH 05/12] Memory compaction core

[Minchan Kim]

On Fri, Feb 19, 2010 at 2:34 AM, Mel Gorman <mel@xxxxxxxxx> wrote:
> On Fri, Feb 19, 2010 at 01:58:44AM +0900, Minchan Kim wrote:
>> On Fri, 2010-02-12 at 12:00 +0000, Mel Gorman wrote:
>> > +/* Isolate free pages onto a private freelist. Must hold zone->lock */
>> > +static int isolate_freepages_block(struct zone *zone,
>>
>> return type 'int'?
>> I think we can't return signed value.
>>
>
> I don't understand your query. What's wrong with returning int?

It's just nitpick. I mean this functions doesn't return minus value.
Never mind.

>
>> > + Â Â Â Â Â Â Â Â Â Â Â Â Â unsigned long blockpfn,
>> > + Â Â Â Â Â Â Â Â Â Â Â Â Â struct list_head *freelist)
>> > +{
>> > + Â unsigned long zone_end_pfn, end_pfn;
>> > + Â int total_isolated = 0;
>> > +
>> > + Â /* Get the last PFN we should scan for free pages at */
>> > + Â zone_end_pfn = zone->zone_start_pfn + zone->spanned_pages;
>> > + Â end_pfn = blockpfn + pageblock_nr_pages;
>> > + Â if (end_pfn > zone_end_pfn)
>> > + Â Â Â Â Â end_pfn = zone_end_pfn;
>> > +
>> > + Â /* 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;
>> > +
>> > + Â Â Â Â Â /* 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++;
>> > + Â Â Â Â Â }
>> > + Â Â Â Â Â blockpfn += isolated - 1;
>
> Incidentally, this line is wrong but will be fixed in line 3. If
> split_free_page() fails, it causes an infinite loop.
>
>> > + Â }
>> > +
>> > + Â return total_isolated;
>> > +}
>> > +
>> > +/* Returns 1 if the page is within a block suitable for migration to */
>> > +static int suitable_migration_target(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 the block is MIGRATE_MOVABLE, allow migration */
>> > + Â if (get_pageblock_migratetype(page) == MIGRATE_MOVABLE)
>> > + Â Â Â Â Â return 1;
>> > +
>> > + Â /* Otherwise skip the block */
>> > + Â return 0;
>> > +}
>> > +
>> > +/*
>> > + * Based on information in the current compact_control, find blocks
>> > + * suitable for isolating free pages from
>> > + */
>> > +static void isolate_freepages(struct zone *zone,
>> > + Â Â Â Â Â Â Â Â Â Â Â Â Â struct compact_control *cc)
>> > +{
>> > + Â struct page *page;
>> > + Â unsigned long high_pfn, low_pfn, pfn;
>> > + Â unsigned long flags;
>> > + Â int nr_freepages = cc->nr_freepages;
>> > + Â struct list_head *freelist = &cc->freepages;
>> > +
>> > + Â pfn = cc->free_pfn;
>> > + Â low_pfn = cc->migrate_pfn + pageblock_nr_pages;
>> > + Â high_pfn = low_pfn;
>> > +
>> > + Â /*
>> > + Â Â* Isolate free pages until enough are available to migrate the
>> > + Â Â* pages on cc->migratepages. We stop searching if the migrate
>> > + Â Â* and free page scanners meet or enough free pages are isolated.
>> > + Â Â*/
>> > + Â spin_lock_irqsave(&zone->lock, flags);
>> > + Â for (; pfn > low_pfn && cc->nr_migratepages > nr_freepages;
>> > + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â pfn -= pageblock_nr_pages) {
>> > + Â Â Â Â Â int isolated;
>> > +
>> > + Â Â Â Â Â if (!pfn_valid(pfn))
>> > + Â Â Â Â Â Â Â Â Â continue;
>> > +
>> > + Â Â Â Â Â /* Check for overlapping nodes/zones */
>> > + Â Â Â Â Â page = pfn_to_page(pfn);
>> > + Â Â Â Â Â if (page_zone(page) != zone)
>> > + Â Â Â Â Â Â Â Â Â continue;
>>
>> We are progressing backward by physical page order in a zone.
>> If we meet crossover between zone, Why are we going backward
>> continuously? Before it happens, migration and free scanner would meet.
>> Am I miss something?
>>
>
> I was considering a situation like the following
>
>
> Node-0 Â Â Node-1 Â Â Â Node-0
> DMA Â Â Â ÂDMA Â Â Â Â ÂDMA
> 0-1023 Â Â 1024-2047 Â Â2048-4096
>
> In that case, a PFN scanner can enter a new node and zone but the migrate
> and free scanners have not necessarily met. This configuration is *extremely*
> rare but it happens on messed-up LPAR configurations on POWER.

I don't know such architecture until now.
Thanks for telling me.
How about adding the comment about that?

>
>> > +
>> > + Â Â Â Â Â /* Check the block is suitable for migration */
>> > + Â Â Â Â Â if (!suitable_migration_target(page))
>> > + Â Â Â Â Â Â Â Â Â continue;
>>
>> Dumb question.
>> suitable_migration_target considers three type's pages
>>
>> 1. free page and page's order >= pageblock_order
>> 2. free pages and pages's order < pageblock_order with movable page
>> 3. used page with movable
>>
>> I can understand 1 and 2 but can't 3. This function is for gathering
>> free page. How do you handle used page as free one?
>>
>> In addition, as I looked into isolate_freepages_block, it doesn't
>> consider 3 by PageBuddy check.
>>
>> I am confusing. Pz, correct me.
>>
>
> I'm afraid I don't understand your question. At the point
> suitable_migration_target() is called, the only concern is finding a pageblock
> of pages that should be scanned for free pages by isolate_freepages_block().
> What do you mean by "used page with movable" ?

After I looked into code, I understand it.
Thanks.

<snip>
>>> +/* Similar to split_page except the page is already free */
>> Sometime, this function changes pages's type to MIGRATE_MOVABLE.
>> I hope adding comment about that.
>>
>
> There is a comment within the function about it. Do you want it moved to
> here?

If you don't mind, I hope so. :)

That's because you wrote down only "except the page is already free" in
function description. So I thought it's only difference with split_page at first
glance. I think information that setting MIGRATE_MOVABLE is important.

Pz, thinks it as just nitpick.
Thanks, Mel.


-- 
Kind regards,
Minchan Kim
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/