[RFC PATCH 0/2] mm/vmstat: Reduce zone lock hold time when reading /proc/pagetypeinfo

[Michal Hocko <mhocko@kernel.org>]

On Wed 23-10-19 10:56:08, Mel Gorman wrote:
> On Wed, Oct 23, 2019 at 11:04:22AM +0200, Michal Hocko wrote:
> > So can we go with this to address the security aspect of this and have
> > something trivial to backport.
> > 
> 
> Yes.

Ok, patch 1 in reply to this email.

> > > > > There is a free_area structure associated with each page order. There
> > > > > is also a nr_free count within the free_area for all the different
> > > > > migration types combined. Tracking the number of free list entries
> > > > > for each migration type will probably add some overhead to the fast
> > > > > paths like moving pages from one migration type to another which may
> > > > > not be desirable.
> > > > 
> > > > Have you tried to measure that overhead?
> > > >  
> > > 
> > > I would prefer this option not be taken. It would increase the cost of
> > > watermark calculations which is a relatively fast path.
> > 
> > Is the change for the wmark check going to require more than
> > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > index c0b2e0306720..5d95313ba4a5 100644
> > --- a/mm/page_alloc.c
> > +++ b/mm/page_alloc.c
> > @@ -3448,9 +3448,6 @@ bool __zone_watermark_ok(struct zone *z, unsigned int order, unsigned long mark,
> >  		struct free_area *area = &z->free_area[o];
> >  		int mt;
> >  
> > -		if (!area->nr_free)
> > -			continue;
> > -
> >  		for (mt = 0; mt < MIGRATE_PCPTYPES; mt++) {
> >  			if (!free_area_empty(area, mt))
> >  				return true;
> > 
> > Is this really going to be visible in practice? Sure we are going to do
> > more checks but most orders tend to have at least some memory in a
> > reasonably balanced system and we can hardly expect an optimal
> > allocation path on those that are not.
> >  
> 
> You also have to iterate over them all later in the same function.  The the
> free counts are per migrate type then they would have to be iterated over
> every time.
> 
> Similarly, there would be multiple places where all the counters would
> have to be iterated -- find_suitable_fallback, show_free_areas,
> fast_isolate_freepages, fill_contig_page_info, zone_init_free_lists etc.
> 
> It'd be a small cost but given that it's aimed at fixing a problem with
> reading pagetypeinfo, is it really worth it? I don't think so.

Fair enough.

[...]
> > As pointed out in other email. The problem with this patch is that it
> > hasn't really removed the iteration over the whole free_list which is
> > the primary problem. So I think that we should either consider this a
> > non-issue and make it "admin knows this is potentially expensive" or do
> > something like Andrew was suggesting if we do not want to change the
> > nr_free accounting.
> > 
> 
> Again, the cost is when reading a proc file. From what Andrew said,
> the lock is necessary to safely walk the list but if anything. I would
> be ok with limiting the length of the walk but honestly, I would also
> be ok with simply deleting the proc file. The utility for debugging a
> problem with it is limited now (it was more important when fragmentation
> avoidance was first introduced) and there is little an admin can do with
> the information. I can't remember the last time I asked for the contents
> of the file when trying to debug a problem. There is a possibility that
> someone will complain but I'm not aware of any utility that reads the
> information and does something useful with it. In the unlikely event
> something breaks, the file can be re-added with a limited walk.

I went with a bound to the pages iteratred over in the free_list. See
patch 2.

-- 
Michal Hocko
SUSE Labs