Re: [Patch v3] mm: thp: grab the lock before manipulation defer list

[David Rientjes <rientjes@google.com>]

On Fri, 17 Jan 2020, Kirill A. Shutemov wrote:

> > Right, and I don't think that it necessarily is and the second 
> > conditional in Wei's patch will always succeed unless we have raced.  That 
> > patch is for a lock concern but I think Kirill's question has uncovered 
> > something more interesting.
> > 
> > Kirill S would definitely be best to answer Kirill T's question, but from 
> > my understanding when mem_cgroup_move_account() is called with 
> > compound == true that we always have an intact pmd (we never migrate 
> > partial page charges for pages on the deferred split queue with the 
> > current charge migration implementation) and thus the underlying page is 
> > not eligible to be split and shouldn't be on the deferred split queue.
> > 
> > In other words, a page being on the deferred split queue for a memcg 
> > should only happen when it is charged to that memcg.  (This wasn't the 
> > case when we only had per-node split queues.)  I think that's currently 
> > broken in mem_cgroup_move_account() before Wei's patch.
> 
> Right. It's broken indeed.
> 
> We are dealing with anon page here. And it cannot be on deferred list as
> long as it's mapped with PMD. We cannot get compound == true &&
> !list_empty() on the (first) enter to the function. Any PMD-mapped page
> will be put onto deferred by the function. This is wrong.
> 
> The fix is not obvious.
> 
> This comment got in mem_cgroup_move_charge_pte_range() my attention:
> 
> 			/*
> 			 * We can have a part of the split pmd here. Moving it
> 			 * can be done but it would be too convoluted so simply
> 			 * ignore such a partial THP and keep it in original
> 			 * memcg. There should be somebody mapping the head.
> 			 */
> 
> That's exactly the case we care about: PTE-mapped THP that has to be split
> under load. We don't move charge of them between memcgs and therefore we
> should not move the page to different memcg.
> 
> I guess this will do the trick :P
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index c5b5f74cfd4d..e87ee4c10f6e 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -5359,14 +5359,6 @@ static int mem_cgroup_move_account(struct page *page,
>  		__mod_lruvec_state(to_vec, NR_WRITEBACK, nr_pages);
>  	}
>  
> -#ifdef CONFIG_TRANSPARENT_HUGEPAGE
> -	if (compound && !list_empty(page_deferred_list(page))) {
> -		spin_lock(&from->deferred_split_queue.split_queue_lock);
> -		list_del_init(page_deferred_list(page));
> -		from->deferred_split_queue.split_queue_len--;
> -		spin_unlock(&from->deferred_split_queue.split_queue_lock);
> -	}
> -#endif
>  	/*
>  	 * It is safe to change page->mem_cgroup here because the page
>  	 * is referenced, charged, and isolated - we can't race with
> @@ -5376,16 +5368,6 @@ static int mem_cgroup_move_account(struct page *page,
>  	/* caller should have done css_get */
>  	page->mem_cgroup = to;
>  
> -#ifdef CONFIG_TRANSPARENT_HUGEPAGE
> -	if (compound && list_empty(page_deferred_list(page))) {
> -		spin_lock(&to->deferred_split_queue.split_queue_lock);
> -		list_add_tail(page_deferred_list(page),
> -			      &to->deferred_split_queue.split_queue);
> -		to->deferred_split_queue.split_queue_len++;
> -		spin_unlock(&to->deferred_split_queue.split_queue_lock);
> -	}
> -#endif
> -
>  	spin_unlock_irqrestore(&from->move_lock, flags);
>  
>  	ret = 0;

Yeah, this is what I was thinking as well.  When 
PageTransHuge(page) == true and there's a mapping pmd, the charge gets 
moved but the page shouldn't appear on any deferred split queue; when 
there isn't a mapped pmd, it should already be on a queue but the charge 
doesn't get moved so no change in which queue is needed.

There was no deferred split handling in mem_cgroup_move_account() needed 
for per-node deferred split queues either so this is purely an issue for 
commit 87eaceb3faa5 ("mm: thp: make deferred split shrinker memcg aware") 
so I think we need your patch and it should be annotated for stable 5.4+.