Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) client-ip=2620:137:e000::3:2;
Mime-Version: 1.0
Subject: RE: [PATCH v2] 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>,
        "T.J. Mercier" <tjmercier@google.com>
CC:     =?UTF-8?B?6rmA7J6s7JuQ?= <jaewon31.kim@samsung.com>,
        "yuzhao@google.com" <yuzhao@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: <20230921122244.1cbf2d57@gandalf.local.home>
Message-ID: <20230922022746epcms1p323c50015241eb01311e7e10579fd4c6c@epcms1p3>
Date:   Fri, 22 Sep 2023 11:27:46 +0900
Content-Transfer-Encoding: 7bit
Content-Type: text/plain; charset="utf-8"
CMS-TYPE: 101P
DLP-Filter: Pass
References: <20230921122244.1cbf2d57@gandalf.local.home>
        <20230921062206.14429-1-jaewon31.kim@samsung.com>
        <CABdmKX1V-yPF260gORh-0LFkL+bDfPhOJteiwBjg1v6rYgrvgg@mail.gmail.com>
        <CGME20230921061944epcas1p2f8a022282709da12840b33c0f5898b5f@epcms1p3>
Precedence: bulk

>On Thu, 21 Sep 2023 09:12:30 -0700                                                                                                                                                                               
>"T.J. Mercier" <tjmercier@google.com> wrote:                                                                                                                                                                     
>                                                                                                                                                                                                                 
>> > +       TP_fast_assign(                                                                                                                                                                                      
>> > +               __entry->nid = nid;                                                                                                                                                                          
>> > +               __entry->nr_reclaimed = nr_reclaimed;                                                                                                                                                        
>> > +               __entry->nr_dirty = stat->nr_dirty;                                                                                                                                                          
>> > +               __entry->nr_writeback = stat->nr_writeback;                                                                                                                                                  
>> > +               __entry->nr_congested = stat->nr_congested;                                                                                                                                                  
>> > +               __entry->nr_immediate = stat->nr_immediate;                                                                                                                                                  
>> > +               __entry->nr_activate0 = stat->nr_activate[0];                                                                                                                                                
>> > +               __entry->nr_activate1 = stat->nr_activate[1];                                                                                                                                                
>> > +               __entry->nr_ref_keep = stat->nr_ref_keep;                                                                                                                                                    
>> > +               __entry->nr_unmap_fail = stat->nr_unmap_fail;                                                                                                                                                
>> > +               __entry->priority = priority;                                                                                                                                                                
>> > +               __entry->reclaim_flags = trace_reclaim_flags(file);                                                                                                                                          
>> > +       ),                                                                                                                                                                                                   
>> > +                                                                                                                                                                                                            
>> > +       TP_printk("nid=%d nr_reclaimed=%ld nr_dirty=%ld nr_writeback=%ld nr_congested=%ld nr_immediate=%ld nr_activate_anon=%d nr_activate_file=%d nr_ref_keep=%ld nr_unmap_fail=%ld priority=%d flags=%s",  
>>                                                                                                                                                                                                                
>> Many of these values are unsigned so I think many of these format                                                                                                                                              
>> specifiers should be %lu instead of %ld.                                                                                                                                                                       

Hello T.J.
Thank you for your comment 
As you expected I got this from the legacy lru trace.
I've changed as you recommended.
Actually I changed isolate_mode, too. Please let me know if this is not actually needed.

--- a/include/trace/events/vmscan.h
+++ b/include/trace/events/vmscan.h
@@ -367,7 +367,7 @@ TRACE_EVENT_CONDITION(mm_vmscan_lru_gen_scan,
         * classzone is previous name of the highest_zoneidx.
         * Reason not to change it is the ABI requirement of the tracepoint.
         */
-       TP_printk("isolate_mode=%d classzone=%d order=%d nr_requested=%lu nr_scanned=%lu nr_skipped=%lu nr_taken=%lu lru=%s",
+       TP_printk("isolate_mode=%u classzone=%d order=%d nr_requested=%lu nr_scanned=%lu nr_skipped=%lu nr_taken=%lu lru=%s",
                __entry->isolate_mode,
                __entry->highest_zoneidx,
                __entry->order,
@@ -525,7 +525,7 @@ TRACE_EVENT(mm_vmscan_lru_gen_evict,
                __entry->reclaim_flags = trace_reclaim_flags(file);
        ),
 
-       TP_printk("nid=%d nr_reclaimed=%ld nr_dirty=%ld nr_writeback=%ld nr_congested=%ld nr_immediate=%ld nr_activate_anon=%d nr_activate_file=%d nr_ref_keep=%ld nr_unmap_fail=%ld priority=%d flags=%s",
+       TP_printk("nid=%d nr_reclaimed=%lu nr_dirty=%lu nr_writeback=%lu nr_congested=%lu nr_immediate=%lu nr_activate_anon=%u nr_activate_file=%u nr_ref_keep=%lu nr_unmap_fail=%lu priority=%d flags=%s",
                __entry->nid, __entry->nr_reclaimed,
                __entry->nr_dirty, __entry->nr_writeback,
                __entry->nr_congested, __entry->nr_immediate,

>                                                                                                                                                                                                                 
>Other than this, from the tracing POV:                                                                                                                                                                           
>                                                                                                                                                                                                                 
>Reviewed-by: Steven Rostedt (Google) <rostedt@goodmis.org>                                                                                                                                                       


Hello Steven
I've appreciated your Reviewed-by, let me take this on my next v3 patch.

>                                                                                                                                                                                                                 
>-- Steve