Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230421101415.5734-1-osalvador@suse.de>
In-Reply-To: <20230421101415.5734-1-osalvador@suse.de>
From:   Alexander Potapenko <glider@google.com>
Date:   Fri, 21 Apr 2023 13:19:42 +0200
Message-ID: <CAG_fn=UzFaHrM2X0_X=9aRPe5Wcmzj_snAbY=GJCj8__h9PxCg@mail.gmail.com>
Subject: Re: [PATCH v4 0/3] page_owner: print stacks and their counter
To:     Oscar Salvador <osalvador@suse.de>
Cc:     Andrew Morton <akpm@linux-foundation.org>,
        linux-kernel@vger.kernel.org, linux-mm@kvack.org,
        Michal Hocko <mhocko@suse.com>,
        Vlastimil Babka <vbabka@suse.cz>,
        Eric Dumazet <edumazet@google.com>,
        Waiman Long <longman@redhat.com>,
        Suren Baghdasaryan <surenb@google.com>,
        Marco Elver <elver@google.com>,
        Andrey Konovalov <andreyknvl@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Fri, Apr 21, 2023 at 12:14=E2=80=AFPM Oscar Salvador <osalvador@suse.de>=
 wrote:
>
> Changes v3 -> v4:
>      - Rebase (long time has passed)
>      - Use boolean instead of enum for action by Alexander Potapenko
>      - (I left some feedback untouched because it's been long and
>         would like to discuss it here now instead of re-vamping
>         and old thread)
>
> Changes v2 -> v3:
>      - Replace interface in favor of seq operations (suggested by Vlastim=
il)
>      - Use debugfs interface to store/read valued (suggested by Ammar)
>
> Hi,
>
> page_owner is a great debug functionality tool that gets us to know
> about all pages that have been allocated/freed and their stacktrace.
> This comes very handy when e.g: debugging leaks, as with some scripting
> we might be able to see those stacktraces that are allocating pages
> but not freeing theme.
>
> In my experience, that is one of the most useful cases, but it can get
> really tedious to screen through all pages aand try to reconstruct the
> stack <-> allocated/freed relationship. There is a lot of noise
> to cancel off.
>
> This patch aims to fix that by adding a new functionality into page_owner=
.
> What this does is to create a new read-only file "page_owner_stacks",
> which prints only the allocating stacktraces and their counting, being th=
at
> the times the stacktrace has allocated - the times it has freed.
>
> So we have a clear overview of stacks <-> allocated/freed relationship
> without the need to fiddle with pages and trying to match free stacktrace=
s
> with allocated stacktraces.
>
> This is achieved by adding a new refcount_t field in the stack_record str=
uct,
> incrementing that refcount_t everytime the same stacktrace allocates,
> and decrementing it when it frees a page. Details can be seen in the
> respective patches.

I think the implementation of these counters is too specific to
page_owner and is hard to use for any other purpose.
If we decide to have them, there should be no page_owner-specific
logic in the way we initialize/increment/decrement these counters.
The thresholds in "mm,page_owner: Filter out stacks by a threshold
counter" should also belong elsewhere.

Given that no other stackdepot user needs these counters, maybe it
should be cleaner to store an opaque struct along with the stack,
passing its size to stack_depot_save(), and letting users access it
directly using the stackdepot handler.

I am also wondering if a separate hashtable mapping handlers to
counters would solve the problem for you?