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

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+.