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

[Wei Yang]

On Wed, Feb 22, 2017 at 12:45:22PM +0100, Michal Hocko wrote:
>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.

Yep, I agree that to pursuit a better solution in the project is our ultimate
goal.

First let me explain why it is not enough to add it in the "different level" .
As you mentioned, it would be better to add this check in
__absent_pages_in_range(). Yes, I agree this would be proper place to add
this check at first sight. While __absent_pages_in_range() return 0 is not a
guarantee the ZONE_MOVEABLE handling would get 0 absent_page . So if we add the
check in __absent_pages_in_range(), we still need to add a check before
ZONE_MOVEABLE handling to avoid the iteration in this loop.

Here is a code snippet, I could come up with your suggestion.

	zone_absent_pages_in_node()
		
		__absent_pages_in_range()
			check zone and node overlap

		check zone and node overlap
		handle ZONE_MOVEABLE

Then let me explain why it is not necessary to add the check in
__absent_pages_in_range() now. Hmm... this looks a very good place to add this
check, since it would guard all cases to avoid these invalid
iterations. While in current implementation zone_absent_pages_in_node() is the
only place where the (start_pfn == end_pfn) could happen.

The __absent_pages_in_range() is invoked at three places:

* numa_meminfo_cover_memory()
* zone_absent_pages_in_node()
* absent_pages_in_range()

And looks the other two would have no chance to pass two equal parameters,
which falls into the check. So it looks not necessary to add a check here for
more general cases. The detailed explanation is posted in this mail,
https://lkml.org/lkml/2017/2/8/337

Based on these two analysis, I choose to put the check outside
__absent_pages_in_range(), which makes the code look like this:

	zone_absent_pages_in_node()
		
		check zone and node overlap
		__absent_pages_in_range()

		handle ZONE_MOVEABLE

So only one check instead of two.

Last but not the least, yes, I agree with you that this check is better to
be put in a different level while it may not as good as we think for current
implementation.

Glad to discuss with you about these details. Not sure which one you like or
you don't like any of them?

>
>-- 
>Michal Hocko
>SUSE Labs

-- 
Wei Yang
Help you, Help me

Attachment: signature.asc
Description: PGP signature