Re: [PATCH RFC] mm/page_idle: simple idle page tracking for virtual memory

[Joel Fernandes <joel@joelfernandes.org>]

On Tue, Jul 23, 2019 at 04:59:07PM +0300, Konstantin Khlebnikov wrote:
> 
> 
> On 23.07.2019 16:46, Joel Fernandes wrote:
> > On Tue, Jul 23, 2019 at 02:54:26PM +0300, Konstantin Khlebnikov wrote:
> > > The page_idle tracking feature currently requires looking up the pagemap
> > > for a process followed by interacting with /sys/kernel/mm/page_idle.
> > > This is quite cumbersome and can be error-prone too. If between
> > > accessing the per-PID pagemap and the global page_idle bitmap, if
> > > something changes with the page then the information is not accurate.
> > > More over looking up PFN from pagemap in Android devices is not
> > > supported by unprivileged process and requires SYS_ADMIN and gives 0 for
> > > the PFN.
> > > 
> > > This patch adds simplified interface which works only with mapped pages:
> > > Run: "echo 6 > /proc/pid/clear_refs" to mark all mapped pages as idle.
> > > Pages that still idle are marked with bit 57 in /proc/pid/pagemap.
> > > Total size of idle pages is shown in /proc/pid/smaps (_rollup).
> > > 
> > > Piece of comment is stolen from Joel Fernandes <joel@joelfernandes.org>
> > 
> > This will not work well for the problem at hand, the heap profiler
> > (heapprofd) only wants to clear the idle flag for the heap memory area which
> > is what it is profiling. There is no reason to do it for all mapped pages.
> > Using the /proc/pid/page_idle in my patch, it can be done selectively for
> > particular memory areas.
> > 
> > I had previously thought of having an interface that accepts an address
> > range to set the idle flag, however that is also more complexity.
> 
> Profiler could look into particular area in /proc/pid/smaps
> or count idle pages via /proc/pid/pagemap.
> 
> Selective /proc/pid/clear_refs is not so hard to add.
> Somthing like echo "6 561214d03000-561214d29000" > /proc/pid/clear_refs
> might be useful for all other operations.

This seems really odd of an interface. Also I don't see how you can avoid
looking up reverse maps to determine if a page is really idle.

What is also more odd is that traditionally clear_refs does interfere with
reclaim due to clearing of accessed bit. Now you have one of the interfaces
with clear_refs that does not interfere with reclaim. That is makes it very
inconsistent. Also in this patch you have 2 interfaces to solve this, where
as my patch added a single clean interface that is easy to use and does not
need parsing of address ranges.

All in all, I don't see much the point of this honestly. But thanks for
poking at it.

thanks,

 - Joel