Message-ID: <YeHHBhhaCf3g9QVe@google.com>
Date:   Fri, 14 Jan 2022 10:55:02 -0800
From:   Minchan Kim <minchan@...nel.org>
To:     David Hildenbrand <david@...hat.com>
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 Fri, Jan 14, 2022 at 05:51:24PM +0100, David Hildenbrand wrote:
> On 14.01.22 17:39, Minchan Kim wrote:
> > On Fri, Jan 14, 2022 at 02:31:43PM +0100, David Hildenbrand wrote:
> >> 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.
> > 
> > 
> > Thanks for the clarification. Is the trace best fit in the case?
> > Presumably you know the broken page, can't you find who owns the page
> > using /proc/pid/pagemap?
> > Furthermore, page_get/put operations commonly could happen in
> > different contexts regardless of page's owner so the trace from
> > different context is still helpful?
> > 
> > If it's helpful, could you tell what you want to make the interface to
> > set the bit of broken page? For example, as-is case for page migration,
> > report_page_pinners is called to mark failed page at unmap_and_move.
> > Do you want to add something similar(maybe, set_page_ref_track under
> > rename) in memory-failure.c?
> > 
> > It would be very helpful to design the feature's interface(surely,
> > naming as well) and write description to convince others "yeah,
> > sounds like so useful for the case and that's best fit than other way".
> 
> I currently don't have time to explore this further, it was just a
> random thought how else this might be useful.

Thanks for the input. The concrete usecase/idea are really important
to define what information the trace will dump as well as defining
interface.

Please consider the current dump content are also good to cover
what you are thinking for potential cases. And potentially, someone
want to see different data for their failure and put event for
their taste. It means we need to tell why the page was marked to
be tracked originally in the put_page, such as, cma, or hwpoison
or something else so we need per-page metadata to distinguish
them. It makes me think over strong justfication even though
page_ext would be a little more tolerable than struct page.

> 
> 
> >>
> >>>
> >>> 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.
> > 
> > PageOwner is your good buddy.
> 
> Page owner tracks who owns a page but not who else holds a reference
> (some buffer, gup, whatsoever), right?

I thought you wanted to use the feature for memory leak since you
mentioned kmemleak. I don't understand fully what you want but
how could you get the trace if the reference holder never release?
Only way to get to the goal is you need to trace all the get trace
before the event happen and then correlate them with put. However,
the get happened long time ago, you need to assign huge amount of
RAM to keep the record. Without compelling usecase, I am not convinced.

> 
> > 
> >>
> >>>>
> >>>>> 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.
> > 
> > I totally agree PagePinOwner is not 100% straightforward. I'm open for
> > other better name. Currently we are discussing how we could generalize
> > and whether it's useful or not. Depending on the discussion, the design/
> > interface as well as naming could be changed. No problem.
> 
> PagePinOwner is just highly misleading. Because that's not what the
> feature does. Having that said, i hope we'll get other opinions as well.

Yeah, I'm open for the better name. Still thinking but I was not good.

Powered by blists - more mailing lists