Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.32 as permitted sender) client-ip=23.128.96.32;
MIME-Version: 1.0
References: <20230921062206.14429-1-jaewon31.kim@samsung.com>
 <CABdmKX1V-yPF260gORh-0LFkL+bDfPhOJteiwBjg1v6rYgrvgg@mail.gmail.com>
 <CGME20230921061944epcas1p2f8a022282709da12840b33c0f5898b5f@epcms1p3>
 <20230921122244.1cbf2d57@gandalf.local.home> <20230922022746epcms1p323c50015241eb01311e7e10579fd4c6c@epcms1p3>
In-Reply-To: <20230922022746epcms1p323c50015241eb01311e7e10579fd4c6c@epcms1p3>
From:   "T.J. Mercier" <tjmercier@google.com>
Date:   Sat, 23 Sep 2023 08:26:01 -0700
Message-ID: <CABdmKX310260fNEH2MAS+n-17XGMp3uUrZThxefAAvzb43XG-g@mail.gmail.com>
Subject: Re: [PATCH v2] vmscan: add trace events for lru_gen
To:     jaewon31.kim@samsung.com
Cc:     Steven Rostedt <rostedt@goodmis.org>,
        "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>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Thu, Sep 21, 2023 at 7:27=E2=80=AFPM =EA=B9=80=EC=9E=AC=EC=9B=90 <jaewon=
31.kim@samsung.com> wrote:
>
> >On Thu, 21 Sep 2023 09:12:30 -0700
> >"T.J. Mercier" <tjmercier@google.com> wrote:
> >
> >> > +       TP_fast_assign(
> >> > +               __entry->nid =3D nid;
> >> > +               __entry->nr_reclaimed =3D nr_reclaimed;
> >> > +               __entry->nr_dirty =3D stat->nr_dirty;
> >> > +               __entry->nr_writeback =3D stat->nr_writeback;
> >> > +               __entry->nr_congested =3D stat->nr_congested;
> >> > +               __entry->nr_immediate =3D stat->nr_immediate;
> >> > +               __entry->nr_activate0 =3D stat->nr_activate[0];
> >> > +               __entry->nr_activate1 =3D stat->nr_activate[1];
> >> > +               __entry->nr_ref_keep =3D stat->nr_ref_keep;
> >> > +               __entry->nr_unmap_fail =3D stat->nr_unmap_fail;
> >> > +               __entry->priority =3D priority;
> >> > +               __entry->reclaim_flags =3D trace_reclaim_flags(file)=
;
> >> > +       ),
> >> > +
> >> > +       TP_printk("nid=3D%d nr_reclaimed=3D%ld nr_dirty=3D%ld nr_wri=
teback=3D%ld nr_congested=3D%ld nr_immediate=3D%ld nr_activate_anon=3D%d nr=
_activate_file=3D%d nr_ref_keep=3D%ld nr_unmap_fail=3D%ld priority=3D%d fla=
gs=3D%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 a=
ctually needed.
>
Great, looks good to me.
Reviewed-by: T.J. Mercier <tjmercier@google.com>

> --- 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 tracepoi=
nt.
>          */
> -       TP_printk("isolate_mode=3D%d classzone=3D%d order=3D%d nr_request=
ed=3D%lu nr_scanned=3D%lu nr_skipped=3D%lu nr_taken=3D%lu lru=3D%s",
> +       TP_printk("isolate_mode=3D%u classzone=3D%d order=3D%d nr_request=
ed=3D%lu nr_scanned=3D%lu nr_skipped=3D%lu nr_taken=3D%lu lru=3D%s",
>                 __entry->isolate_mode,
>                 __entry->highest_zoneidx,
>                 __entry->order,
> @@ -525,7 +525,7 @@ TRACE_EVENT(mm_vmscan_lru_gen_evict,
>                 __entry->reclaim_flags =3D trace_reclaim_flags(file);
>         ),
>
> -       TP_printk("nid=3D%d nr_reclaimed=3D%ld nr_dirty=3D%ld nr_writebac=
k=3D%ld nr_congested=3D%ld nr_immediate=3D%ld nr_activate_anon=3D%d nr_acti=
vate_file=3D%d nr_ref_keep=3D%ld nr_unmap_fail=3D%ld priority=3D%d flags=3D=
%s",
> +       TP_printk("nid=3D%d nr_reclaimed=3D%lu nr_dirty=3D%lu nr_writebac=
k=3D%lu nr_congested=3D%lu nr_immediate=3D%lu nr_activate_anon=3D%u nr_acti=
vate_file=3D%u nr_ref_keep=3D%lu nr_unmap_fail=3D%lu priority=3D%d flags=3D=
%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