[PATCH 1/3] mm/memcg: revise the using condition of lock_page_lruvec function series

[PATCH 1/3] mm/memcg: revise the using condition of lock_page_lruvec function series

[Alex Shi]

The series function could be used under lock_page_memcg(), add this and
a bit style changes following commit_charge().

Signed-off-by: Alex Shi <alex.shi-KPsoFbNs7GizrGE5bRqYAgC/G2K4zDHf@public.gmane.org>
Cc: Hugh Dickins <hughd-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
Cc: Johannes Weiner <hannes-druUgvl0LCNAfugRpC6u6w@public.gmane.org>
Cc: Michal Hocko <mhocko-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Vladimir Davydov <vdavydov.dev-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
Cc: cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: linux-mm-Bw31MaZKKs3YtjvyW6yDsg@public.gmane.org
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
---
 mm/memcontrol.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index b80328f52fb4..e6b50d068b2f 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -1345,10 +1345,11 @@ void lruvec_memcg_debug(struct lruvec *lruvec, struct page *page)
  * lock_page_lruvec - lock and return lruvec for a given page.
  * @page: the page
  *
- * This series functions should be used in either conditions:
- * PageLRU is cleared or unset
- * or page->_refcount is zero
- * or page is locked.
+ * This series functions should be used in any one of following conditions:
+ * - PageLRU is cleared or unset
+ * - page->_refcount is zero
+ * - page is locked.
+ * - lock_page_memcg()
  */
 struct lruvec *lock_page_lruvec(struct page *page)
 {
-- 
2.29.GIT

[PATCH 2/3] mm/memcg: remove rcu locking for lock_page_lruvec function series

[Alex Shi]

The rcu_read_lock was used to block memcg destory, but with the detailed
calling conditions, the memcg won't gone since the page is hold. So we
don't need it now, let's remove them to save locking load in debugging.

Signed-off-by: Alex Shi <alex.shi@linux.alibaba.com>
Cc: Hugh Dickins <hughd@google.com>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Michal Hocko <mhocko@kernel.org>
Cc: Vladimir Davydov <vdavydov.dev@gmail.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: cgroups@vger.kernel.org
Cc: linux-mm@kvack.org
Cc: linux-kernel@vger.kernel.org
---
 mm/memcontrol.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index e6b50d068b2f..98bbee1d2faf 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -1356,10 +1356,8 @@ struct lruvec *lock_page_lruvec(struct page *page)
 	struct lruvec *lruvec;
 	struct pglist_data *pgdat = page_pgdat(page);
 
-	rcu_read_lock();
 	lruvec = mem_cgroup_page_lruvec(page, pgdat);
 	spin_lock(&lruvec->lru_lock);
-	rcu_read_unlock();
 
 	lruvec_memcg_debug(lruvec, page);
 
@@ -1371,10 +1369,8 @@ struct lruvec *lock_page_lruvec_irq(struct page *page)
 	struct lruvec *lruvec;
 	struct pglist_data *pgdat = page_pgdat(page);
 
-	rcu_read_lock();
 	lruvec = mem_cgroup_page_lruvec(page, pgdat);
 	spin_lock_irq(&lruvec->lru_lock);
-	rcu_read_unlock();
 
 	lruvec_memcg_debug(lruvec, page);
 
@@ -1386,10 +1382,8 @@ struct lruvec *lock_page_lruvec_irqsave(struct page *page, unsigned long *flags)
 	struct lruvec *lruvec;
 	struct pglist_data *pgdat = page_pgdat(page);
 
-	rcu_read_lock();
 	lruvec = mem_cgroup_page_lruvec(page, pgdat);
 	spin_lock_irqsave(&lruvec->lru_lock, *flags);
-	rcu_read_unlock();
 
 	lruvec_memcg_debug(lruvec, page);
 
-- 
2.29.GIT

Re: [PATCH 1/3] mm/memcg: revise the using condition of lock_page_lruvec function series

[Hugh Dickins]

On Thu, 17 Dec 2020, Alex Shi wrote:

> The series function could be used under lock_page_memcg(), add this and
> a bit style changes following commit_charge().
> 
> Signed-off-by: Alex Shi <alex.shi@linux.alibaba.com>
> Cc: Hugh Dickins <hughd@google.com>

This patch, or its intention,
Acked-by: Hugh Dickins <hughd@google.com>
but rewording suggested below, and requested above -
which left me very puzzled before eventually I understood it.
I don't think we need to talk about "a bit style changes",
but the cross-reference to commit_charge() is helpful.

"
lock_page_lruvec() and its variants are safe to use under the same
conditions as commit_charge(): add lock_page_memcg() to the comment.
"

> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Cc: Michal Hocko <mhocko@kernel.org>
> Cc: Vladimir Davydov <vdavydov.dev@gmail.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: cgroups@vger.kernel.org
> Cc: linux-mm@kvack.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  mm/memcontrol.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index b80328f52fb4..e6b50d068b2f 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -1345,10 +1345,11 @@ void lruvec_memcg_debug(struct lruvec *lruvec, struct page *page)
>   * lock_page_lruvec - lock and return lruvec for a given page.
>   * @page: the page
>   *
> - * This series functions should be used in either conditions:
> - * PageLRU is cleared or unset
> - * or page->_refcount is zero
> - * or page is locked.
> + * This series functions should be used in any one of following conditions:

These functions are safe to use under any of the following conditions:

> + * - PageLRU is cleared or unset
> + * - page->_refcount is zero
> + * - page is locked.

Remove that full stop...

> + * - lock_page_memcg()

... and, if you wish (I don't care), add full stop at the end of that line.

Maybe reorder those to the same order as listed in commit_charge().
Copy its text exactly? I don't think so, actually, I find your wording
(e.g. _refcount is zero) more explicit: good to have both descriptions.

>   */
>  struct lruvec *lock_page_lruvec(struct page *page)
>  {
> -- 
> 2.29.GIT

Re: [PATCH 2/3] mm/memcg: remove rcu locking for lock_page_lruvec function series

[Hugh Dickins]

On Thu, 17 Dec 2020, Alex Shi wrote:

> The rcu_read_lock was used to block memcg destory, but with the detailed
> calling conditions, the memcg won't gone since the page is hold. So we
> don't need it now, let's remove them to save locking load in debugging.

"
lock_page_lruvec() and its variants used rcu_read_lock() with the
intention of safeguarding against the mem_cgroup being destroyed
concurrently; but so long as they are called under the specified
conditions (as they are), there is no way for the page's mem_cgroup
to be destroyed.  Delete the unnecessary rcu_read_lock() and _unlock().
"

This has little to do with a "locking load in debugging" - so what?
But everything to do with deleting bogosity, the sooner the better.

> 
> Signed-off-by: Alex Shi <alex.shi@linux.alibaba.com>
> Cc: Hugh Dickins <hughd@google.com>

Acked-by: Hugh Dickins <hughd@google.com>

This really surprised me!  Nice change, but how on earth did we not
notice until now?  The rcu_read_lock() seems to have come in, without
explanation, somewhere between lru_lock v9 and v11 (I never saw v10); and
I guess I was so used to needing rcu_read_lock() in my own implementation,
that I was blind to its irrelevance in yours.  Cc'ing Alex Duyck, since
he was generally very alert to this kind of thing - be good to have his
Ack too.  Also Cc'ing Hui Su, who sent a similar but unexplained patch
just before yours.

> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Cc: Michal Hocko <mhocko@kernel.org>
> Cc: Vladimir Davydov <vdavydov.dev@gmail.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: cgroups@vger.kernel.org
> Cc: linux-mm@kvack.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  mm/memcontrol.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index e6b50d068b2f..98bbee1d2faf 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -1356,10 +1356,8 @@ struct lruvec *lock_page_lruvec(struct page *page)
>  	struct lruvec *lruvec;
>  	struct pglist_data *pgdat = page_pgdat(page);
>  
> -	rcu_read_lock();
>  	lruvec = mem_cgroup_page_lruvec(page, pgdat);
>  	spin_lock(&lruvec->lru_lock);
> -	rcu_read_unlock();
>  
>  	lruvec_memcg_debug(lruvec, page);
>  
> @@ -1371,10 +1369,8 @@ struct lruvec *lock_page_lruvec_irq(struct page *page)
>  	struct lruvec *lruvec;
>  	struct pglist_data *pgdat = page_pgdat(page);
>  
> -	rcu_read_lock();
>  	lruvec = mem_cgroup_page_lruvec(page, pgdat);
>  	spin_lock_irq(&lruvec->lru_lock);
> -	rcu_read_unlock();
>  
>  	lruvec_memcg_debug(lruvec, page);
>  
> @@ -1386,10 +1382,8 @@ struct lruvec *lock_page_lruvec_irqsave(struct page *page, unsigned long *flags)
>  	struct lruvec *lruvec;
>  	struct pglist_data *pgdat = page_pgdat(page);
>  
> -	rcu_read_lock();
>  	lruvec = mem_cgroup_page_lruvec(page, pgdat);
>  	spin_lock_irqsave(&lruvec->lru_lock, *flags);
> -	rcu_read_unlock();
>  
>  	lruvec_memcg_debug(lruvec, page);
>  
> -- 
> 2.29.GIT

Re: [PATCH 1/3] mm/memcg: revise the using condition of lock_page_lruvec function series

[Alex Shi]

ÔÚ 2020/12/22 ÉÏÎç11:01, Hugh Dickins дµÀ:
> On Thu, 17 Dec 2020, Alex Shi wrote:
> 
>> The series function could be used under lock_page_memcg(), add this and
>> a bit style changes following commit_charge().
>>
>> Signed-off-by: Alex Shi <alex.shi@linux.alibaba.com>
>> Cc: Hugh Dickins <hughd@google.com>
> 
> This patch, or its intention,
> Acked-by: Hugh Dickins <hughd@google.com>
> but rewording suggested below, and requested above -
> which left me very puzzled before eventually I understood it.
> I don't think we need to talk about "a bit style changes",
> but the cross-reference to commit_charge() is helpful.
> 
> "
> lock_page_lruvec() and its variants are safe to use under the same
> conditions as commit_charge(): add lock_page_memcg() to the comment.
> "

Thanks a lot, Hugh. Yes, your commit log are far more better than mine. :)

I will resent with your changes and Ack.

Thanks!
Alex

> 
>> Cc: Johannes Weiner <hannes@cmpxchg.org>
>> Cc: Michal Hocko <mhocko@kernel.org>
>> Cc: Vladimir Davydov <vdavydov.dev@gmail.com>
>> Cc: Andrew Morton <akpm@linux-foundation.org>
>> Cc: cgroups@vger.kernel.org
>> Cc: linux-mm@kvack.org
>> Cc: linux-kernel@vger.kernel.org
>> ---
>>  mm/memcontrol.c | 9 +++++----
>>  1 file changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
>> index b80328f52fb4..e6b50d068b2f 100644
>> --- a/mm/memcontrol.c
>> +++ b/mm/memcontrol.c
>> @@ -1345,10 +1345,11 @@ void lruvec_memcg_debug(struct lruvec *lruvec, struct page *page)
>>   * lock_page_lruvec - lock and return lruvec for a given page.
>>   * @page: the page
>>   *
>> - * This series functions should be used in either conditions:
>> - * PageLRU is cleared or unset
>> - * or page->_refcount is zero
>> - * or page is locked.
>> + * This series functions should be used in any one of following conditions:
> 
> These functions are safe to use under any of the following conditions:
> 
>> + * - PageLRU is cleared or unset
>> + * - page->_refcount is zero
>> + * - page is locked.
> 
> Remove that full stop...
> 
>> + * - lock_page_memcg()
> 
> ... and, if you wish (I don't care), add full stop at the end of that line.
> 
> Maybe reorder those to the same order as listed in commit_charge().
> Copy its text exactly? I don't think so, actually, I find your wording
> (e.g. _refcount is zero) more explicit: good to have both descriptions.
> 
>>   */
>>  struct lruvec *lock_page_lruvec(struct page *page)
>>  {
>> -- 
>> 2.29.GIT