From: Vlastimil Babka <email@example.com>
To: Andrey Konovalov <firstname.lastname@example.org>,
Oscar Salvador <email@example.com>
Cc: Andrew Morton <firstname.lastname@example.org>,
Linux Memory Management List <email@example.com>,
Michal Hocko <firstname.lastname@example.org>,
Eric Dumazet <email@example.com>,
Waiman Long <firstname.lastname@example.org>,
Suren Baghdasaryan <email@example.com>,
Marco Elver <firstname.lastname@example.org>,
Alexander Potapenko <email@example.com>
Subject: Re: [PATCH v2 1/3] lib/stackdepot: Add a refcount field in stack_record
Date: Mon, 19 Sep 2022 17:01:06 +0200 [thread overview]
Message-ID: <firstname.lastname@example.org> (raw)
On 9/11/22 00:33, Andrey Konovalov wrote:
>> We cannot do that for __reset_page_owner(), because the stack we are
>> saving is the freeing stacktrace, and we are not interested in that.
>> That is why __reset_page_owner() does:
>> depot_stack_handle_t alloc_handle;
>> alloc_handle = page_owner->handle;
>> handle = save_stack(GFP_NOWAIT | __GFP_NOWARN, STACK_DEPOT_ACTION_NONE);
>> page_owner->free_handle = handle
>> alloc_handle contains the handle for the allocation stacktrace, which was set
>> in __set_page_owner(), and page_owner->free handle contains the handle for the
>> freeing stacktrace.
>> But we are only interested in the allocation stack and we only want to increment/decrement
>> that on allocation/free.
> But what is the problem with incrementing the refcount for free
> stacktrace in __reset_page_owner? You save the stack there anyway.
> Or is this because you don't want to see free stack traces when
> filtering for bigger refcounts? I would argue that this is not a thing
> stack depot should care about. If there's a refcount, it should
> reflect the true number of references.
But to keep this really a "true number" we would have to now decrement the
counter when the page gets freed by a different stack trace, i.e. we replace
the previously stored free_handle with another one. Which is possible, but
seems like a waste of CPU?
> Perhaps, what you need is some kind of per-stack trace user metadata.
> And the details of what format this metadata takes shouldn't be
> present in the stack depot code.
I was thinking ideally we might want fully separate stackdepot storages
per-stack trace user, i.e. separate hashmaps and dynamically allocated
handles instead of the static "slabs" array. Different users tend to have
distinct stacks so that would make lookups more effective too. Only
CONFIG_STACKDEPOT_ALWAYS_INIT users (KASAN) would keep using the static
array due to their inherent limitations wrt dynamic allocations.
Then these different stackdepot instances could be optionally refcounted or
not, and if need be, have additional metadata as you suggest.
>> > Could you split out the stack depot and the page_owner changes into
>> > separate patches?
>> I could, I am not sure if it would make the review any easier though,
>> as you could not match stackdepot <-> page_owner changes.
>> And we should be adding a bunch of code that would not be used till later on.
>> But I can try it out if there is a strong opinion.
> Yes, splitting would be quite useful. It allows backporting stack
> depot changes without having to backport any page_owner code. As long
> as there are not breaking interface changes, of course.
next prev parent reply other threads:[~2022-09-19 15:01 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-09-05 3:10 [PATCH v2 0/3] page_owner: print stacks and their counter Oscar Salvador
2022-09-05 3:10 ` [PATCH v2 1/3] lib/stackdepot: Add a refcount field in stack_record Oscar Salvador
2022-09-05 20:57 ` Andrey Konovalov
2022-09-06 3:54 ` Oscar Salvador
2022-09-10 22:33 ` Andrey Konovalov
2022-09-19 15:01 ` Vlastimil Babka [this message]
2022-09-05 3:10 ` [PATCH v2 2/3] mm, page_owner: Add page_owner_stacks file to print out only stacks and their counter Oscar Salvador
2022-09-05 12:57 ` Marco Elver
2022-09-05 13:00 ` Marco Elver
2022-09-06 7:43 ` Oscar Salvador
2022-09-06 8:35 ` Marco Elver
2022-09-07 4:00 ` Oscar Salvador
2022-09-07 7:14 ` Marco Elver
2022-09-08 3:32 ` Oscar Salvador
2022-09-08 5:31 ` Marco Elver
2022-09-05 22:20 ` kernel test robot
2022-09-05 3:10 ` [PATCH v2 3/3] mm,page_owner: Filter out stacks by a threshold counter Oscar Salvador
2022-09-05 10:51 ` Ammar Faizi
2022-09-05 11:31 ` Michal Hocko
2022-09-05 11:54 ` Ammar Faizi
2022-09-05 12:02 ` Michal Hocko
2022-09-05 12:42 ` Ammar Faizi
2022-09-19 15:23 ` Vlastimil Babka
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.