Re: [PATCH] mm/page_alloc: return 0 in case this node has no page within the zone

[Michal Hocko]

On Wed 22-02-17 18:51:31, Wei Yang wrote:
> On Wed, Feb 22, 2017 at 09:49:47AM +0100, Michal Hocko wrote:
> >On Thu 09-02-17 21:59:29, Wei Yang wrote:
> >> On Tue, Feb 07, 2017 at 04:41:21PM +0100, Michal Hocko wrote:
> >> >On Tue 07-02-17 23:32:47, Wei Yang wrote:
> >> >> On Tue, Feb 07, 2017 at 10:45:57AM +0100, Michal Hocko wrote:
> >> >[...]
> >> >> >Is there any reason why for_each_mem_pfn_range cannot be changed to
> >> >> >honor the given start/end pfns instead? I can imagine that a small zone
> >> >> >would see a similar pointless iterations...
> >> >> >
> >> >> 
> >> >> Hmm... No special reason, just not thought about this implementation. And
> >> >> actually I just do the similar thing as in zone_spanned_pages_in_node(), in
> >> >> which also return 0 when there is no overlap.
> >> >> 
> >> >> BTW, I don't get your point. You wish to put the check in
> >> >> for_each_mem_pfn_range() definition?
> >> >
> >> >My point was that you are handling one special case (an empty zone) but
> >> >the underlying problem is that __absent_pages_in_range might be wasting
> >> >cycles iterating over memblocks that are way outside of the given pfn
> >> >range. At least this is my understanding. If you fix that you do not
> >> >need the special case, right?
> >> >-- 
> >> >Michal Hocko
> >> >SUSE Labs
> >> 
> >> > Not really, sorry, this area is full of awkward and subtle code when new
> >> > changes build on top of previous awkwardness/surprises. Any cleanup
> >> > would be really appreciated. That is the reason I didn't like the
> >> > initial check all that much.
> >> 
> >> Looks my fetchmail failed to get your last reply. So I copied it here.
> >> 
> >> Yes, the change here looks not that nice, while currently this is what I can't
> >> come up with.
> >
> >THen I will suggest dropping this patch from the mmotm tree because it
> >doesn't sound like a big improvement and I would encourage you or
> >anybody else to take a deeper look and unclutter this area to be more
> >readable and better maintainable.
> 
> Hi, Michal
> 
> I don't get your point, which part of the code makes you feel uncomfortable?

It adds a check which would better be handled at a different level. I've
tried to explain what are my concerns about quick&dirty solutions in
this area. I would agree to add the check as a immediate workaround if
this had some measurable benefits but the changelog doesn't mention
any. So I do not really see this as an improvement in the end. If we
want to address the suboptimal code, let's do it properly rather than
spewing ifs all over the code.

-- 
Michal Hocko
SUSE Labs