linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] mm/memcg: Fix incorrect flushing of lruvec data in obj_stock
@ 2021-08-02  2:28 Waiman Long
  2021-08-02  6:28 ` Michal Hocko
  2021-08-02 14:32 ` Shakeel Butt
  0 siblings, 2 replies; 4+ 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, cgroups, linux-mm, 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@redhat.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;
 	}
 
 	bytes = (idx == NR_SLAB_RECLAIMABLE_B) ? &stock->nr_slab_reclaimable_b
-- 
2.18.1



^ permalink raw reply related	[flat|nested] 4+ messages in thread
* Re: [PATCH] mm/memcg: Fix incorrect flushing of lruvec data in obj_stock
  2021-08-02  2:28 [PATCH] mm/memcg: Fix incorrect flushing of lruvec data in obj_stock Waiman Long
@ 2021-08-02  6:28 ` Michal Hocko
  2021-08-02 13:32   ` Waiman Long
  2021-08-02 14:32 ` Shakeel Butt
  1 sibling, 1 reply; 4+ 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] 4+ messages in thread
* Re: [PATCH] mm/memcg: Fix incorrect flushing of lruvec data in obj_stock
  2021-08-02  6:28 ` Michal Hocko
@ 2021-08-02 13:32   ` Waiman Long
  0 siblings, 0 replies; 4+ 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] 4+ messages in thread
* Re: [PATCH] mm/memcg: Fix incorrect flushing of lruvec data in obj_stock
  2021-08-02  2:28 [PATCH] mm/memcg: Fix incorrect flushing of lruvec data in obj_stock Waiman Long
  2021-08-02  6:28 ` Michal Hocko
@ 2021-08-02 14:32 ` Shakeel Butt
  1 sibling, 0 replies; 4+ 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] 4+ messages in thread
end of thread, other threads:[~2021-08-02 14:32 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-02  2:28 [PATCH] mm/memcg: Fix incorrect flushing of lruvec data in obj_stock Waiman Long
2021-08-02  6:28 ` Michal Hocko
2021-08-02 13:32   ` Waiman Long
2021-08-02 14:32 ` Shakeel Butt

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).