Date:   Fri, 14 Jan 2022 14:31:43 +0100
From:   David Hildenbrand <david@...hat.com>
To:     Minchan Kim <minchan@...nel.org>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        Michal Hocko <mhocko@...e.com>, linux-mm <linux-mm@...ck.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Suren Baghdasaryan <surenb@...gle.com>,
        John Dias <joaodias@...gle.com>, huww98@...look.com,
        John Hubbard <jhubbard@...dia.com>
Subject: Re: [RFC v2] mm: introduce page pin owner

On 12.01.22 21:41, Minchan Kim wrote:
> On Wed, Jan 12, 2022 at 06:42:21PM +0100, David Hildenbrand wrote:
>>>>
>>>> What about something like:
>>>>
>>>> "mm: selective tracing of page reference holders on unref"
>>>>
>>>> PAGE_EXT_PIN_OWNER -> PAGE_EXT_TRACE_UNREF
>>>>
>>>> $whatever feature/user can then set the bit, for example, when migration
>>>> fails.
>>>
>>> I couldn't imagine put_page tracking is generally useful except
>>> migration failure. Do you have reasonable usecase in your mind
>>> to make the feature general to be used?
>>
>> HWpoison etc. purposes maybe? Trace who still held a reference a page
>> that was poisoned and couldn't be removed?  Or in general, tracking
> 
> I am not familiar with hwpoison so here dumb question goes:
> Is that different one with __soft_offline_page?
> It uses migrate_pages so current interface supports it with filter.

__soft_offline_page() won't kill the target and try to migrate because
the pages are about to be damaged and we can still access them.

ordinary memory errors mean we kill the target because we cannot access
the page anymore without triggering MCEs (or worse IIUC) again.

So in my thinking, after memory_failure(), it could eventually be
helpful to figure out who still has a (temporary) reference to such a
broken page, even after killing the process. But that's just one idea I
quickly came up with.

> 
> echo "memory_failure" > $trace_dir/events/page_pin_owner/report_page_pinners/filter
> 
>> references to something that should have a refcount of 0 because it
>> should have been freed, but for some reason there are still references
>> around?
> 
> Sounds like you are talking about memory leak? What's the purpose
> with trace, not using other existing tool to find memory leak?
> 

IIRC, kmemleak can find objects that are no longer referenced, and we
cannot track buddy allocations, but only kmalloc and friends.

>>
>>> Otherwise, I'd like to have feature naming more higher level
>>> to represent page migration failure and then tracking unref of
>>> the page. In the sense, PagePinOwner John suggested was good
>>> candidate(Even, my original naming PagePinner was worse) since
>>
>> Personally, I dislike both variants.
>>
>>> I was trouble to abstract the feature with short word.
>>> If we approach "what feature is doing" rather than "what's
>>> the feature's goal"(I feel the your suggestion would be close
>>> to what feature is doing), I'd like to express "unreference on
>>> migraiton failed page" so PAGE_EXT_UNMIGRATED_UNREF
>>> (However, I prefer the feature naming more "what we want to achieve")
>>>
>> E.g., PAGE_EXT_TRACE_UNREF will trace unref to the page once the bit is
>> set. The functionality itself is completely independent of migration
>> failures. That's just the code that sets it to enable the underlying
>> tracing for that specific page.
> 
> I agree that make something general is great but I also want to avoid
> create something too big from the beginning with just imagination.
> So, I'd like to hear more concrete and appealing usecases and then
> we could think over this trace approach is really the best one to
> achieve the goal. Once it's agreed, the naming you suggested would
> make sense. 

At least for me it's a lot cleaner if a feature clearly expresses what
it actually does. Staring at PAGE_EXT_PIN_OWNER I initially had no clue.
I was assuming we would actually track (not trace!) all active FOLL_PIN
(not unref callers!). Maybe that makes it clearer why I'd prefer a
clearer name.

>>
>> Makes sense, I was expecting the output to be large, but possible it's
>> going to be way too large.
>>
>> Would it also make sense to track for a flagged page new taken
>> references, such that you can differentiate between new (e.g.,
>> temporary) ones and previous ones? Feels like a reasonable addition.
> 
> I actually tried it and it showed 2x times bigger output.

Is 2x that bad? Or would it be worth making it configurable?

> For me to debug CMA alloation failure, the new get_page callstack
> after migration failure were waste since they repeated from lru
> adding, isolate from the LRU something. Rather than get callsite,
> I needed only put call sites since I could deduce where the pair-get
> came from.

Could maybe some filters help that exclude such LRU activity?

-- 
Thanks,

David / dhildenb

Powered by blists - more mailing lists