From: Joonsoo Kim <iamjoonsoo.kim@lge.com>
To: Vlastimil Babka <vbabka@suse.cz>
Cc: Andrew Morton <akpm@linux-foundation.org>,
Rik van Riel <riel@redhat.com>,
Johannes Weiner <hannes@cmpxchg.org>,
mgorman@techsingularity.net, Laura Abbott <lauraa@codeaurora.org>,
Minchan Kim <minchan@kernel.org>,
Marek Szyprowski <m.szyprowski@samsung.com>,
Michal Nazarewicz <mina86@mina86.com>,
"Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>,
linux-mm@kvack.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v5 3/6] mm/cma: populate ZONE_CMA
Date: Thu, 22 Sep 2016 15:50:48 +0900 [thread overview]
Message-ID: <20160922065048.GD27958@js1304-P5Q-DELUXE> (raw)
In-Reply-To: <20160922054546.GC27958@js1304-P5Q-DELUXE>
On Thu, Sep 22, 2016 at 02:45:46PM +0900, Joonsoo Kim wrote:
> On Wed, Sep 21, 2016 at 11:20:11AM +0200, Vlastimil Babka wrote:
> > On 08/29/2016 07:07 AM, js1304@gmail.com wrote:
> > >From: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> > >
> > >Until now, reserved pages for CMA are managed in the ordinary zones
> > >where page's pfn are belong to. This approach has numorous problems
> > >and fixing them isn't easy. (It is mentioned on previous patch.)
> > >To fix this situation, ZONE_CMA is introduced in previous patch, but,
> > >not yet populated. This patch implement population of ZONE_CMA
> > >by stealing reserved pages from the ordinary zones.
> > >
> > >Unlike previous implementation that kernel allocation request with
> > >__GFP_MOVABLE could be serviced from CMA region, allocation request only
> > >with GFP_HIGHUSER_MOVABLE can be serviced from CMA region in the new
> > >approach. This is an inevitable design decision to use the zone
> > >implementation because ZONE_CMA could contain highmem. Due to this
> > >decision, ZONE_CMA will work like as ZONE_HIGHMEM or ZONE_MOVABLE.
> > >
> > >I don't think it would be a problem because most of file cache pages
> > >and anonymous pages are requested with GFP_HIGHUSER_MOVABLE. It could
> > >be proved by the fact that there are many systems with ZONE_HIGHMEM and
> > >they work fine. Notable disadvantage is that we cannot use these pages
> > >for blockdev file cache page, because it usually has __GFP_MOVABLE but
> > >not __GFP_HIGHMEM and __GFP_USER. But, in this case, there is pros and
> > >cons. In my experience, blockdev file cache pages are one of the top
> > >reason that causes cma_alloc() to fail temporarily. So, we can get more
> > >guarantee of cma_alloc() success by discarding that case.
> > >
> > >Implementation itself is very easy to understand. Steal when cma area is
> > >initialized and recalculate various per zone stat/threshold.
> > >
> > >Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> >
> > ...
> >
> > >@@ -145,6 +145,28 @@ err:
> > > static int __init cma_init_reserved_areas(void)
> > > {
> > > int i;
> > >+ struct zone *zone;
> > >+ unsigned long start_pfn = UINT_MAX, end_pfn = 0;
> > >+
> > >+ if (!cma_area_count)
> > >+ return 0;
> > >+
> > >+ for (i = 0; i < cma_area_count; i++) {
> > >+ if (start_pfn > cma_areas[i].base_pfn)
> > >+ start_pfn = cma_areas[i].base_pfn;
> > >+ if (end_pfn < cma_areas[i].base_pfn + cma_areas[i].count)
> > >+ end_pfn = cma_areas[i].base_pfn + cma_areas[i].count;
> > >+ }
> > >+
> > >+ for_each_zone(zone) {
> > >+ if (!is_zone_cma(zone))
> > >+ continue;
> > >+
> > >+ /* ZONE_CMA doesn't need to exceed CMA region */
> > >+ zone->zone_start_pfn = max(zone->zone_start_pfn, start_pfn);
> > >+ zone->spanned_pages = min(zone_end_pfn(zone), end_pfn) -
> > >+ zone->zone_start_pfn;
> > >+ }
> >
> > Hmm, so what happens on a system with multiple nodes? Each will have
> > its own ZONE_CMA, and all will have the same start pfn and spanned
> > pages?
>
> Each of zone_start_pfn and spanned_pages are initialized in
> calculate_node_totalpages() which considers node boundary. So, they will
> have not the same start pfn and spanned pages. However, each would
> contain unnecessary holes.
>
> >
> > > /* Free whole pageblock and set its migration type to MIGRATE_CMA. */
> > > void __init init_cma_reserved_pageblock(struct page *page)
> > > {
> > > unsigned i = pageblock_nr_pages;
> > >+ unsigned long pfn = page_to_pfn(page);
> > > struct page *p = page;
> > >+ int nid = page_to_nid(page);
> > >+
> > >+ /*
> > >+ * ZONE_CMA will steal present pages from other zones by changing
> > >+ * page links so page_zone() is changed. Before that,
> > >+ * we need to adjust previous zone's page count first.
> > >+ */
> > >+ adjust_present_page_count(page, -pageblock_nr_pages);
> > >
> > > do {
> > > __ClearPageReserved(p);
> > > set_page_count(p, 0);
> > >- } while (++p, --i);
> > >+
> > >+ /* Steal pages from other zones */
> > >+ set_page_links(p, ZONE_CMA, nid, pfn);
> > >+ } while (++p, ++pfn, --i);
> > >+
> > >+ adjust_present_page_count(page, pageblock_nr_pages);
> >
> > This seems to assign pages to ZONE_CMA on the proper node, which is
> > good. But then ZONE_CMA on multiple nodes will have unnecessary
> > holes in the spanned pages, as each will contain only a subset.
>
> True, I will fix it and respin the series.
I now realize that it's too late to send full series for next
merge window. I will send full series after next merge window is closed.
Anyway, I'd like to confirm that following incremental patch will solve
your concern.
Thanks.
------>8--------------
mm/cma.c | 25 ++++++++++++++++---------
1 file changed, 16 insertions(+), 9 deletions(-)
diff --git a/mm/cma.c b/mm/cma.c
index d69bdf7..8375554 100644
--- a/mm/cma.c
+++ b/mm/cma.c
@@ -146,22 +146,29 @@ static int __init cma_init_reserved_areas(void)
{
int i;
struct zone *zone;
- unsigned long start_pfn = UINT_MAX, end_pfn = 0;
+ pg_data_t *pgdat;
if (!cma_area_count)
return 0;
- for (i = 0; i < cma_area_count; i++) {
- if (start_pfn > cma_areas[i].base_pfn)
- start_pfn = cma_areas[i].base_pfn;
- if (end_pfn < cma_areas[i].base_pfn + cma_areas[i].count)
- end_pfn = cma_areas[i].base_pfn + cma_areas[i].count;
- }
+ for_each_online_pgdat(pgdat) {
+ unsigned long start_pfn = UINT_MAX, end_pfn = 0;
- for_each_zone(zone) {
- if (!is_zone_cma(zone))
+ for (i = 0; i < cma_area_count; i++) {
+ if (page_to_nid(pfn_to_page(cma_areas[i].base_pfn)) !=
+ pgdat->node_id)
+ continue;
+
+ start_pfn = min(start_pfn, cma_areas[i].base_pfn);
+ end_pfn = max(end_pfn, cma_areas[i].base_pfn +
+ cma_areas[i].count);
+ }
+
+ if (!end_pfn)
continue;
+ zone = &pgdat->node_zones[ZONE_CMA];
+
/* ZONE_CMA doesn't need to exceed CMA region */
zone->zone_start_pfn = max(zone->zone_start_pfn, start_pfn);
zone->spanned_pages = min(zone_end_pfn(zone), end_pfn) -
next prev parent reply other threads:[~2016-09-22 6:57 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-08-29 5:07 [PATCH v5 0/6] Introduce ZONE_CMA js1304
2016-08-29 5:07 ` [PATCH v5 1/6] mm/page_alloc: don't reserve ZONE_HIGHMEM for ZONE_MOVABLE request js1304
2016-09-16 3:14 ` Aneesh Kumar K.V
2016-09-22 5:30 ` Joonsoo Kim
2016-09-21 9:06 ` Vlastimil Babka
2016-08-29 5:07 ` [PATCH v5 2/6] mm/cma: introduce new zone, ZONE_CMA js1304
2016-08-30 10:35 ` Aneesh Kumar K.V
2016-08-30 12:40 ` Aneesh Kumar K.V
2016-08-31 7:58 ` Joonsoo Kim
2016-09-21 9:11 ` Vlastimil Babka
2016-08-29 5:07 ` [PATCH v5 3/6] mm/cma: populate ZONE_CMA js1304
2016-09-21 9:20 ` Vlastimil Babka
2016-09-22 5:45 ` Joonsoo Kim
2016-09-22 6:50 ` Joonsoo Kim [this message]
2016-09-22 15:59 ` Vlastimil Babka
2016-09-28 5:34 ` Joonsoo Kim
2016-08-29 5:07 ` [PATCH v5 4/6] mm/cma: remove ALLOC_CMA js1304
2016-08-29 5:07 ` [PATCH v5 5/6] mm/cma: remove MIGRATE_CMA js1304
2016-08-29 5:07 ` [PATCH v5 6/6] mm/cma: remove per zone CMA stat js1304
2016-08-29 9:27 ` [PATCH v5 0/6] Introduce ZONE_CMA Aneesh Kumar K.V
2016-08-30 8:21 ` Joonsoo Kim
2016-08-30 10:39 ` Aneesh Kumar K.V
2016-08-31 8:03 ` Joonsoo Kim
2016-09-01 5:47 ` Aneesh Kumar K.V
2016-09-01 6:01 ` Joonsoo Kim
2016-09-21 14:47 ` Aneesh Kumar K.V
2016-09-22 5:32 ` Joonsoo Kim
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20160922065048.GD27958@js1304-P5Q-DELUXE \
--to=iamjoonsoo.kim@lge.com \
--cc=akpm@linux-foundation.org \
--cc=aneesh.kumar@linux.vnet.ibm.com \
--cc=hannes@cmpxchg.org \
--cc=lauraa@codeaurora.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=m.szyprowski@samsung.com \
--cc=mgorman@techsingularity.net \
--cc=mina86@mina86.com \
--cc=minchan@kernel.org \
--cc=riel@redhat.com \
--cc=vbabka@suse.cz \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).