Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758816Ab0LNKSR (ORCPT ); Tue, 14 Dec 2010 05:18:17 -0500 Received: from mail-fx0-f43.google.com ([209.85.161.43]:51081 "EHLO mail-fx0-f43.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754739Ab0LNKSP (ORCPT ); Tue, 14 Dec 2010 05:18:15 -0500 DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=sender:from:to:cc:subject:references:x-face:face:x-url:x-pgp-fp :x-pgp:date:in-reply-to:message-id:user-agent:mime-version :content-type; b=dnHqfY/B+xtLdvVBHb0MkZxxz9PwhVznFlH5SZgKMLuOfsroW2DWvp0bBnGtjobcal 8NBAeIy89FQIedvxmGDJvvTBRnYRlQzJfoTMX9uNHYuFVbusl78BBko2/YoiXnaqtJtv bVIiVkxYWVvOAaOVHKEmAm4UP6pxfV9+UnGS4= From: Michal Nazarewicz To: KAMEZAWA Hiroyuki Cc: Michal Nazarewicz , Andrew Morton , Ankita Garg , BooJin Kim , Daniel Walker , Johan MOSSBERG , Marek Szyprowski , Mel Gorman , "Paul E. McKenney" , linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-media@vger.kernel.org, linux-mm@kvack.org, Kyungmin Park Subject: Re: [PATCHv7 06/10] mm: MIGRATE_CMA migration type added References: <25250c1c5fb3ffd0c33ce744965bc8e958220f58.1292004520.git.m.nazarewicz@samsung.com> <20101214101508.4199a3dd.kamezawa.hiroyu@jp.fujitsu.com> X-Face: PbkBB1w#)bOqd`iCe"Ds{e+!C7`pkC9a|f)Qo^BMQvy\q5x3?vDQJeN(DS?|-^$uMti[3D*#^_Ts"pU$jBQLq~Ud6iNwAw_r_o_4]|JO?]}P_}Nc&"p#D(ZgUb4uCNPe7~a[DbPG0T~!&c.y$Ur,=N4RT>]dNpd;KFrfMCylc}gc??'U2j,!8%xdD Face: iVBORw0KGgoAAAANSUhEUgAAADAAAAAwBAMAAAClLOS0AAAAJFBMVEWbfGlUPDDHgE57V0jUupKjgIObY0PLrom9mH4dFRK4gmjPs41MxjOgAAACQElEQVQ4jW3TMWvbQBQHcBk1xE6WyALX1069oZBMlq+ouUwpEQQ6uRjttkWP4CmBgGM0BQLBdPFZYPsyFUo6uEtKDQ7oy/U96XR2Ux8ehH/89Z6enqxBcS7Lg81jmSuujrfCZcLI/TYYvbGj+jbgFpHJ/bqQAUISj8iLyu4LuFHJTosxsucO4jSDNE0Hq3hwK/ceQ5sx97b8LcUDsILfk+ovHkOIsMbBfg43VuQ5Ln9YAGCkUdKJoXR9EclFBhixy3EGVz1K6eEkhxCAkeMMnqoAhAKwhoUJkDrCqvbecaYINlFKSRS1i12VKH1XpUd4qxL876EkMcDvHj3s5RBajHHMlA5iK32e0C7VgG0RlzFPvoYHZLRmAC0BmNcBruhkE0KsMsbEc62ZwUJDxWUdMsMhVqovoT96i/DnX/ASvz/6hbCabELLk/6FF/8PNpPCGqcZTGFcBhhAaZZDbQPaAB3+KrWWy2XgbYDNIinkdWAFcCpraDE/knwe5DBqGmgzESl1p2E4MWAz0VUPgYYzmfWb9yS4vCvgsxJriNTHoIBz5YteBvg+VGISQWUqhMiByPIPpygeDBE6elD973xWwKkEiHZAHKjhuPsFnBuArrzxtakRcISv+XMIPl4aGBUJm8Emk7qBYU8IlgNEIpiJhk/No24jHwkKTFHDWfPniR4iw5vJaw2nzSjfq2zffcE/GDjRC2dn0J0XwPAbDL84TvaFCJEU4Oml9pRyEUhR3Cl2t01AoEjRbs0sYugp14/4X5n4pU4EHHnMAAAAAElFTkSuQmCC X-Url: http://mina86.com/ X-PGP-FP: 9134 06FA 7AD8 D134 9D0C C33F 532C CB00 B7C6 DF1E X-PGP: B7C6DF1E Date: Tue, 14 Dec 2010 11:18:09 +0100 In-Reply-To: <20101214101508.4199a3dd.kamezawa.hiroyu@jp.fujitsu.com> (KAMEZAWA Hiroyuki's message of "Tue, 14 Dec 2010 10:15:08 +0900") Message-ID: <874oaghd3y.fsf@erwin.mina86.com> User-Agent: Gnus/5.110011 (No Gnus v0.11) Emacs/24.0 (Slckware Linux) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 9033 Lines: 229 > On Mon, 13 Dec 2010 12:26:47 +0100 > Michal Nazarewicz wrote: >> diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h >> @@ -35,13 +35,24 @@ >> */ >> #define PAGE_ALLOC_COSTLY_ORDER 3 >> >> -#define MIGRATE_UNMOVABLE 0 >> -#define MIGRATE_RECLAIMABLE 1 >> -#define MIGRATE_MOVABLE 2 >> -#define MIGRATE_PCPTYPES 3 /* the number of types on the pcp lists */ >> -#define MIGRATE_RESERVE 3 >> -#define MIGRATE_ISOLATE 4 /* can't allocate from here */ >> -#define MIGRATE_TYPES 5 >> +enum { >> + MIGRATE_UNMOVABLE, >> + MIGRATE_RECLAIMABLE, >> + MIGRATE_MOVABLE, >> + MIGRATE_PCPTYPES, /* the number of types on the pcp lists */ >> + MIGRATE_RESERVE = MIGRATE_PCPTYPES, >> + MIGRATE_ISOLATE, /* can't allocate from here */ >> +#ifdef CONFIG_MIGRATE_CMA >> + MIGRATE_CMA, /* only movable */ >> +#endif >> + MIGRATE_TYPES >> +}; > KAMEZAWA Hiroyuki writes: > I personaly would like to put MIGRATE_ISOLATE to be bottom of enum > because it means _not_for_allocation. Will change. I didn't want to change the value of MIGRATE_ISOLATE in fear of breaking something but hopefully no one depends on MIGRATE_ISOLATE's value. >> diff --git a/mm/compaction.c b/mm/compaction.c >> @@ -824,11 +848,15 @@ struct page *__rmqueue_smallest(struct zone *zone, unsigned int order, >> * This array describes the order lists are fallen back to when >> * the free lists for the desirable migrate type are depleted >> */ >> -static int fallbacks[MIGRATE_TYPES][MIGRATE_TYPES-1] = { >> +static int fallbacks[MIGRATE_TYPES][4] = { >> [MIGRATE_UNMOVABLE] = { MIGRATE_RECLAIMABLE, MIGRATE_MOVABLE, MIGRATE_RESERVE }, >> [MIGRATE_RECLAIMABLE] = { MIGRATE_UNMOVABLE, MIGRATE_MOVABLE, MIGRATE_RESERVE }, >> +#ifdef CONFIG_MIGRATE_CMA >> + [MIGRATE_MOVABLE] = { MIGRATE_RECLAIMABLE, MIGRATE_UNMOVABLE, MIGRATE_CMA , MIGRATE_RESERVE }, >> +#else >> [MIGRATE_MOVABLE] = { MIGRATE_RECLAIMABLE, MIGRATE_UNMOVABLE, MIGRATE_RESERVE }, >> - [MIGRATE_RESERVE] = { MIGRATE_RESERVE, MIGRATE_RESERVE, MIGRATE_RESERVE }, /* Never used */ >> +#endif >> + [MIGRATE_RESERVE] = { MIGRATE_RESERVE }, /* Never used */ >> }; >> >> /* >> @@ -924,12 +952,12 @@ __rmqueue_fallback(struct zone *zone, int order, int start_migratetype) >> /* Find the largest possible block of pages in the other list */ >> for (current_order = MAX_ORDER-1; current_order >= order; >> --current_order) { >> - for (i = 0; i < MIGRATE_TYPES - 1; i++) { >> + for (i = 0; i < ARRAY_SIZE(fallbacks[0]); i++) { > Why fallbacks[0] ? and why do you need to change this ? I've changed the dimensions of fallbacks matrix, in particular second dimension from MIGRATE_TYPE - 1 to 4 so this place needed to be changed as well. Now, I think changing to ARRAY_SIZE() is just the safest option available. This is actually just a minor optimisation. >> migratetype = fallbacks[start_migratetype][i]; >> >> /* MIGRATE_RESERVE handled later if necessary */ >> if (migratetype == MIGRATE_RESERVE) >> - continue; >> + break; >> > Isn't this change enough for your purpose ? This is mostly just an optimisation really. I'm not sure what you think is my purpose here. ;) It does fix an this issue of some of the fallback[*] arrays having MIGRATETYPE_UNMOVABLE at the end. >> @@ -1042,7 +1083,12 @@ static int rmqueue_bulk(struct zone *zone, unsigned int order, >> list_add(&page->lru, list); >> else >> list_add_tail(&page->lru, list); >> - set_page_private(page, migratetype); >> +#ifdef CONFIG_MIGRATE_CMA >> + if (is_pageblock_cma(page)) >> + set_page_private(page, MIGRATE_CMA); >> + else >> +#endif >> + set_page_private(page, migratetype); > Hmm, doesn't this meet your changes which makes MIGRATE_CMA > > MIGRATE_PCPLIST ? And I think putting mixture of pages marked of > MIGRATE_TYPE onto a pcplist is ugly. You mean that pcplist page is on can disagree with page_private()? I didn't think this was such a big deal honestly. Unless MIGRATE_CMA < MIGRATE_PCPTYPES, a special case is needed either here or in free_pcppages_bulk(), so I think this comes down to whether to make MIGRATE_CAM < MIGRATE_PCPTYPES for which see below. >> @@ -1181,9 +1227,16 @@ void free_hot_cold_page(struct page *page, int cold) >> * offlined but treat RESERVE as movable pages so we can get those >> * areas back if necessary. Otherwise, we may have to free >> * excessively into the page allocator >> + * >> + * Still, do not change migration type of MIGRATE_CMA pages (if >> + * they'd be recorded as MIGRATE_MOVABLE an unmovable page could >> + * be allocated from MIGRATE_CMA block and we don't want to allow >> + * that). In this respect, treat MIGRATE_CMA like >> + * MIGRATE_ISOLATE. >> */ >> if (migratetype >= MIGRATE_PCPTYPES) { >> - if (unlikely(migratetype == MIGRATE_ISOLATE)) { >> + if (unlikely(migratetype == MIGRATE_ISOLATE >> + || is_migrate_cma(migratetype))) { >> free_one_page(zone, page, 0, migratetype); >> goto out; >> } > Doesn't this add *BAD* performance impact for usual use of pages > marked as MIGRATE_CMA ? IIUC, All pcp pages must be _drained_ at page > migration after making migrate_type as ISOLATED. So, this change > should be unnecessary. Come to think of it, it would appear that you are right. I'll remove this change. > BTW, How about changing MIGRATE_CMA < MIGRATE_PCPTYPES ? and allow to > have it's own pcp list ? > > I think > == > again: > if (likely(order == 0)) { > struct per_cpu_pages *pcp; > struct list_head *list; > > local_irq_save(flags); > pcp = &this_cpu_ptr(zone->pageset)->pcp; > list = &pcp->lists[migratetype]; > if (list_empty(list)) { > pcp->count += rmqueue_bulk(zone, 0, > pcp->batch, list, > migratetype, cold); > if (unlikely(list_empty(list))) { > + if (migrate_type == MIGRATE_MOVABLE) { /*allow extra fallback*/ > + migrate_type == MIGRATE_CMA > + goto again; (This unbalances local_irq_save but that's just a minor note.) > + } > + } > goto failed; > } > > if (cold) > page = list_entry(list->prev, struct page, lru); > else > page = list_entry(list->next, struct page, lru); > > list_del(&page->lru); > pcp->count--; > == > Will work enough as a fallback path which allows to allocate a memory > from CMA area if there aren't enough free pages. (and makes FALLBACK > type as) > > fallbacks[MIGRATE_CMA] = {?????}, > > for no fallbacks. Yes, I think that would work. I didn't want to create a new pcp list especially since in most respects it behaves just like MIGRATE_MOVABLE. Moreover, with MIGRATE_MOVABLE and MIGRATE_CMA sharing the same pcp list the above additional fallback path is not necessary and instead the already existing __rmqueue_fallback() path can be used. >> @@ -1272,7 +1325,8 @@ int split_free_page(struct page *page) >> if (order >= pageblock_order - 1) { >> struct page *endpage = page + (1 << order) - 1; >> for (; page < endpage; page += pageblock_nr_pages) >> - set_pageblock_migratetype(page, MIGRATE_MOVABLE); >> + if (!is_pageblock_cma(page)) >> + set_pageblock_migratetype(page, MIGRATE_MOVABLE); >> } >> >> return 1 << order; >> @@ -5366,6 +5420,15 @@ int set_migratetype_isolate(struct page *page) >> zone_idx = zone_idx(zone); >> >> spin_lock_irqsave(&zone->lock, flags); >> + /* >> + * Treat MIGRATE_CMA specially since it may contain immobile >> + * CMA pages -- that's fine. CMA is likely going to touch >> + * only the mobile pages in the pageblokc. >> + */ >> + if (is_pageblock_cma(page)) { >> + ret = 0; >> + goto out; >> + } >> >> pfn = page_to_pfn(page); >> arg.start_pfn = pfn; > Hmm, I'm not sure why you dont' have any change in __free_one_page() > which overwrite pageblock type. Is MIGRATE_CMA range is aligned to > MAX_ORDER ? If so, please mention about it in patch description or > comment because of the patch order. Yep, you're correct. For MIGRATE_CMA to be usable, pages marked with it must be aligned to MAX_ORDER_NR_PAGES. I'll add that in a comment somewhere. -- Best regards, _ _ .o. | Liege of Serenly Enlightened Majesty of o' \,=./ `o ..o | Computer Science, Michal "mina86" Nazarewicz (o o) ooo +------ooO--(_)--Ooo-- -- 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/