On Thu, Jan 12, 2017 at 06:02:38PM +0100, Vlastimil Babka wrote:
> > Signed-off-by: Mel Gorman <mgor...@techsingularity.net>
> > Acked-by: Hillf Danton <hillf...@alibaba-inc.com>
> > Acked-by: Jesper Dangaard Brouer <bro...@redhat.com>
> 
> 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

Reply via email to