[rh7,v2] mm/memcontrol: memcg_move_account() get rid of lock_page dependency.

Submitted by Andrey Ryabinin on March 4, 2020, 12:56 p.m.

Details

Message ID 20200304125608.937-1-aryabinin@virtuozzo.com
State New
Series "mm/memcontrol: memcg_move_account() get rid of lock_page dependency."
Headers show

Commit Message

Andrey Ryabinin March 4, 2020, 12:56 p.m.
Instead of trylock'ing the page, use the lru_lock to prevent the
racing against mem_cgroup_migrate(). So the memcg_move_account() doesn't
depend on hung IO.

The trylock seems to be needed to prevent mem_cgroup_migrate() to see
disbalanced pc->mem_cgroup && pc->flags state. We could achieve the same
by using lru_lock instead. memcg_move_account() only works with isolated
from LRU pages, so it can race only with mem_cgroup_migrate() called
with lrucare=true, otherwise the page is not on LRU so the memcg_move_account()
can't see it.

Also memcg_move_account() might race against try_get_mem_cgroup_from_page()
when mem_cgroup_move_account() called for online cgroup. This happens when
task moved between cgroups and memory.move_charge_at_immigrate set at
certain value. Keep the trylock_page() to protect us from such race just
in case.

For offlined cgroup the race agains try_get_mem_cgroup_from_page()
shouldn't be possible as it will bail out on failed css_tryget().

https://jira.sw.ru/browse/PSBM-101639
https://jira.sw.ru/browse/PSBM-101757
https://jira.sw.ru/browse/PSBM-94117
Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
---
 mm/memcontrol.c | 32 +++++++++++++++++++++-----------
 1 file changed, 21 insertions(+), 11 deletions(-)

