[v1,01/14] include/linux/memcontrol.h: do not warn in page_memcg_rcu() if !CONFIG_MEMCG

Commit Message

Yu Zhao March 13, 2021, 7:57 a.m. UTC

We want to make sure the rcu lock is held while using
page_memcg_rcu(). But having a WARN_ON_ONCE() in page_memcg_rcu() when
!CONFIG_MEMCG is superfluous because of the following legit use case:

  memcg = lock_page_memcg(page1)
    (rcu_read_lock() if CONFIG_MEMCG=y)

  do something to page1

  if (page_memcg_rcu(page2) == memcg)
    do something to page2 too as it cannot be migrated away from the
    memcg either.

  unlock_page_memcg(page1)
    (rcu_read_unlock() if CONFIG_MEMCG=y)

This patch removes the WARN_ON_ONCE() from page_memcg_rcu() for the
!CONFIG_MEMCG case.

Signed-off-by: Yu Zhao <[e-mail]>
---
 include/linux/memcontrol.h | 1 -
 1 file changed, 1 deletion(-)

Comments

Matthew Wilcox March 13, 2021, 3:09 p.m. UTC | #1

On Sat, Mar 13, 2021 at 12:57:34AM -0700, Yu Zhao wrote:
> We want to make sure the rcu lock is held while using
> page_memcg_rcu(). But having a WARN_ON_ONCE() in page_memcg_rcu() when
> !CONFIG_MEMCG is superfluous because of the following legit use case:
> 
>   memcg = lock_page_memcg(page1)
>     (rcu_read_lock() if CONFIG_MEMCG=y)
> 
>   do something to page1
> 
>   if (page_memcg_rcu(page2) == memcg)
>     do something to page2 too as it cannot be migrated away from the
>     memcg either.
> 
>   unlock_page_memcg(page1)
>     (rcu_read_unlock() if CONFIG_MEMCG=y)
> 
> This patch removes the WARN_ON_ONCE() from page_memcg_rcu() for the
> !CONFIG_MEMCG case.

I think this is wrong.  Usually we try to have the same locking
environment no matter what the CONFIG options are, like with
kmap_atomic().  I think lock_page_memcg() should disable RCU even if
CONFIG_MEMCG=n.

Yu Zhao March 14, 2021, 7:45 a.m. UTC | #2

On Sat, Mar 13, 2021 at 03:09:18PM +0000, Matthew Wilcox wrote:
> On Sat, Mar 13, 2021 at 12:57:34AM -0700, Yu Zhao wrote:
> > We want to make sure the rcu lock is held while using
> > page_memcg_rcu(). But having a WARN_ON_ONCE() in page_memcg_rcu() when
> > !CONFIG_MEMCG is superfluous because of the following legit use case:
> > 
> >   memcg = lock_page_memcg(page1)
> >     (rcu_read_lock() if CONFIG_MEMCG=y)
> > 
> >   do something to page1
> > 
> >   if (page_memcg_rcu(page2) == memcg)
> >     do something to page2 too as it cannot be migrated away from the
> >     memcg either.
> > 
> >   unlock_page_memcg(page1)
> >     (rcu_read_unlock() if CONFIG_MEMCG=y)
> > 
> > This patch removes the WARN_ON_ONCE() from page_memcg_rcu() for the
> > !CONFIG_MEMCG case.
> 
> I think this is wrong.  Usually we try to have the same locking
> environment no matter what the CONFIG options are, like with
> kmap_atomic().  I think lock_page_memcg() should disable RCU even if
> CONFIG_MEMCG=n.

I agree in principle. On this topic I often debate myself where to
draw the line between being rigorous and paranoid. But in this
particular case, I thought it's no brainer because, imo, most of the
systems that don't use memcgs are small and preemptable, e.g.,
openwrt. They wouldn't appreciate a larger code size or rcu stalls due
to preemptions of functions that take rcu locks just to be rigorous.

This shouldn't be a problem if we only do so when CONFIG_DEBUG_VM=y,
but then its test coverage is another question. I'd be happy to work
out something in this direction, hopefully worth the trouble, if you
think this compromise is acceptable.

Patch

diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
index e6dc793d587d..f325aeb4b4e8 100644
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -1079,7 +1079,6 @@  static inline struct mem_cgroup *page_memcg(struct page *page)
 
 static inline struct mem_cgroup *page_memcg_rcu(struct page *page)
 {
-	WARN_ON_ONCE(!rcu_read_lock_held());
 	return NULL;
 }