* [PATCH] mm/memcg: Fix incorrect flushing of lruvec data in obj_stock
@ 2021-08-02 2:28 ` Waiman Long
0 siblings, 0 replies; 9+ messages in thread
From: Waiman Long @ 2021-08-02 2:28 UTC (permalink / raw)
To: Johannes Weiner, Michal Hocko, Vladimir Davydov, Andrew Morton,
Tejun Heo, Christoph Lameter, Pekka Enberg, David Rientjes,
Joonsoo Kim, Vlastimil Babka, Roman Gushchin
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
cgroups-u79uwXL29TY76Z2rM5mHXA, linux-mm-Bw31MaZKKs3YtjvyW6yDsg,
Shakeel Butt, Muchun Song, Alex Shi, Chris Down, Yafang Shao,
Wei Yang, Masayoshi Mizuma, Xing Zhengjun, Matthew Wilcox,
Waiman Long
When mod_objcg_state() is called with a pgdat that is different from
that in the obj_stock, the old lruvec data cached in obj_stock are
flushed out. Unfortunately, they were flushed to the new pgdat and
hence the wrong node, not the one cached in obj_stock.
Fix that by flushing the data to the cached pgdat instead.
Fixes: 68ac5b3c8db2 ("mm/memcg: cache vmstat data in percpu memcg_stock_pcp")
Signed-off-by: Waiman Long <longman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
mm/memcontrol.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index ae1f5d0cb581..881ec4ddddcd 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -3106,17 +3106,19 @@ void mod_objcg_state(struct obj_cgroup *objcg, struct pglist_data *pgdat,
stock->cached_pgdat = pgdat;
} else if (stock->cached_pgdat != pgdat) {
/* Flush the existing cached vmstat data */
+ struct pglist_data *oldpg = stock->cached_pgdat;
+
+ stock->cached_pgdat = pgdat;
if (stock->nr_slab_reclaimable_b) {
- mod_objcg_mlstate(objcg, pgdat, NR_SLAB_RECLAIMABLE_B,
+ mod_objcg_mlstate(objcg, oldpg, NR_SLAB_RECLAIMABLE_B,
stock->nr_slab_reclaimable_b);
stock->nr_slab_reclaimable_b = 0;
}
if (stock->nr_slab_unreclaimable_b) {
- mod_objcg_mlstate(objcg, pgdat, NR_SLAB_UNRECLAIMABLE_B,
+ mod_objcg_mlstate(objcg, oldpg, NR_SLAB_UNRECLAIMABLE_B,
stock->nr_slab_unreclaimable_b);
stock->nr_slab_unreclaimable_b = 0;
}
- stock->cached_pgdat = pgdat;
}
bytes = (idx == NR_SLAB_RECLAIMABLE_B) ? &stock->nr_slab_reclaimable_b
--
2.18.1
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH] mm/memcg: Fix incorrect flushing of lruvec data in obj_stock
@ 2021-08-02 6:28 ` Michal Hocko
0 siblings, 0 replies; 9+ messages in thread
From: Michal Hocko @ 2021-08-02 6:28 UTC (permalink / raw)
To: Waiman Long
Cc: Johannes Weiner, Vladimir Davydov, Andrew Morton, Tejun Heo,
Christoph Lameter, Pekka Enberg, David Rientjes, Joonsoo Kim,
Vlastimil Babka, Roman Gushchin, linux-kernel, cgroups, linux-mm,
Shakeel Butt, Muchun Song, Alex Shi, Chris Down, Yafang Shao,
Wei Yang, Masayoshi Mizuma, Xing Zhengjun, Matthew Wilcox
On Sun 01-08-21 22:28:27, Waiman Long wrote:
> When mod_objcg_state() is called with a pgdat that is different from
> that in the obj_stock, the old lruvec data cached in obj_stock are
> flushed out. Unfortunately, they were flushed to the new pgdat and
> hence the wrong node, not the one cached in obj_stock.
It would be great to explicitly mention user observable problems here. I
do assume this will make slab stats skewed but the effect wouldn't be
very big, right?
> Fix that by flushing the data to the cached pgdat instead.
>
> Fixes: 68ac5b3c8db2 ("mm/memcg: cache vmstat data in percpu memcg_stock_pcp")
> Signed-off-by: Waiman Long <longman@redhat.com>
Acked-by: Michal Hocko <mhocko@suse.com>
> ---
> mm/memcontrol.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index ae1f5d0cb581..881ec4ddddcd 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -3106,17 +3106,19 @@ void mod_objcg_state(struct obj_cgroup *objcg, struct pglist_data *pgdat,
> stock->cached_pgdat = pgdat;
> } else if (stock->cached_pgdat != pgdat) {
> /* Flush the existing cached vmstat data */
> + struct pglist_data *oldpg = stock->cached_pgdat;
> +
> + stock->cached_pgdat = pgdat;
> if (stock->nr_slab_reclaimable_b) {
> - mod_objcg_mlstate(objcg, pgdat, NR_SLAB_RECLAIMABLE_B,
> + mod_objcg_mlstate(objcg, oldpg, NR_SLAB_RECLAIMABLE_B,
> stock->nr_slab_reclaimable_b);
> stock->nr_slab_reclaimable_b = 0;
> }
> if (stock->nr_slab_unreclaimable_b) {
> - mod_objcg_mlstate(objcg, pgdat, NR_SLAB_UNRECLAIMABLE_B,
> + mod_objcg_mlstate(objcg, oldpg, NR_SLAB_UNRECLAIMABLE_B,
> stock->nr_slab_unreclaimable_b);
> stock->nr_slab_unreclaimable_b = 0;
> }
> - stock->cached_pgdat = pgdat;
Minor nit. Is there any reason to move the cached_pgdat? TBH I found the
original way better from the readability POV.
> }
>
> bytes = (idx == NR_SLAB_RECLAIMABLE_B) ? &stock->nr_slab_reclaimable_b
> --
> 2.18.1
--
Michal Hocko
SUSE Labs
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] mm/memcg: Fix incorrect flushing of lruvec data in obj_stock
@ 2021-08-02 6:28 ` Michal Hocko
0 siblings, 0 replies; 9+ messages in thread
From: Michal Hocko @ 2021-08-02 6:28 UTC (permalink / raw)
To: Waiman Long
Cc: Johannes Weiner, Vladimir Davydov, Andrew Morton, Tejun Heo,
Christoph Lameter, Pekka Enberg, David Rientjes, Joonsoo Kim,
Vlastimil Babka, Roman Gushchin,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
cgroups-u79uwXL29TY76Z2rM5mHXA, linux-mm-Bw31MaZKKs3YtjvyW6yDsg,
Shakeel Butt, Muchun Song, Alex Shi, Chris Down, Yafang Shao,
Wei Yang, Masayoshi Mizuma, Xing Zhengjun, Matthew
On Sun 01-08-21 22:28:27, Waiman Long wrote:
> When mod_objcg_state() is called with a pgdat that is different from
> that in the obj_stock, the old lruvec data cached in obj_stock are
> flushed out. Unfortunately, they were flushed to the new pgdat and
> hence the wrong node, not the one cached in obj_stock.
It would be great to explicitly mention user observable problems here. I
do assume this will make slab stats skewed but the effect wouldn't be
very big, right?
> Fix that by flushing the data to the cached pgdat instead.
>
> Fixes: 68ac5b3c8db2 ("mm/memcg: cache vmstat data in percpu memcg_stock_pcp")
> Signed-off-by: Waiman Long <longman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Acked-by: Michal Hocko <mhocko-IBi9RG/b67k@public.gmane.org>
> ---
> mm/memcontrol.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index ae1f5d0cb581..881ec4ddddcd 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -3106,17 +3106,19 @@ void mod_objcg_state(struct obj_cgroup *objcg, struct pglist_data *pgdat,
> stock->cached_pgdat = pgdat;
> } else if (stock->cached_pgdat != pgdat) {
> /* Flush the existing cached vmstat data */
> + struct pglist_data *oldpg = stock->cached_pgdat;
> +
> + stock->cached_pgdat = pgdat;
> if (stock->nr_slab_reclaimable_b) {
> - mod_objcg_mlstate(objcg, pgdat, NR_SLAB_RECLAIMABLE_B,
> + mod_objcg_mlstate(objcg, oldpg, NR_SLAB_RECLAIMABLE_B,
> stock->nr_slab_reclaimable_b);
> stock->nr_slab_reclaimable_b = 0;
> }
> if (stock->nr_slab_unreclaimable_b) {
> - mod_objcg_mlstate(objcg, pgdat, NR_SLAB_UNRECLAIMABLE_B,
> + mod_objcg_mlstate(objcg, oldpg, NR_SLAB_UNRECLAIMABLE_B,
> stock->nr_slab_unreclaimable_b);
> stock->nr_slab_unreclaimable_b = 0;
> }
> - stock->cached_pgdat = pgdat;
Minor nit. Is there any reason to move the cached_pgdat? TBH I found the
original way better from the readability POV.
> }
>
> bytes = (idx == NR_SLAB_RECLAIMABLE_B) ? &stock->nr_slab_reclaimable_b
> --
> 2.18.1
--
Michal Hocko
SUSE Labs
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] mm/memcg: Fix incorrect flushing of lruvec data in obj_stock
@ 2021-08-02 13:32 ` Waiman Long
0 siblings, 0 replies; 9+ messages in thread
From: Waiman Long @ 2021-08-02 13:32 UTC (permalink / raw)
To: Michal Hocko
Cc: Johannes Weiner, Vladimir Davydov, Andrew Morton, Tejun Heo,
Christoph Lameter, Pekka Enberg, David Rientjes, Joonsoo Kim,
Vlastimil Babka, Roman Gushchin, linux-kernel, cgroups, linux-mm,
Shakeel Butt, Muchun Song, Alex Shi, Chris Down, Yafang Shao,
Wei Yang, Masayoshi Mizuma, Xing Zhengjun, Matthew Wilcox
On 8/2/21 2:28 AM, Michal Hocko wrote:
> On Sun 01-08-21 22:28:27, Waiman Long wrote:
>> When mod_objcg_state() is called with a pgdat that is different from
>> that in the obj_stock, the old lruvec data cached in obj_stock are
>> flushed out. Unfortunately, they were flushed to the new pgdat and
>> hence the wrong node, not the one cached in obj_stock.
> It would be great to explicitly mention user observable problems here. I
> do assume this will make slab stats skewed but the effect wouldn't be
> very big, right?
It is the /sys/devices/system/node/node*/meminfo that will get skewed.
Not /proc/meminfo. So it is a relatively minor issue. Will update the
patch to mention that.
>> Fix that by flushing the data to the cached pgdat instead.
>>
>> Fixes: 68ac5b3c8db2 ("mm/memcg: cache vmstat data in percpu memcg_stock_pcp")
>> Signed-off-by: Waiman Long <longman@redhat.com>
> Acked-by: Michal Hocko <mhocko@suse.com>
>
>> ---
>> mm/memcontrol.c | 8 +++++---
>> 1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
>> index ae1f5d0cb581..881ec4ddddcd 100644
>> --- a/mm/memcontrol.c
>> +++ b/mm/memcontrol.c
>> @@ -3106,17 +3106,19 @@ void mod_objcg_state(struct obj_cgroup *objcg, struct pglist_data *pgdat,
>> stock->cached_pgdat = pgdat;
>> } else if (stock->cached_pgdat != pgdat) {
>> /* Flush the existing cached vmstat data */
>> + struct pglist_data *oldpg = stock->cached_pgdat;
>> +
>> + stock->cached_pgdat = pgdat;
>> if (stock->nr_slab_reclaimable_b) {
>> - mod_objcg_mlstate(objcg, pgdat, NR_SLAB_RECLAIMABLE_B,
>> + mod_objcg_mlstate(objcg, oldpg, NR_SLAB_RECLAIMABLE_B,
>> stock->nr_slab_reclaimable_b);
>> stock->nr_slab_reclaimable_b = 0;
>> }
>> if (stock->nr_slab_unreclaimable_b) {
>> - mod_objcg_mlstate(objcg, pgdat, NR_SLAB_UNRECLAIMABLE_B,
>> + mod_objcg_mlstate(objcg, oldpg, NR_SLAB_UNRECLAIMABLE_B,
>> stock->nr_slab_unreclaimable_b);
>> stock->nr_slab_unreclaimable_b = 0;
>> }
>> - stock->cached_pgdat = pgdat;
> Minor nit. Is there any reason to move the cached_pgdat? TBH I found the
> original way better from the readability POV.
Right. Will move it back to its original place.
Cheers,
Longman
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] mm/memcg: Fix incorrect flushing of lruvec data in obj_stock
@ 2021-08-02 13:32 ` Waiman Long
0 siblings, 0 replies; 9+ messages in thread
From: Waiman Long @ 2021-08-02 13:32 UTC (permalink / raw)
To: Michal Hocko
Cc: Johannes Weiner, Vladimir Davydov, Andrew Morton, Tejun Heo,
Christoph Lameter, Pekka Enberg, David Rientjes, Joonsoo Kim,
Vlastimil Babka, Roman Gushchin,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
cgroups-u79uwXL29TY76Z2rM5mHXA, linux-mm-Bw31MaZKKs3YtjvyW6yDsg,
Shakeel Butt, Muchun Song, Alex Shi, Chris Down, Yafang Shao,
Wei Yang, Masayoshi Mizuma, Xing Zhengjun, Matthew
On 8/2/21 2:28 AM, Michal Hocko wrote:
> On Sun 01-08-21 22:28:27, Waiman Long wrote:
>> When mod_objcg_state() is called with a pgdat that is different from
>> that in the obj_stock, the old lruvec data cached in obj_stock are
>> flushed out. Unfortunately, they were flushed to the new pgdat and
>> hence the wrong node, not the one cached in obj_stock.
> It would be great to explicitly mention user observable problems here. I
> do assume this will make slab stats skewed but the effect wouldn't be
> very big, right?
It is the /sys/devices/system/node/node*/meminfo that will get skewed.
Not /proc/meminfo. So it is a relatively minor issue. Will update the
patch to mention that.
>> Fix that by flushing the data to the cached pgdat instead.
>>
>> Fixes: 68ac5b3c8db2 ("mm/memcg: cache vmstat data in percpu memcg_stock_pcp")
>> Signed-off-by: Waiman Long <longman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Acked-by: Michal Hocko <mhocko-IBi9RG/b67k@public.gmane.org>
>
>> ---
>> mm/memcontrol.c | 8 +++++---
>> 1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
>> index ae1f5d0cb581..881ec4ddddcd 100644
>> --- a/mm/memcontrol.c
>> +++ b/mm/memcontrol.c
>> @@ -3106,17 +3106,19 @@ void mod_objcg_state(struct obj_cgroup *objcg, struct pglist_data *pgdat,
>> stock->cached_pgdat = pgdat;
>> } else if (stock->cached_pgdat != pgdat) {
>> /* Flush the existing cached vmstat data */
>> + struct pglist_data *oldpg = stock->cached_pgdat;
>> +
>> + stock->cached_pgdat = pgdat;
>> if (stock->nr_slab_reclaimable_b) {
>> - mod_objcg_mlstate(objcg, pgdat, NR_SLAB_RECLAIMABLE_B,
>> + mod_objcg_mlstate(objcg, oldpg, NR_SLAB_RECLAIMABLE_B,
>> stock->nr_slab_reclaimable_b);
>> stock->nr_slab_reclaimable_b = 0;
>> }
>> if (stock->nr_slab_unreclaimable_b) {
>> - mod_objcg_mlstate(objcg, pgdat, NR_SLAB_UNRECLAIMABLE_B,
>> + mod_objcg_mlstate(objcg, oldpg, NR_SLAB_UNRECLAIMABLE_B,
>> stock->nr_slab_unreclaimable_b);
>> stock->nr_slab_unreclaimable_b = 0;
>> }
>> - stock->cached_pgdat = pgdat;
> Minor nit. Is there any reason to move the cached_pgdat? TBH I found the
> original way better from the readability POV.
Right. Will move it back to its original place.
Cheers,
Longman
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] mm/memcg: Fix incorrect flushing of lruvec data in obj_stock
2021-08-02 2:28 ` Waiman Long
(?)
@ 2021-08-02 14:32 ` Shakeel Butt
-1 siblings, 0 replies; 9+ messages in thread
From: Shakeel Butt @ 2021-08-02 14:32 UTC (permalink / raw)
To: Waiman Long
Cc: Johannes Weiner, Michal Hocko, Vladimir Davydov, Andrew Morton,
Tejun Heo, Christoph Lameter, Pekka Enberg, David Rientjes,
Joonsoo Kim, Vlastimil Babka, Roman Gushchin, LKML, Cgroups,
Linux MM, Muchun Song, Alex Shi, Chris Down, Yafang Shao,
Wei Yang, Masayoshi Mizuma, Xing Zhengjun, Matthew Wilcox
On Sun, Aug 1, 2021 at 7:28 PM Waiman Long <longman@redhat.com> wrote:
>
> When mod_objcg_state() is called with a pgdat that is different from
> that in the obj_stock, the old lruvec data cached in obj_stock are
> flushed out. Unfortunately, they were flushed to the new pgdat and
> hence the wrong node, not the one cached in obj_stock.
>
> Fix that by flushing the data to the cached pgdat instead.
>
> Fixes: 68ac5b3c8db2 ("mm/memcg: cache vmstat data in percpu memcg_stock_pcp")
> Signed-off-by: Waiman Long <longman@redhat.com>
After incorporating Michal's comments, you can add:
Reviewed-by: Shakeel Butt <shakeelb@google.com>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] mm/memcg: Fix incorrect flushing of lruvec data in obj_stock
@ 2021-08-02 14:32 ` Shakeel Butt
0 siblings, 0 replies; 9+ messages in thread
From: Shakeel Butt @ 2021-08-02 14:32 UTC (permalink / raw)
To: Waiman Long
Cc: Johannes Weiner, Michal Hocko, Vladimir Davydov, Andrew Morton,
Tejun Heo, Christoph Lameter, Pekka Enberg, David Rientjes,
Joonsoo Kim, Vlastimil Babka, Roman Gushchin, LKML, Cgroups,
Linux MM, Muchun Song, Alex Shi, Chris Down, Yafang Shao,
Wei Yang, Masayoshi Mizuma, Xing Zhengjun
On Sun, Aug 1, 2021 at 7:28 PM Waiman Long <longman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>
> When mod_objcg_state() is called with a pgdat that is different from
> that in the obj_stock, the old lruvec data cached in obj_stock are
> flushed out. Unfortunately, they were flushed to the new pgdat and
> hence the wrong node, not the one cached in obj_stock.
>
> Fix that by flushing the data to the cached pgdat instead.
>
> Fixes: 68ac5b3c8db2 ("mm/memcg: cache vmstat data in percpu memcg_stock_pcp")
> Signed-off-by: Waiman Long <longman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
After incorporating Michal's comments, you can add:
Reviewed-by: Shakeel Butt <shakeelb-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] mm/memcg: Fix incorrect flushing of lruvec data in obj_stock
@ 2021-08-02 14:32 ` Shakeel Butt
0 siblings, 0 replies; 9+ messages in thread
From: Shakeel Butt @ 2021-08-02 14:32 UTC (permalink / raw)
To: Waiman Long
Cc: Johannes Weiner, Michal Hocko, Vladimir Davydov, Andrew Morton,
Tejun Heo, Christoph Lameter, Pekka Enberg, David Rientjes,
Joonsoo Kim, Vlastimil Babka, Roman Gushchin, LKML, Cgroups,
Linux MM, Muchun Song, Alex Shi, Chris Down, Yafang Shao,
Wei Yang, Masayoshi Mizuma, Xing Zhengjun, Matthew Wilcox
On Sun, Aug 1, 2021 at 7:28 PM Waiman Long <longman@redhat.com> wrote:
>
> When mod_objcg_state() is called with a pgdat that is different from
> that in the obj_stock, the old lruvec data cached in obj_stock are
> flushed out. Unfortunately, they were flushed to the new pgdat and
> hence the wrong node, not the one cached in obj_stock.
>
> Fix that by flushing the data to the cached pgdat instead.
>
> Fixes: 68ac5b3c8db2 ("mm/memcg: cache vmstat data in percpu memcg_stock_pcp")
> Signed-off-by: Waiman Long <longman@redhat.com>
After incorporating Michal's comments, you can add:
Reviewed-by: Shakeel Butt <shakeelb@google.com>
^ permalink raw reply [flat|nested] 9+ messages in thread