On Thu, 21 Sep 2023 09:12:30 -0700
"T.J. Mercier" <[email protected]> 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.
Other than this, from the tracing POV:
Reviewed-by: Steven Rostedt (Google) <[email protected]>
-- Steve
>On Thu, 21 Sep 2023 09:12:30 -0700
>"T.J. Mercier" <[email protected]> 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) <[email protected]>
Hello Steven
I've appreciated your Reviewed-by, let me take this on my next v3 patch.
>
>-- Steve
On Thu, Sep 21, 2023 at 7:27 PM 김재원 <[email protected]> wrote:
>
> >On Thu, 21 Sep 2023 09:12:30 -0700
> >"T.J. Mercier" <[email protected]> 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.
>
Great, looks good to me.
Reviewed-by: T.J. Mercier <[email protected]>
> --- 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) <[email protected]>
>
>
> Hello Steven
> I've appreciated your Reviewed-by, let me take this on my next v3 patch.
>
> >
> >-- Steve