Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) client-ip=23.128.96.37;
MIME-Version: 1.0
References: <20230926051035epcms1p312b531bba2c17bb763b6f046a45c86da@epcms1p3>
 <CAOUHufZwLET1E9dQq15LX27v+20HwWstL4XFi+qzVuz1voLW=w@mail.gmail.com>
 <20230926042250.6028-1-jaewon31.kim@samsung.com> <CGME20230926042019epcas1p11c28533f7b7db99db9f9d8a03ddd332c@epcms1p5>
 <20230926073333epcms1p14c9798232b395007eb20becb5dbc4b4e@epcms1p1> <20230926141519epcms1p5b7808c768df48647516f458529e4e3c8@epcms1p5>
In-Reply-To: <20230926141519epcms1p5b7808c768df48647516f458529e4e3c8@epcms1p5>
From:   Jaewon Kim <jaewon31.kim@gmail.com>
Date:   Mon, 2 Oct 2023 08:41:15 +0900
Message-ID: <CAJrd-Uu16oZncaFzycGoyryxyjEGFWJdecQoShveGV6_GGJLiQ@mail.gmail.com>
Subject: Re: [PATCH v4] vmscan: add trace events for lru_gen
To:     jaewon31.kim@samsung.com
Cc:     Yu Zhao <yuzhao@google.com>,
        "rostedt@goodmis.org" <rostedt@goodmis.org>,
        "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>,
        "sj@kernel.org" <sj@kernel.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>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hello Yu Zhao

Could you give me your comment? I am waiting for your opinion on the
reuse method. I'm planning to resend it as a complete patch with it.
Thank you.


On Tue, Sep 26, 2023 at 11:15=E2=80=AFPM =EA=B9=80=EC=9E=AC=EC=9B=90 <jaewo=
n31.kim@samsung.com> wrote:
>
> >>>On Mon, Sep 25, 2023 at 10:20?PM Jaewon Kim <jaewon31.kim@samsung.com>=
 wrote:
> >>>>
> >>>> As the legacy lru provides, the lru_gen needs some trace events for
> >>>> debugging.
> >>>>
> >>>> This commit introduces 2 trace events.
> >>>>   trace_mm_vmscan_lru_gen_scan
> >>>>   trace_mm_vmscan_lru_gen_evict
> >>>>
> >>>> Each event is similar to the following legacy events.
> >>>>   trace_mm_vmscan_lru_isolate,
> >>>>   trace_mm_vmscan_lru_shrink_[in]active
> >>>
> >>>We should just reuse trace_mm_vmscan_lru_isolate and
> >>>trace_mm_vmscan_lru_shrink_inactive instead of adding new tracepoints.
> >>>
> >>>To reuse trace_mm_vmscan_lru_isolate, we'd just need to append two new
> >>>names to LRU_NAMES.
> >>>
> >>>The naming of trace_mm_vmscan_lru_shrink_inactive might seem confusing
> >>>but it's how MGLRU maintains the compatibility, e.g., the existing
> >>>active/inactive counters in /proc/vmstat.
> >>
> >>
> >>Hello
> >>
> >>Actually I had tried to reuse them. But some value was not that compati=
ble.
> >>Let me try that way again.
> >>
> >>>
> >
> >Hello Yu Zhao
> >
> >Could you look into what I tried below? I reused the legacy trace events=
 as you recommened.
> >
> >For the nr_scanned for trace_mm_vmscan_lru_shrink_inactive, I just used =
the scanned returned from isolate_folios.
> >I thought this is right as scan_folios also uses its isolated.
> >  __count_vm_events(PGSCAN_ANON + type, isolated);
> >But I guess the scanned in scan_folios is actually the one used in shrin=
k_inactive_list
>
> please ignore nr_scanned thing above I just misread the code.
>
> This is an example, I think it works well.
>
>  mm_vmscan_lru_isolate: isolate_mode=3D0 classzone=3D2 order=3D0 nr_reque=
sted=3D4096 nr_scanned=3D64 nr_skipped=3D0 nr_taken=3D64 lru=3Dinactive_fil=
e
>  mm_vmscan_lru_shrink_inactive: nid=3D0 nr_scanned=3D64 nr_reclaimed=3D63=
 nr_dirty=3D0 nr_writeback=3D0 nr_congested=3D0 nr_immediate=3D0 nr_activat=
