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

Message

Michal Hocko Oct. 23, 2019, 10:27 a.m. UTC

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.

Comments

Michal Hocko Oct. 24, 2019, 8:20 a.m. UTC | #1

On Wed 23-10-19 12:27:35, Michal Hocko wrote:
[...]
> I went with a bound to the pages iteratred over in the free_list. See
> patch 2.

I will fold http://lkml.kernel.org/r/[e-mail]
to patch 2 unless there are any objections. If there are no further
comments I will send the two patches without an RFC tomorrow.

Thanks for all the feedback.

Waiman Long Oct. 24, 2019, 4:16 p.m. UTC | #2

On 10/24/19 4:20 AM, Michal Hocko wrote:
> On Wed 23-10-19 12:27:35, Michal Hocko wrote:
> [...]
>> I went with a bound to the pages iteratred over in the free_list. See
>> patch 2.
> I will fold http://lkml.kernel.org/r/[e-mail]
> to patch 2 unless there are any objections. If there are no further
> comments I will send the two patches without an RFC tomorrow.
>
> Thanks for all the feedback.

I am fine with your change. My concern is to make sure that there is a
reasonable bound to the worst case scenario. With that change, the upper
bound is iterating 100,000 list entries. I think Andrew suggested
lowering it to 1024. That I think may be too low, but I don't mind if it
is lowered somewhat from the current value.

Cheers,
Longman