Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.38 as permitted sender) client-ip=23.128.96.38;
Mime-Version: 1.0
Subject: RE: (2) [PATCH] vmscan: add trace events for lru_gen
Reply-To: jaewon31.kim@samsung.com
Sender: =?UTF-8?B?6rmA7J6s7JuQ?= <jaewon31.kim@samsung.com>
From:   =?UTF-8?B?6rmA7J6s7JuQ?= <jaewon31.kim@samsung.com>
To:     Steven Rostedt <rostedt@goodmis.org>,
        =?UTF-8?B?6rmA7J6s7JuQ?= <jaewon31.kim@samsung.com>
CC:     "yuzhao@google.com" <yuzhao@google.com>,
        "tjmercier@google.com" <tjmercier@google.com>,
        "kaleshsingh@google.com" <kaleshsingh@google.com>,
        "akpm@linux-foundation.org" <akpm@linux-foundation.org>,
        "vbabka@suse.cz" <vbabka@suse.cz>,
        "hannes@cmpxchg.org" <hannes@cmpxchg.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-trace-kernel@vger.kernel.org" 
        <linux-trace-kernel@vger.kernel.org>,
        "linux-mm@kvack.org" <linux-mm@kvack.org>,
        "jaewon31.kim@gmail.com" <jaewon31.kim@gmail.com>
In-Reply-To: <20230920103233.145ac387@gandalf.local.home>
Message-ID: <20230921060915epcms1p6a03b613816b2a99d75ddfd1e8c4a44f7@epcms1p6>
Date:   Thu, 21 Sep 2023 15:09:15 +0900
Content-Transfer-Encoding: 7bit
Content-Type: text/plain; charset="utf-8"
CMS-TYPE: 101P
DLP-Filter: Pass
References: <20230920103233.145ac387@gandalf.local.home>
        <20230919095927.5a964094@gandalf.local.home>
        <20230919025216.1878-1-jaewon31.kim@samsung.com>
        <20230920074948epcms1p82d18c2f4d6a0b5699d50fc419b9ba9fe@epcms1p8>
        <CGME20230919025012epcas1p2c2c57a34eea3299ef3bb643f4cbddf0b@epcms1p6>
Precedence: bulk

>> Great. Thank you for your comment.
>> 
>> For the putting the struct scan_control *sc inside the trace,
>> I couldn't do that because struct scan_control is defined in mm/vmscan.c.
>> I think I should not move it to a seperate header file.
>
>Well if you ever decide to do so, one thing to do is to move the
>trace/events/vmscan.h into mm/ as trace_vmscan.h so that it would have
>access to local header files. Then all you need to do is to move the
>struct scan_control into a local mm/X.h header file.
>
>> 
>> As you may expect, I just made this by copying the existing
>> trace_mm_vmscan_lru_isolate and trace_mm_vmscan_lru_shrink_inactive
>> 
>> I've tried to change like this.
>> Would this be good for you?
>
>The below looks fine to me. Thanks.
>
>-- Steve

Thank you for your comment.
But I still don't know if I can move the strut scan_control into a
new separate header file, mm/X.h. I guess it was meant to be located
in vmscan.c only.

Let me just send v2 patch with only this change, and wait for other
mm guys comments regarding the moving the struct scan_control thing.

Thank you very much.

>
>> 
>> 
>> --- a/include/trace/events/vmscan.h
>> +++ b/include/trace/events/vmscan.h
>> @@ -327,7 +327,7 @@ TRACE_EVENT(mm_vmscan_lru_isolate,
>>                 __print_symbolic(__entry->lru, LRU_NAMES))
>>  );
>>  
>> -TRACE_EVENT(mm_vmscan_lru_gen_scan,
>> +TRACE_EVENT_CONDITION(mm_vmscan_lru_gen_scan,
>>         TP_PROTO(int highest_zoneidx,
>>                 int order,
>>                 unsigned long nr_requested,
>> @@ -339,6 +339,8 @@ TRACE_EVENT(mm_vmscan_lru_gen_scan,
>>  
>>         TP_ARGS(highest_zoneidx, order, nr_requested, nr_scanned, nr_skipped, nr_taken, isolate_mode, lru),
>>  
>> +       TP_CONDITION(nr_scanned),
>> +
>>         TP_STRUCT__entry(
>>                 __field(int, highest_zoneidx)
>>                 __field(int, order)
>> @@ -494,7 +496,6 @@ TRACE_EVENT(mm_vmscan_lru_gen_evict,
>>         TP_ARGS(nid, nr_reclaimed, stat, priority, file),
>>  
>>         TP_STRUCT__entry(
>> -               __field(int, nid)
>>                 __field(unsigned long, nr_reclaimed)
>>                 __field(unsigned long, nr_dirty)
>>                 __field(unsigned long, nr_writeback)
>> @@ -504,6 +505,7 @@ TRACE_EVENT(mm_vmscan_lru_gen_evict,
>>                 __field(unsigned int, nr_activate1)
>>                 __field(unsigned long, nr_ref_keep)
>>                 __field(unsigned long, nr_unmap_fail)
>> +               __field(int, nid)
>>                 __field(int, priority)
>>                 __field(int, reclaim_flags)
>>         ),
>> 
>> --- a/mm/vmscan.c
>> +++ b/mm/vmscan.c
>> @@ -5131,10 +5131,9 @@ static int scan_folios(struct lruvec *lruvec, struct scan_control *sc,
>>         __count_memcg_events(memcg, PGREFILL, sorted);
>>         __count_vm_events(PGSCAN_ANON + type, isolated);
>>  
>> -       if (scanned)
>> -               trace_mm_vmscan_lru_gen_scan(sc->reclaim_idx, sc->order,
>> -                               MAX_LRU_BATCH, scanned, skipped, isolated,
>> -                               sc->may_unmap ? 0 : ISOLATE_UNMAPPED, type);
>> +       trace_mm_vmscan_lru_gen_scan(sc->reclaim_idx, sc->order, MAX_LRU_BATCH,
>> +                       scanned, skipped, isolated,
>> +                       sc->may_unmap ? 0 : ISOLATE_UNMAPPED, type);
>> 
>> 
>> 
>> >