On Thu, Aug 22, 2019 at 02:56:56PM +0200, Vlastimil Babka wrote:
> >> @@ -2816,11 +2821,14 @@ void free_transhuge_page(struct page *page)
> >> ds_queue->split_queue_len--;
> >> list_del(page_deferred_list(page));
> >> }
> >> + __mod_node_page_state(page_pgdat(page), NR_KERNEL_MISC_RECLAIMABLE,
> >> + -page[2].nr_freeable);
> >> + page[2].nr_freeable = 0;
>
> Wouldn't it be safer to fully tie the nr_freeable use to adding the page to the
> deffered list? So here the code would be in the if (!list_empty()) { } part above.
>
> >> spin_unlock_irqrestore(&ds_queue->split_queue_lock, flags);
> >> free_compound_page(page);
> >> }
> >>
> >> -void deferred_split_huge_page(struct page *page)
> >> +void deferred_split_huge_page(struct page *page, unsigned int nr)
> >> {
> >> struct deferred_split *ds_queue = get_deferred_split_queue(page);
> >> #ifdef CONFIG_MEMCG
> >> @@ -2844,6 +2852,9 @@ void deferred_split_huge_page(struct page *page)
> >> return;
> >>
> >> spin_lock_irqsave(&ds_queue->split_queue_lock, flags);
> >> + page[2].nr_freeable += nr;
> >> + __mod_node_page_state(page_pgdat(page), NR_KERNEL_MISC_RECLAIMABLE,
> >> + nr);
>
> Same here, only do this when adding to the list, below? Or we might perhaps
> account base pages multiple times?
No, it cannot be under list_empty() check. Consider the case when a THP
got unmapped 4k a time. You need to put it on the list once, but account
it every time.
--
Kirill A. Shutemov
