Re: [PATCH v3 1/1] mm: page_alloc: skip over regions of invalid pfns on UMA

[Michal Hocko]

On Mon 12-02-18 17:16:40, Eugeniu Rosca wrote:
> Hi Michal,
> 
> On Mon, Feb 12, 2018 at 04:03:14PM +0100, Michal Hocko wrote:
> > On Sat 03-02-18 13:24:22, Eugeniu Rosca wrote:
> > [...]
> > > That said, I really hope this won't be the last comment in the thread
> > > and appropriate suggestions will come on how to go forward.
> > 
> > Just to make sure we are on the same page. I was suggesting the
> > following. The patch is slightly larger just because I move
> > memblock_next_valid_pfn around which I find better than sprinkling
> > ifdefs around. Please note I haven't tried to compile test this.
> 
> I got your point. So, I was wrong. You are not preferring v2 of this
> patch, but suggest a new variant of it. For the record, I've also
> build/boot-tested your variant with no issues. The reason I did not
> make it my favorite is to allow reviewers to concentrate on what's
> actually the essence of this change, i.e. relaxing the dependency of
> memblock_next_valid_pfn() from HAVE_MEMBLOCK_NODE_MAP (which requires/
> depends on NUMA) to HAVE_MEMBLOCK (which doesn't).

Yes, and that makes perfect sense.

> As I've said in some previous reply, I am open minded about which
> variant is selected by MM people, since, from my point of view, all of
> them do the same thing with variable degree of code readability.

Agreed. I just wanted to reduce to necessity to define
memblock_next_valid_pfn for !CONFIG_HAVE_MEMBLOCK. IS_ENABLED check also
nicely hides the ifdefery. I also prefer to have more compact ifdef
blocks rather than smaller ones split by other functions.

> For me it's not a problem to submit a new patch. I guess that a
> prerequisite for this is to reach some agreement on what people think is
> the best option, which I feel didn't occur yet.

I do not have a _strong_ preference here as well. So I will leave the
decision to you.

In any case feel free to add
Acked-by: Michal Hocko <mhocko@xxxxxxxx>

-- 
Michal Hocko
SUSE Labs