Re: [PATCH 3/3] mm, page_allocator: Only use per-cpu allocator for irq-safe requests

[Mel Gorman]

On Thu, Jan 12, 2017 at 06:02:38PM +0100, Vlastimil Babka wrote:
> > Signed-off-by: Mel Gorman <mgorman@xxxxxxxxxxxxxxxxxxx>
> > Acked-by: Hillf Danton <hillf.zj@xxxxxxxxxxxxxxx>
> > Acked-by: Jesper Dangaard Brouer <brouer@xxxxxxxxxx>
> 
> Very promising! But I have some worries. Should we put something like
> VM_BUG_ON(in_interrupt()) into free_hot_cold_page() and rmqueue_pcplist() to
> catch future potential misuses and also document this requirement? Also
> free_hot_cold_page() has other call sites besides __free_pages() and I'm not
> sure if those are all guaranteed to be !IRQ? E.g. free_hot_cold_page_list()
> which is called by release_page() which uses irq-safe lock operations...
> 

They are not guaranteed to be !irq but the API is easier to call incorrectly
than it could be. I think the checks can be pushed further down without
excessive overhead.

> Smaller nit below:
> 
> > @@ -2453,8 +2450,8 @@ void free_hot_cold_page(struct page *page, bool cold)
> > 
> >  	migratetype = get_pfnblock_migratetype(page, pfn);
> >  	set_pcppage_migratetype(page, migratetype);
> > -	local_irq_save(flags);
> > -	__count_vm_event(PGFREE);
> > +	preempt_disable();
> > +	count_vm_event(PGFREE);
> 
> AFAICS preempt_disable() is enough for using __count_vm_event(), no?
> 

It is, I'll fix it.

Thanks.

-- 
Mel Gorman
SUSE Labs