| Message ID | 20210729125755.16871-6-linmiaohe@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Cleanups and fixup for memcontrol | expand |
On Thu, Jul 29, 2021 at 5:58 AM Miaohe Lin <linmiaohe@huawei.com> wrote: > > We should always ensure __mod_node_page_state() is called with preempt > disabled or percpu ops may manipulate the wrong cpu when preempt happened. > > Fixes: b4e0b68fbd9d ("mm: memcontrol: use obj_cgroup APIs to charge kmem pages") > Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> > --- > mm/memcontrol.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index 70a32174e7c4..616d1a72ece3 100644 > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -697,8 +697,8 @@ void __mod_lruvec_page_state(struct page *page, enum node_stat_item idx, > memcg = page_memcg(head); > /* Untracked pages have no memcg, no lruvec. Update only the node */ > if (!memcg) { > - rcu_read_unlock(); > __mod_node_page_state(pgdat, idx, val); > + rcu_read_unlock(); This rcu is for page_memcg. The preemption and interrupts are disabled across __mod_lruvec_page_state(). > return; > } > > -- > 2.23.0 >
On 2021/7/29 22:39, Shakeel Butt wrote: > On Thu, Jul 29, 2021 at 5:58 AM Miaohe Lin <linmiaohe@huawei.com> wrote: >> >> We should always ensure __mod_node_page_state() is called with preempt >> disabled or percpu ops may manipulate the wrong cpu when preempt happened. >> >> Fixes: b4e0b68fbd9d ("mm: memcontrol: use obj_cgroup APIs to charge kmem pages") >> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> >> --- >> mm/memcontrol.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/mm/memcontrol.c b/mm/memcontrol.c >> index 70a32174e7c4..616d1a72ece3 100644 >> --- a/mm/memcontrol.c >> +++ b/mm/memcontrol.c >> @@ -697,8 +697,8 @@ void __mod_lruvec_page_state(struct page *page, enum node_stat_item idx, >> memcg = page_memcg(head); >> /* Untracked pages have no memcg, no lruvec. Update only the node */ >> if (!memcg) { >> - rcu_read_unlock(); >> __mod_node_page_state(pgdat, idx, val); >> + rcu_read_unlock(); > > This rcu is for page_memcg. The preemption and interrupts are disabled > across __mod_lruvec_page_state(). > I thought it's used to protect __mod_node_page_state(). Looks somewhat confusing for me. Many thanks for pointing this out! >> return; >> } >> >> -- >> 2.23.0 >> > . >
On Fri, Jul 30, 2021 at 9:52 AM Miaohe Lin <linmiaohe@huawei.com> wrote: > > On 2021/7/29 22:39, Shakeel Butt wrote: > > On Thu, Jul 29, 2021 at 5:58 AM Miaohe Lin <linmiaohe@huawei.com> wrote: > >> > >> We should always ensure __mod_node_page_state() is called with preempt > >> disabled or percpu ops may manipulate the wrong cpu when preempt happened. > >> > >> Fixes: b4e0b68fbd9d ("mm: memcontrol: use obj_cgroup APIs to charge kmem pages") > >> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> > >> --- > >> mm/memcontrol.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/mm/memcontrol.c b/mm/memcontrol.c > >> index 70a32174e7c4..616d1a72ece3 100644 > >> --- a/mm/memcontrol.c > >> +++ b/mm/memcontrol.c > >> @@ -697,8 +697,8 @@ void __mod_lruvec_page_state(struct page *page, enum node_stat_item idx, > >> memcg = page_memcg(head); > >> /* Untracked pages have no memcg, no lruvec. Update only the node */ > >> if (!memcg) { > >> - rcu_read_unlock(); > >> __mod_node_page_state(pgdat, idx, val); > >> + rcu_read_unlock(); > > > > This rcu is for page_memcg. The preemption and interrupts are disabled > > across __mod_lruvec_page_state(). > > > > I thought it's used to protect __mod_node_page_state(). Looks somewhat confusing for me. > Many thanks for pointing this out! Hi Miaohe, git show b4e0b68fbd9d can help you find out why we add the rcu read lock around it. Thanks. > > >> return; > >> } > >> > >> -- > >> 2.23.0 > >> > > . > > >
On 2021/7/30 10:33, Muchun Song wrote: > On Fri, Jul 30, 2021 at 9:52 AM Miaohe Lin <linmiaohe@huawei.com> wrote: >> >> On 2021/7/29 22:39, Shakeel Butt wrote: >>> On Thu, Jul 29, 2021 at 5:58 AM Miaohe Lin <linmiaohe@huawei.com> wrote: >>>> >>>> We should always ensure __mod_node_page_state() is called with preempt >>>> disabled or percpu ops may manipulate the wrong cpu when preempt happened. >>>> >>>> Fixes: b4e0b68fbd9d ("mm: memcontrol: use obj_cgroup APIs to charge kmem pages") >>>> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> >>>> --- >>>> mm/memcontrol.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c >>>> index 70a32174e7c4..616d1a72ece3 100644 >>>> --- a/mm/memcontrol.c >>>> +++ b/mm/memcontrol.c >>>> @@ -697,8 +697,8 @@ void __mod_lruvec_page_state(struct page *page, enum node_stat_item idx, >>>> memcg = page_memcg(head); >>>> /* Untracked pages have no memcg, no lruvec. Update only the node */ >>>> if (!memcg) { >>>> - rcu_read_unlock(); >>>> __mod_node_page_state(pgdat, idx, val); >>>> + rcu_read_unlock(); >>> >>> This rcu is for page_memcg. The preemption and interrupts are disabled >>> across __mod_lruvec_page_state(). >>> >> >> I thought it's used to protect __mod_node_page_state(). Looks somewhat confusing for me. >> Many thanks for pointing this out! > > Hi Miaohe, > > git show b4e0b68fbd9d can help you find out why we add > the rcu read lock around it. Thanks for your tip. That's my overlook when I checked this commit. I should have looked at this more closely. :( > > Thanks. > >> >>>> return; >>>> } >>>> >>>> -- >>>> 2.23.0 >>>> >>> . >>> >> > . >
diff --git a/mm/memcontrol.c b/mm/memcontrol.c index 70a32174e7c4..616d1a72ece3 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -697,8 +697,8 @@ void __mod_lruvec_page_state(struct page *page, enum node_stat_item idx, memcg = page_memcg(head); /* Untracked pages have no memcg, no lruvec. Update only the node */ if (!memcg) { - rcu_read_unlock(); __mod_node_page_state(pgdat, idx, val); + rcu_read_unlock(); return; }
We should always ensure __mod_node_page_state() is called with preempt disabled or percpu ops may manipulate the wrong cpu when preempt happened. Fixes: b4e0b68fbd9d ("mm: memcontrol: use obj_cgroup APIs to charge kmem pages") Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> --- mm/memcontrol.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)