e_anon=3D0 nr_activate_file=3D1 nr_ref_keep=3D0 nr_unmap_fail=3D0 priority=
=3D2 flags=3DRECLAIM_WB_FILE|RECLAIM_WB_ASYNC
>
> >
> >I tested this on both 0 and 7 of /sys/kernel/mm/lru_gen/enabled
> >
> >
> >diff --git a/mm/vmscan.c b/mm/vmscan.c
> >index a4e44f1c97c1..b61a0156559c 100644
> >--- a/mm/vmscan.c
> >+++ b/mm/vmscan.c
> >@@ -4328,6 +4328,7 @@ static int scan_folios(struct lruvec *lruvec, stru=
ct scan_control *sc,
> >        int sorted =3D 0;
> >        int scanned =3D 0;
> >        int isolated =3D 0;
> >+       int skipped =3D 0;
> >        int remaining =3D MAX_LRU_BATCH;
> >        struct lru_gen_folio *lrugen =3D &lruvec->lrugen;
> >        struct mem_cgroup *memcg =3D lruvec_memcg(lruvec);
> >@@ -4341,7 +4342,7 @@ static int scan_folios(struct lruvec *lruvec, stru=
ct scan_control *sc,
> >
> >        for (i =3D MAX_NR_ZONES; i > 0; i--) {
> >                LIST_HEAD(moved);
> >-               int skipped =3D 0;
> >+               int skipped_zone =3D 0;
> >                int zone =3D (sc->reclaim_idx + i) % MAX_NR_ZONES;
> >                struct list_head *head =3D &lrugen->folios[gen][type][zo=
ne];
> >
> >@@ -4363,16 +4364,17 @@ static int scan_folios(struct lruvec *lruvec, st=
ruct scan_control *sc,
> >                                isolated +=3D delta;
> >                        } else {
> >                                list_move(&folio->lru, &moved);
> >-                               skipped +=3D delta;
> >+                               skipped_zone +=3D delta;
> >                        }
> >
> >-                       if (!--remaining || max(isolated, skipped) >=3D =
MIN_LRU_BATCH)
> >+                       if (!--remaining || max(isolated, skipped_zone) =
>=3D MIN_LRU_BATCH)
> >                                break;
> >                }
> >
> >-               if (skipped) {
> >+               if (skipped_zone) {
> >                        list_splice(&moved, head);
> >-                       __count_zid_vm_events(PGSCAN_SKIP, zone, skipped=
);
> >+                       __count_zid_vm_events(PGSCAN_SKIP, zone, skipped=
_zone);
> >+                       skipped +=3D skipped_zone;
> >                }
> >
> >                if (!remaining || isolated >=3D MIN_LRU_BATCH)
> >@@ -4387,6 +4389,9 @@ static int scan_folios(struct lruvec *lruvec, stru=
ct scan_control *sc,
> >        __count_memcg_events(memcg, item, isolated);
> >        __count_memcg_events(memcg, PGREFILL, sorted);
> >        __count_vm_events(PGSCAN_ANON + type, isolated);
> >+       trace_mm_vmscan_lru_isolate(sc->reclaim_idx, sc->order, MAX_LRU_=
BATCH,
> >+                                   scanned, skipped, isolated,
> >+                                   type ? LRU_INACTIVE_FILE : LRU_INACT=
IVE_ANON);
> >
> >        /*
> >         * There might not be eligible folios due to reclaim_idx. Check =
the
> >@@ -4517,6 +4522,9 @@ static int evict_folios(struct lruvec *lruvec, str=
uct scan_control *sc, int swap
> > retry:
> >        reclaimed =3D shrink_folio_list(&list, pgdat, sc, &stat, false);
> >        sc->nr_reclaimed +=3D reclaimed;
> >+       trace_mm_vmscan_lru_shrink_inactive(pgdat->node_id,
> >+                       scanned, reclaimed, &stat, sc->priority,
> >+                       type ? LRU_INACTIVE_FILE : LRU_INACTIVE_ANON);
> >
> >        list_for_each_entry_safe_reverse(folio, next, &list, lru) {
> >                if (!folio_evictable(folio)) {
> >