2019-11-16 03:17:14

by Alex Shi

[permalink] [raw]
Subject: [PATCH v3 2/7] mm/lruvec: add irqsave flags into lruvec struct

We need a irqflags vaiable to save state when do irqsave action, declare
it here would make code more clear/clean.

Rong Chen <[email protected]> reported the 'irqflags' variable need
move to the tail of lruvec struct otherwise it causes 18% regressions of
vm-scalability testing on his machine. So add the flags and lru_lock to
both near struct tail, even I have no clue of this perf losing.

Originally-from: Hugh Dickins <[email protected]>
Signed-off-by: Alex Shi <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Vlastimil Babka <[email protected]>
Cc: Dan Williams <[email protected]>
Cc: Michal Hocko <[email protected]>
Cc: Mel Gorman <[email protected]>
Cc: Wei Yang <[email protected]>
Cc: Johannes Weiner <[email protected]>
Cc: Arun KS <[email protected]>
Cc: Tejun Heo <[email protected]>
Cc: Konstantin Khlebnikov <[email protected]>
CC: Rong Chen <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
---
include/linux/mmzone.h | 2 ++
1 file changed, 2 insertions(+)

diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
index a13b8a602ee5..9b8b8daf4e03 100644
--- a/include/linux/mmzone.h
+++ b/include/linux/mmzone.h
@@ -269,6 +269,8 @@ struct lruvec {
unsigned long flags;
/* per lruvec lru_lock for memcg */
spinlock_t lru_lock;
+ /* flags for irqsave */
+ unsigned long irqflags;
#ifdef CONFIG_MEMCG
struct pglist_data *pgdat;
#endif
--
1.8.3.1


2019-11-16 06:34:03

by Shakeel Butt

[permalink] [raw]
Subject: Re: [PATCH v3 2/7] mm/lruvec: add irqsave flags into lruvec struct

On Fri, Nov 15, 2019 at 7:15 PM Alex Shi <[email protected]> wrote:
>
> We need a irqflags vaiable to save state when do irqsave action, declare
> it here would make code more clear/clean.
>
> Rong Chen <[email protected]> reported the 'irqflags' variable need
> move to the tail of lruvec struct otherwise it causes 18% regressions of
> vm-scalability testing on his machine. So add the flags and lru_lock to
> both near struct tail, even I have no clue of this perf losing.

Regressions compared to what? Also no need to have a separate patch.

>
> Originally-from: Hugh Dickins <[email protected]>
> Signed-off-by: Alex Shi <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Cc: Vlastimil Babka <[email protected]>
> Cc: Dan Williams <[email protected]>
> Cc: Michal Hocko <[email protected]>
> Cc: Mel Gorman <[email protected]>
> Cc: Wei Yang <[email protected]>
> Cc: Johannes Weiner <[email protected]>
> Cc: Arun KS <[email protected]>
> Cc: Tejun Heo <[email protected]>
> Cc: Konstantin Khlebnikov <[email protected]>
> CC: Rong Chen <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> ---
> include/linux/mmzone.h | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
> index a13b8a602ee5..9b8b8daf4e03 100644
> --- a/include/linux/mmzone.h
> +++ b/include/linux/mmzone.h
> @@ -269,6 +269,8 @@ struct lruvec {
> unsigned long flags;
> /* per lruvec lru_lock for memcg */
> spinlock_t lru_lock;
> + /* flags for irqsave */
> + unsigned long irqflags;
> #ifdef CONFIG_MEMCG
> struct pglist_data *pgdat;
> #endif
> --
> 1.8.3.1
>

2019-11-18 02:56:32

by Alex Shi

[permalink] [raw]
Subject: Re: [PATCH v3 2/7] mm/lruvec: add irqsave flags into lruvec struct



在 2019/11/16 下午2:31, Shakeel Butt 写道:
> On Fri, Nov 15, 2019 at 7:15 PM Alex Shi <[email protected]> wrote:
>> We need a irqflags vaiable to save state when do irqsave action, declare
>> it here would make code more clear/clean.
>>
>> Rong Chen <[email protected]> reported the 'irqflags' variable need
>> move to the tail of lruvec struct otherwise it causes 18% regressions of
>> vm-scalability testing on his machine. So add the flags and lru_lock to
>> both near struct tail, even I have no clue of this perf losing.
> Regressions compared to what? Also no need to have a separate patch.
>

Thanks for question, Shakeel,
I will change the commit log and mention the original place: the head of lruvec struct.
As to the folding, the 3rd is already toooo long. May I leave it here to relief a littel bit burden on reading?

Thanks
Alex

2019-11-22 06:56:14

by Christoph Hellwig

[permalink] [raw]
Subject: Re: [PATCH v3 2/7] mm/lruvec: add irqsave flags into lruvec struct

On Sat, Nov 16, 2019 at 11:15:01AM +0800, Alex Shi wrote:
> We need a irqflags vaiable to save state when do irqsave action, declare
> it here would make code more clear/clean.

This patch is wrong on multiple levels. Adding a field without the
users is completely pointless. And as a general pattern we never
add the irqsave flags to sturctures. They are an intimate part of the
calling conventions, and storing them in a structure will lead to
subtile bugs.