Patch hide | download patch | download mbox

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 0514f9b2b230..28ed9c10e6c2 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -3953,13 +3953,18 @@  static int mem_cgroup_move_account(struct page *page,
 	if (nr_pages > 1 && !PageTransHuge(page))
 		goto out;
 
-	/*
-	 * Prevent mem_cgroup_migrate() from looking at pc->mem_cgroup
-	 * of its source page while we change it: page migration takes
-	 * both pages off the LRU, but page cache replacement doesn't.
-	 */
-	if (!trylock_page(page))
-		goto out;
+	if (!from->is_offline) {
+		/* Protect us from racing against try_get_mem_cgroup_from_page() */
+		if (!trylock_page(page))
+			goto out;
+	} else {
+		/*
+		 * Prevent mem_cgroup_migrate() from looking at pc->mem_cgroup
+		 * of its source page while we change it: page migration takes
+		 * both pages off the LRU, but page cache replacement doesn't.
+		 */
+		spin_lock_irq(&page_zone(page)->lru_lock);
+	}
 
 	ret = -EINVAL;
 	if (!PageCgroupUsed(pc) || pc->mem_cgroup != from)
@@ -3992,7 +3997,10 @@  static int mem_cgroup_move_account(struct page *page,
 	memcg_check_events(from, page);
 	local_irq_restore(flags);
 out_unlock:
-	unlock_page(page);
+	if (!from->is_offline)
+		unlock_page(page);
+	else
+		spin_unlock_irq(&page_zone(page)->lru_lock);
 out:
 	return ret;
 }
@@ -7774,6 +7782,7 @@  void mem_cgroup_migrate(struct page *oldpage, struct page *newpage,
 {
 	unsigned int nr_pages = 1;
 	struct page_cgroup *pc;
+	struct mem_cgroup *memcg;
 	int isolated;
 
 	VM_BUG_ON_PAGE(!PageLocked(oldpage), oldpage);
@@ -7807,17 +7816,18 @@  void mem_cgroup_migrate(struct page *oldpage, struct page *newpage,
 	if (lrucare)
 		lock_page_lru(oldpage, &isolated);
 
+	memcg = READ_ONCE(pc->mem_cgroup);
 	pc->flags = 0;
 
 	if (lrucare)
 		unlock_page_lru(oldpage, isolated);
 
 	local_irq_disable();
-	mem_cgroup_charge_statistics(pc->mem_cgroup, oldpage, -nr_pages);
-	memcg_check_events(pc->mem_cgroup, oldpage);
+	mem_cgroup_charge_statistics(memcg, oldpage, -nr_pages);
+	memcg_check_events(memcg, oldpage);
 	local_irq_enable();
 
-	commit_charge(newpage, pc->mem_cgroup, nr_pages, lrucare);
+	commit_charge(newpage, memcg, nr_pages, lrucare);
 }
 
 static int __init cgroup_memory(char *s)

Comments

Vasily Averin March 5, 2020, 5:19 a.m.
On 3/4/20 3:56 PM, Andrey Ryabinin wrote:
> Instead of trylock'ing the page, use the lru_lock to prevent the
> racing against mem_cgroup_migrate(). So the memcg_move_account() doesn't
> depend on hung IO.
> 
> The trylock seems to be needed to prevent mem_cgroup_migrate() to see
> disbalanced pc->mem_cgroup && pc->flags state. We could achieve the same
> by using lru_lock instead. memcg_move_account() only works with isolated
> from LRU pages, so it can race only with mem_cgroup_migrate() called
> with lrucare=true, otherwise the page is not on LRU so the memcg_move_account()
> can't see it.
> 
> Also memcg_move_account() might race against try_get_mem_cgroup_from_page()
> when mem_cgroup_move_account() called for online cgroup. This happens when
> task moved between cgroups and memory.move_charge_at_immigrate set at
> certain value. Keep the trylock_page() to protect us from such race just
> in case.

Can we use trylock_page() in all cases and use "if(is_offline) lru_lock" only if lockpage fails?
 
> For offlined cgroup the race agains try_get_mem_cgroup_from_page()
> shouldn't be possible as it will bail out on failed css_tryget().
> 
> https://jira.sw.ru/browse/PSBM-101639
> https://jira.sw.ru/browse/PSBM-101757
> https://jira.sw.ru/browse/PSBM-94117
> Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
> ---
>  mm/memcontrol.c | 32 +++++++++++++++++++++-----------
>  1 file changed, 21 insertions(+), 11 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 0514f9b2b230..28ed9c10e6c2 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -3953,13 +3953,18 @@ static int mem_cgroup_move_account(struct page *page,
>  	if (nr_pages > 1 && !PageTransHuge(page))
>  		goto out;
>  
> -	/*
> -	 * Prevent mem_cgroup_migrate() from looking at pc->mem_cgroup
> -	 * of its source page while we change it: page migration takes
> -	 * both pages off the LRU, but page cache replacement doesn't.
> -	 */
> -	if (!trylock_page(page))
> -		goto out;
> +	if (!from->is_offline) {
> +		/* Protect us from racing against try_get_mem_cgroup_from_page() */
> +		if (!trylock_page(page))
> +			goto out;
> +	} else {
> +		/*
> +		 * Prevent mem_cgroup_migrate() from looking at pc->mem_cgroup
> +		 * of its source page while we change it: page migration takes
> +		 * both pages off the LRU, but page cache replacement doesn't.
> +		 */
> +		spin_lock_irq(&page_zone(page)->lru_lock);
> +	}
>  
>  	ret = -EINVAL;
>  	if (!PageCgroupUsed(pc) || pc->mem_cgroup != from)
> @@ -3992,7 +3997,10 @@ static int mem_cgroup_move_account(struct page *page,
>  	memcg_check_events(from, page);
>  	local_irq_restore(flags);
>  out_unlock:
> -	unlock_page(page);
> +	if (!from->is_offline)
> +		unlock_page(page);
> +	else
> +		spin_unlock_irq(&page_zone(page)->lru_lock);
>  out:
>  	return ret;
>  }
> @@ -7774,6 +7782,7 @@ void mem_cgroup_migrate(struct page *oldpage, struct page *newpage,
>  {
>  	unsigned int nr_pages = 1;
>  	struct page_cgroup *pc;
> +	struct mem_cgroup *memcg;
>  	int isolated;
>  
>  	VM_BUG_ON_PAGE(!PageLocked(oldpage), oldpage);
> @@ -7807,17 +7816,18 @@ void mem_cgroup_migrate(struct page *oldpage, struct page *newpage,
>  	if (lrucare)
>  		lock_page_lru(oldpage, &isolated);
>  
> +	memcg = READ_ONCE(pc->mem_cgroup);
>  	pc->flags = 0;
>  
>  	if (lrucare)
>  		unlock_page_lru(oldpage, isolated);
>  
>  	local_irq_disable();
> -	mem_cgroup_charge_statistics(pc->mem_cgroup, oldpage, -nr_pages);
> -	memcg_check_events(pc->mem_cgroup, oldpage);
> +	mem_cgroup_charge_statistics(memcg, oldpage, -nr_pages);
> +	memcg_check_events(memcg, oldpage);
>  	local_irq_enable();
>  
> -	commit_charge(newpage, pc->mem_cgroup, nr_pages, lrucare);
> +	commit_charge(newpage, memcg, nr_pages, lrucare);
>  }
>  
>  static int __init cgroup_memory(char *s)
>
Andrey Ryabinin March 5, 2020, 8:54 a.m.
On 3/5/20 8:19 AM, Vasily Averin wrote:
> On 3/4/20 3:56 PM, Andrey Ryabinin wrote:
>> Instead of trylock'ing the page, use the lru_lock to prevent the
>> racing against mem_cgroup_migrate(). So the memcg_move_account() doesn't
>> depend on hung IO.
>>
>> The trylock seems to be needed to prevent mem_cgroup_migrate() to see
>> disbalanced pc->mem_cgroup && pc->flags state. We could achieve the same
>> by using lru_lock instead. memcg_move_account() only works with isolated
>> from LRU pages, so it can race only with mem_cgroup_migrate() called
>> with lrucare=true, otherwise the page is not on LRU so the memcg_move_account()
>> can't see it.
>>
>> Also memcg_move_account() might race against try_get_mem_cgroup_from_page()
>> when mem_cgroup_move_account() called for online cgroup. This happens when
>> task moved between cgroups and memory.move_charge_at_immigrate set at
>> certain value. Keep the trylock_page() to protect us from such race just
>> in case.
> 
> Can we use trylock_page() in all cases and use "if(is_offline) lru_lock" only if lockpage fails?
>  

Seems possible, don't see the benefit of such approach though.