Hello Andrew,
On Tue, Nov 05, 2013 at 01:44:48PM -0800, Andrew Morton wrote:
> On Tue, 29 Oct 2013 09:33:23 +0000 Mel Gorman <mgorman@xxxxxxx> wrote:
>
> > On Mon, Oct 28, 2013 at 07:42:49PM +0800, zhang.mingjun@xxxxxxxxxx wrote:
> > > From: Mingjun Zhang <troy.zhangmingjun@xxxxxxxxxx>
> > >
> > > free_contig_range frees cma pages one by one and MIGRATE_CMA pages will be
> > > used as MIGRATE_MOVEABLE pages in the pcp list, it causes unnecessary
> > > migration action when these pages reused by CMA.
> > >
> > > Signed-off-by: Mingjun Zhang <troy.zhangmingjun@xxxxxxxxxx>
> > > ---
> > > mm/page_alloc.c | 3 ++-
> > > 1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > > index 0ee638f..84b9d84 100644
> > > --- a/mm/page_alloc.c
> > > +++ b/mm/page_alloc.c
> > > @@ -1362,7 +1362,8 @@ void free_hot_cold_page(struct page *page, int cold)
> > > * excessively into the page allocator
> > > */
> > > if (migratetype >= MIGRATE_PCPTYPES) {
> > > - if (unlikely(is_migrate_isolate(migratetype))) {
> > > + if (unlikely(is_migrate_isolate(migratetype))
> > > + || is_migrate_cma(migratetype))
> > > free_one_page(zone, page, 0, migratetype);
> > > goto out;
> >
> > This slightly impacts the page allocator free path for a marginal gain
> > on CMA which are relatively rare allocations. There is no obvious
> > benefit to this patch as I expect CMA allocations to flush the PCP lists
> > when a range of pages have been isolated and migrated. Is there any
> > measurable benefit to this patch?
>
> The added overhead is pretty small - just a comparison of a local with
> a constant. And that cost is not incurred for MIGRATE_UNMOVABLE,
> MIGRATE_RECLAIMABLE and MIGRATE_MOVABLE, which are the common cases
> (yes?).
True but bloat code might affect icache so we should be careful.
And what Mel has a concern is about zone->lock, which would be more contended.
I agree his opinion.
In addition, I think the gain is marginal because normally CMA is big range
so free_contig_range in dma release path will fill per_cpu_pages with freed pages
easily so it could drain per_cpu_pages frequently so race which steal page from
per_cpu_pages is not big, I guess.
Morever, we could change free_contig_range with batch_free_page which would
be useful for other cases if they want to free many number of pages
all at once.
The bottom line is we need *number and real scenario* for that.
If it's really needed, after merging this patch, we could enhance it with
batch_free_page so we could solve Mel's concern, too.
>
> This thread is a bit straggly and inconclusive, but it sounds to me
> that the benefit to CMA users is quite large and the cost to others is
> small, so I'm inclined to run with the original patch. Someone stop me
> if that's wrong.
I want you to stop until we see the number.
>
> (we could speed up some of the migratetype tests if the MIGRATE_foo
> constants were converted to bitfields. The above test becomes "if
> (migratetype & (MIGRATE_CMA|MIGRATE_ISOLATE))").
>
> (why is is_migrate_cma() implemented as a macro in mmzone.h while
> is_migrate_isolate() is an inline in page-isolation.h?)
Just preference?
I like inline than macro and that's why is_migrate_isolate was inline.
>
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@xxxxxxxxx. For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>
--
Kind regards,
Minchan Kim
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>