Messages in this thread | | | From | Ard Biesheuvel <> | Date | Thu, 15 Mar 2018 13:48:00 +0000 | Subject | Re: [PATCH] Revert "mm/page_alloc: fix memmap_init_zone pageblock alignment" |
| |
On 15 March 2018 at 11:43, Michal Hocko <mhocko@kernel.org> wrote: > On Thu 15-03-18 10:17:24, Ard Biesheuvel wrote: >> On 15 March 2018 at 10:14, Michal Hocko <mhocko@kernel.org> wrote: >> > On Wed 14-03-18 15:54:16, Ard Biesheuvel wrote: >> >> On 14 March 2018 at 14:54, Michal Hocko <mhocko@kernel.org> wrote: >> >> > On Wed 14-03-18 14:35:12, Ard Biesheuvel wrote: >> >> >> On 14 March 2018 at 14:13, Michal Hocko <mhocko@kernel.org> wrote: >> >> >> > Does http://lkml.kernel.org/r/20180313224240.25295-1-neelx@redhat.com >> >> >> > fix your issue? From the debugging info you provided it should because >> >> >> > the patch prevents jumping backwards. >> >> >> > >> >> >> >> >> >> The patch does fix the boot hang. >> >> >> >> >> >> But I am concerned that we are papering over a fundamental flaw in >> >> >> memblock_next_valid_pfn(). >> >> > >> >> > It seems that memblock_next_valid_pfn is doing the right thing here. It >> >> > is the alignment which moves the pfn back AFAICS. I am not really >> >> > impressed about the original patch either, to be completely honest. >> >> > It just looks awfully tricky. I still didn't manage to wrap my head >> >> > around the original issue though so I do not have much better ideas to >> >> > be honest. >> >> >> >> So first of all, memblock_next_valid_pfn() never refers to its max_pfn >> >> argument, which is odd nut easily fixed. >> > >> > There is a patch to remove that parameter sitting in the mmotm tree. >> > >> >> Then, the whole idea of substracting one so that the pfn++ will >> >> produce the expected value is rather hacky, >> > >> > Absolutely agreed! >> > >> >> But the real problem is that rounding down pfn for the next iteration >> >> is dodgy, because early_pfn_valid() isn't guaranteed to return true >> >> for the rounded down value. I know it is probably fine in reality, but >> >> dodgy as hell. >> > >> > Yes, that is what I meant when saying I was not impressed... I am always >> > nervous when a loop makes jumps back and forth. I _think_ the main >> > problem here is that we try to initialize a partial pageblock even >> > though a part of it is invalid. We should simply ignore struct pages >> > for those pfns. We don't do that and that is mostly because of the >> > disconnect between what the page allocator and early init code refers to >> > as a unit of memory to care about. I do not remember exactly why but I >> > strongly suspect this is mostly a performance optimization on the page >> > allocator side so that we do not have to check each and every pfn. Maybe >> > we should signal partial pageblocks from an early code and drop the >> > optimization in the page allocator init code. >> > >> >> The same applies to the call to early_pfn_in_nid() btw >> > >> > Why? >> >> By 'the same' I mean it isn't guaranteed to return true for the >> rounded down value *at the API level*. I understand it will be mostly >> fine in reality, but juggling (in)valid PFNs like this is likely to >> end badly. > > OK, I see your point now. I can really imagine that sub-pageblocks would > be splitted into different NUMA nodes but that should be really rare. >
Yes, it should never happen.
But these abstractions exist for a reason: it makes this code understandable to humans, and so taking all kinds of shortcuts around them makes the code unmaintainable.
If ARM's implementation of pfn_valid() is flawed, we should fix it. If memblock_next_valid_pfn() is flawed, we should fix it. But papering over these issues by bypassing the abstractions is really not the way to go (but I think we're already in agreement there)
| |