2020-12-17 06:30:56

by Alex Shi

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

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 <[email protected]>
Cc: Hugh Dickins <[email protected]>
Cc: Johannes Weiner <[email protected]>
Cc: Michal Hocko <[email protected]>
Cc: Vladimir Davydov <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
---
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

2020-12-22 03:40:39

by Hugh Dickins

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

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 <[email protected]>
> Cc: Hugh Dickins <[email protected]>

Acked-by: Hugh Dickins <[email protected]>

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 <[email protected]>
> Cc: Michal Hocko <[email protected]>
> Cc: Vladimir Davydov <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> ---
> 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