| Message ID | 1612239114-28428-2-git-send-email-anshuman.khandual@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64/mm: Fix pfn_valid() for ZONE_DEVICE based memory | expand |
On Tue, Feb 02, 2021 at 09:41:53AM +0530, Anshuman Khandual wrote: > pfn_valid() validates a pfn but basically it checks for a valid struct page > backing for that pfn. It should always return positive for memory ranges > backed with struct page mapping. But currently pfn_valid() fails for all > ZONE_DEVICE based memory types even though they have struct page mapping. > > pfn_valid() asserts that there is a memblock entry for a given pfn without > MEMBLOCK_NOMAP flag being set. The problem with ZONE_DEVICE based memory is > that they do not have memblock entries. Hence memblock_is_map_memory() will > invariably fail via memblock_search() for a ZONE_DEVICE based address. This > eventually fails pfn_valid() which is wrong. memblock_is_map_memory() needs > to be skipped for such memory ranges. As ZONE_DEVICE memory gets hotplugged > into the system via memremap_pages() called from a driver, their respective > memory sections will not have SECTION_IS_EARLY set. > > Normal hotplug memory will never have MEMBLOCK_NOMAP set in their memblock > regions. Because the flag MEMBLOCK_NOMAP was specifically designed and set > for firmware reserved memory regions. memblock_is_map_memory() can just be > skipped as its always going to be positive and that will be an optimization > for the normal hotplug memory. Like ZONE_DEVICE based memory, all normal > hotplugged memory too will not have SECTION_IS_EARLY set for their sections > > Skipping memblock_is_map_memory() for all non early memory sections would > fix pfn_valid() problem for ZONE_DEVICE based memory and also improve its > performance for normal hotplug memory as well. Hmm. Although I follow your logic, this does seem to rely on an awful lot of assumptions to continue to hold true as the kernel evolves. In particular, how do we ensure that early sections are always fully backed with 'struct page's and never contain any nomap entries? What's to stop somebody changing that and quietly breaking our pfn_valid() implementation? And to be clear, I'm not trying to say that this patch is broken. I'm just trying to work out how on Earth we can maintain it! Will
On Tue, Feb 02, 2021 at 12:32:15PM +0000, Will Deacon wrote: > On Tue, Feb 02, 2021 at 09:41:53AM +0530, Anshuman Khandual wrote: > > pfn_valid() validates a pfn but basically it checks for a valid struct page > > backing for that pfn. It should always return positive for memory ranges > > backed with struct page mapping. But currently pfn_valid() fails for all > > ZONE_DEVICE based memory types even though they have struct page mapping. > > > > pfn_valid() asserts that there is a memblock entry for a given pfn without > > MEMBLOCK_NOMAP flag being set. The problem with ZONE_DEVICE based memory is > > that they do not have memblock entries. Hence memblock_is_map_memory() will > > invariably fail via memblock_search() for a ZONE_DEVICE based address. This > > eventually fails pfn_valid() which is wrong. memblock_is_map_memory() needs > > to be skipped for such memory ranges. As ZONE_DEVICE memory gets hotplugged > > into the system via memremap_pages() called from a driver, their respective > > memory sections will not have SECTION_IS_EARLY set. > > > > Normal hotplug memory will never have MEMBLOCK_NOMAP set in their memblock > > regions. Because the flag MEMBLOCK_NOMAP was specifically designed and set > > for firmware reserved memory regions. memblock_is_map_memory() can just be > > skipped as its always going to be positive and that will be an optimization > > for the normal hotplug memory. Like ZONE_DEVICE based memory, all normal > > hotplugged memory too will not have SECTION_IS_EARLY set for their sections > > > > Skipping memblock_is_map_memory() for all non early memory sections would > > fix pfn_valid() problem for ZONE_DEVICE based memory and also improve its > > performance for normal hotplug memory as well. > > Hmm. Although I follow your logic, this does seem to rely on an awful lot of > assumptions to continue to hold true as the kernel evolves. In particular, > how do we ensure that early sections are always fully backed with Sorry, typo here: ^^^ should be *non-early* sections. Will
On 02.02.21 13:35, Will Deacon wrote: > On Tue, Feb 02, 2021 at 12:32:15PM +0000, Will Deacon wrote: >> On Tue, Feb 02, 2021 at 09:41:53AM +0530, Anshuman Khandual wrote: >>> pfn_valid() validates a pfn but basically it checks for a valid struct page >>> backing for that pfn. It should always return positive for memory ranges >>> backed with struct page mapping. But currently pfn_valid() fails for all >>> ZONE_DEVICE based memory types even though they have struct page mapping. >>> >>> pfn_valid() asserts that there is a memblock entry for a given pfn without >>> MEMBLOCK_NOMAP flag being set. The problem with ZONE_DEVICE based memory is >>> that they do not have memblock entries. Hence memblock_is_map_memory() will >>> invariably fail via memblock_search() for a ZONE_DEVICE based address. This >>> eventually fails pfn_valid() which is wrong. memblock_is_map_memory() needs >>> to be skipped for such memory ranges. As ZONE_DEVICE memory gets hotplugged >>> into the system via memremap_pages() called from a driver, their respective >>> memory sections will not have SECTION_IS_EARLY set. >>> >>> Normal hotplug memory will never have MEMBLOCK_NOMAP set in their memblock >>> regions. Because the flag MEMBLOCK_NOMAP was specifically designed and set >>> for firmware reserved memory regions. memblock_is_map_memory() can just be >>> skipped as its always going to be positive and that will be an optimization >>> for the normal hotplug memory. Like ZONE_DEVICE based memory, all normal >>> hotplugged memory too will not have SECTION_IS_EARLY set for their sections >>> >>> Skipping memblock_is_map_memory() for all non early memory sections would >>> fix pfn_valid() problem for ZONE_DEVICE based memory and also improve its >>> performance for normal hotplug memory as well. >> >> Hmm. Although I follow your logic, this does seem to rely on an awful lot of >> assumptions to continue to hold true as the kernel evolves. In particular, >> how do we ensure that early sections are always fully backed with > > Sorry, typo here: ^^^ should be *non-early* sections. It might be a good idea to have a look at generic include/linux/mmzone.h:pfn_valid() As I expressed already, long term we should really get rid of the arm64 variant and rather special-case the generic one. Then we won't go out of sync - just as it happened with ZONE_DEVICE handling here.
On Tue, Feb 02, 2021 at 01:39:29PM +0100, David Hildenbrand wrote: > On 02.02.21 13:35, Will Deacon wrote: > > On Tue, Feb 02, 2021 at 12:32:15PM +0000, Will Deacon wrote: > > > On Tue, Feb 02, 2021 at 09:41:53AM +0530, Anshuman Khandual wrote: > > > > pfn_valid() validates a pfn but basically it checks for a valid struct page > > > > backing for that pfn. It should always return positive for memory ranges > > > > backed with struct page mapping. But currently pfn_valid() fails for all > > > > ZONE_DEVICE based memory types even though they have struct page mapping. > > > > > > > > pfn_valid() asserts that there is a memblock entry for a given pfn without > > > > MEMBLOCK_NOMAP flag being set. The problem with ZONE_DEVICE based memory is > > > > that they do not have memblock entries. Hence memblock_is_map_memory() will > > > > invariably fail via memblock_search() for a ZONE_DEVICE based address. This > > > > eventually fails pfn_valid() which is wrong. memblock_is_map_memory() needs > > > > to be skipped for such memory ranges. As ZONE_DEVICE memory gets hotplugged > > > > into the system via memremap_pages() called from a driver, their respective > > > > memory sections will not have SECTION_IS_EARLY set. > > > > > > > > Normal hotplug memory will never have MEMBLOCK_NOMAP set in their memblock > > > > regions. Because the flag MEMBLOCK_NOMAP was specifically designed and set > > > > for firmware reserved memory regions. memblock_is_map_memory() can just be > > > > skipped as its always going to be positive and that will be an optimization > > > > for the normal hotplug memory. Like ZONE_DEVICE based memory, all normal > > > > hotplugged memory too will not have SECTION_IS_EARLY set for their sections > > > > > > > > Skipping memblock_is_map_memory() for all non early memory sections would > > > > fix pfn_valid() problem for ZONE_DEVICE based memory and also improve its > > > > performance for normal hotplug memory as well. > > > > > > Hmm. Although I follow your logic, this does seem to rely on an awful lot of > > > assumptions to continue to hold true as the kernel evolves. In particular, > > > how do we ensure that early sections are always fully backed with > > > > Sorry, typo here: ^^^ should be *non-early* sections. > > It might be a good idea to have a look at generic > include/linux/mmzone.h:pfn_valid() The generic implementation already makes assumptions that aren't true on arm64, so that's why we've ended up with our own implementation. But the patches here put us in a position where I worry that pfn_valid() may return 'true' in future for cases where the underlying struct page is either non-existent or bogus, and debugging those failures really sucks. We had a raft of those back when NOMAP was introduced and I don't want to re-live that experience. > As I expressed already, long term we should really get rid of the arm64 > variant and rather special-case the generic one. Then we won't go out of > sync - just as it happened with ZONE_DEVICE handling here. Why does this have to be long term? This ZONE_DEVICE stuff could be the carrot on the stick :) Will
On 02.02.21 13:51, Will Deacon wrote: > On Tue, Feb 02, 2021 at 01:39:29PM +0100, David Hildenbrand wrote: >> On 02.02.21 13:35, Will Deacon wrote: >>> On Tue, Feb 02, 2021 at 12:32:15PM +0000, Will Deacon wrote: >>>> On Tue, Feb 02, 2021 at 09:41:53AM +0530, Anshuman Khandual wrote: >>>>> pfn_valid() validates a pfn but basically it checks for a valid struct page >>>>> backing for that pfn. It should always return positive for memory ranges >>>>> backed with struct page mapping. But currently pfn_valid() fails for all >>>>> ZONE_DEVICE based memory types even though they have struct page mapping. >>>>> >>>>> pfn_valid() asserts that there is a memblock entry for a given pfn without >>>>> MEMBLOCK_NOMAP flag being set. The problem with ZONE_DEVICE based memory is >>>>> that they do not have memblock entries. Hence memblock_is_map_memory() will >>>>> invariably fail via memblock_search() for a ZONE_DEVICE based address. This >>>>> eventually fails pfn_valid() which is wrong. memblock_is_map_memory() needs >>>>> to be skipped for such memory ranges. As ZONE_DEVICE memory gets hotplugged >>>>> into the system via memremap_pages() called from a driver, their respective >>>>> memory sections will not have SECTION_IS_EARLY set. >>>>> >>>>> Normal hotplug memory will never have MEMBLOCK_NOMAP set in their memblock >>>>> regions. Because the flag MEMBLOCK_NOMAP was specifically designed and set >>>>> for firmware reserved memory regions. memblock_is_map_memory() can just be >>>>> skipped as its always going to be positive and that will be an optimization >>>>> for the normal hotplug memory. Like ZONE_DEVICE based memory, all normal >>>>> hotplugged memory too will not have SECTION_IS_EARLY set for their sections >>>>> >>>>> Skipping memblock_is_map_memory() for all non early memory sections would >>>>> fix pfn_valid() problem for ZONE_DEVICE based memory and also improve its >>>>> performance for normal hotplug memory as well. >>>> >>>> Hmm. Although I follow your logic, this does seem to rely on an awful lot of >>>> assumptions to continue to hold true as the kernel evolves. In particular, >>>> how do we ensure that early sections are always fully backed with >>> >>> Sorry, typo here: ^^^ should be *non-early* sections. >> >> It might be a good idea to have a look at generic >> include/linux/mmzone.h:pfn_valid() > > The generic implementation already makes assumptions that aren't true on > arm64, so that's why we've ended up with our own implementation. But the > patches here put us in a position where I worry that pfn_valid() may return > 'true' in future for cases where the underlying struct page is either > non-existent or bogus, and debugging those failures really sucks. We had a > raft of those back when NOMAP was introduced and I don't want to re-live > that experience. Yeah, and I agree when it comes to boot mem. However, the way generic memory hotplug/memremap infrastructure (->!early sections) works does not allow for such special cases you mention and would break quite some other code if messed up. So I wouldn't worry about that part too much for now. > >> As I expressed already, long term we should really get rid of the arm64 >> variant and rather special-case the generic one. Then we won't go out of >> sync - just as it happened with ZONE_DEVICE handling here. > > Why does this have to be long term? This ZONE_DEVICE stuff could be the > carrot on the stick :) Yes, I suggested to do it now, but Anshuman convinced me that doing a simple fix upfront might be cleaner --- for example when it comes to backporting :)
On 2/2/21 6:26 PM, David Hildenbrand wrote: > On 02.02.21 13:51, Will Deacon wrote: >> On Tue, Feb 02, 2021 at 01:39:29PM +0100, David Hildenbrand wrote: >>> On 02.02.21 13:35, Will Deacon wrote: >>>> On Tue, Feb 02, 2021 at 12:32:15PM +0000, Will Deacon wrote: >>>>> On Tue, Feb 02, 2021 at 09:41:53AM +0530, Anshuman Khandual wrote: >>>>>> pfn_valid() validates a pfn but basically it checks for a valid struct page >>>>>> backing for that pfn. It should always return positive for memory ranges >>>>>> backed with struct page mapping. But currently pfn_valid() fails for all >>>>>> ZONE_DEVICE based memory types even though they have struct page mapping. >>>>>> >>>>>> pfn_valid() asserts that there is a memblock entry for a given pfn without >>>>>> MEMBLOCK_NOMAP flag being set. The problem with ZONE_DEVICE based memory is >>>>>> that they do not have memblock entries. Hence memblock_is_map_memory() will >>>>>> invariably fail via memblock_search() for a ZONE_DEVICE based address. This >>>>>> eventually fails pfn_valid() which is wrong. memblock_is_map_memory() needs >>>>>> to be skipped for such memory ranges. As ZONE_DEVICE memory gets hotplugged >>>>>> into the system via memremap_pages() called from a driver, their respective >>>>>> memory sections will not have SECTION_IS_EARLY set. >>>>>> >>>>>> Normal hotplug memory will never have MEMBLOCK_NOMAP set in their memblock >>>>>> regions. Because the flag MEMBLOCK_NOMAP was specifically designed and set >>>>>> for firmware reserved memory regions. memblock_is_map_memory() can just be >>>>>> skipped as its always going to be positive and that will be an optimization >>>>>> for the normal hotplug memory. Like ZONE_DEVICE based memory, all normal >>>>>> hotplugged memory too will not have SECTION_IS_EARLY set for their sections >>>>>> >>>>>> Skipping memblock_is_map_memory() for all non early memory sections would >>>>>> fix pfn_valid() problem for ZONE_DEVICE based memory and also improve its >>>>>> performance for normal hotplug memory as well. >>>>> >>>>> Hmm. Although I follow your logic, this does seem to rely on an awful lot of >>>>> assumptions to continue to hold true as the kernel evolves. In particular, >>>>> how do we ensure that early sections are always fully backed with >>>> >>>> Sorry, typo here: ^^^ should be *non-early* sections. >>> >>> It might be a good idea to have a look at generic >>> include/linux/mmzone.h:pfn_valid() >> >> The generic implementation already makes assumptions that aren't true on >> arm64, so that's why we've ended up with our own implementation. But the >> patches here put us in a position where I worry that pfn_valid() may return >> 'true' in future for cases where the underlying struct page is either >> non-existent or bogus, and debugging those failures really sucks. We had a >> raft of those back when NOMAP was introduced and I don't want to re-live >> that experience. > > Yeah, and I agree when it comes to boot mem. However, the way generic memory hotplug/memremap infrastructure (->!early sections) works does not allow for such special cases you mention and would break quite some other code if messed up. So I wouldn't worry about that part too much for now. Agreed. > >> >>> As I expressed already, long term we should really get rid of the arm64 >>> variant and rather special-case the generic one. Then we won't go out of >>> sync - just as it happened with ZONE_DEVICE handling here. >> >> Why does this have to be long term? This ZONE_DEVICE stuff could be the >> carrot on the stick :) > > Yes, I suggested to do it now, but Anshuman convinced me that doing a simple fix upfront might be cleaner --- for example when it comes to backporting :) Right. The current pfn_valid() breaks for ZONE_DEVICE memory and this fixes the problem in the present context which can be easily backported if required. Changing or rather overhauling the generic code with new configs as proposed earlier (which I am planning to work on subsequently) would definitely be an improvement for the current pfn_valid() situation in terms of maintainability but then it should not stop us from fixing the problem now.
On Wed, Feb 03, 2021 at 09:20:39AM +0530, Anshuman Khandual wrote: > On 2/2/21 6:26 PM, David Hildenbrand wrote: > > On 02.02.21 13:51, Will Deacon wrote: > >> On Tue, Feb 02, 2021 at 01:39:29PM +0100, David Hildenbrand wrote: > >>> As I expressed already, long term we should really get rid of the arm64 > >>> variant and rather special-case the generic one. Then we won't go out of > >>> sync - just as it happened with ZONE_DEVICE handling here. > >> > >> Why does this have to be long term? This ZONE_DEVICE stuff could be the > >> carrot on the stick :) > > > > Yes, I suggested to do it now, but Anshuman convinced me that doing a > > simple fix upfront might be cleaner --- for example when it comes to > > backporting :) > > Right. The current pfn_valid() breaks for ZONE_DEVICE memory and this fixes > the problem in the present context which can be easily backported if required. > > Changing or rather overhauling the generic code with new configs as proposed > earlier (which I am planning to work on subsequently) would definitely be an > improvement for the current pfn_valid() situation in terms of maintainability > but then it should not stop us from fixing the problem now. Alright, I've mulled this over a bit. I don't agree that this patch helps with maintainability (quite the opposite, in fact), but perfection is the enemy of the good so I'll queue the series for 5.12. However, I'll revert the changes at the first sign of a problem, so please do work towards a generic solution which can replace this in the medium term. Even just adding hooks with well-defined (documented) semantics to pfn_valid() instead of having architectures override it wholesale would be a massive step forward in my opinion. Will
On Fri, Feb 05, 2021 at 06:55:53PM +0000, Will Deacon wrote: > On Wed, Feb 03, 2021 at 09:20:39AM +0530, Anshuman Khandual wrote: > > On 2/2/21 6:26 PM, David Hildenbrand wrote: > > > On 02.02.21 13:51, Will Deacon wrote: > > >> On Tue, Feb 02, 2021 at 01:39:29PM +0100, David Hildenbrand wrote: > > >>> As I expressed already, long term we should really get rid of the arm64 > > >>> variant and rather special-case the generic one. Then we won't go out of > > >>> sync - just as it happened with ZONE_DEVICE handling here. > > >> > > >> Why does this have to be long term? This ZONE_DEVICE stuff could be the > > >> carrot on the stick :) > > > > > > Yes, I suggested to do it now, but Anshuman convinced me that doing a > > > simple fix upfront might be cleaner --- for example when it comes to > > > backporting :) > > > > Right. The current pfn_valid() breaks for ZONE_DEVICE memory and this fixes > > the problem in the present context which can be easily backported if required. > > > > Changing or rather overhauling the generic code with new configs as proposed > > earlier (which I am planning to work on subsequently) would definitely be an > > improvement for the current pfn_valid() situation in terms of maintainability > > but then it should not stop us from fixing the problem now. > > Alright, I've mulled this over a bit. I don't agree that this patch helps > with maintainability (quite the opposite, in fact), but perfection is the > enemy of the good so I'll queue the series for 5.12. However, I'll revert > the changes at the first sign of a problem, so please do work towards a > generic solution which can replace this in the medium term. ... and dropped. These patches appear to be responsible for a boot regression reported by CKI: https://lore.kernel.org/r/cki.8D1CB60FEC.K6NJMEFQPV@redhat.com Will
On 2/11/21 5:23 PM, Will Deacon wrote: > On Fri, Feb 05, 2021 at 06:55:53PM +0000, Will Deacon wrote: >> On Wed, Feb 03, 2021 at 09:20:39AM +0530, Anshuman Khandual wrote: >>> On 2/2/21 6:26 PM, David Hildenbrand wrote: >>>> On 02.02.21 13:51, Will Deacon wrote: >>>>> On Tue, Feb 02, 2021 at 01:39:29PM +0100, David Hildenbrand wrote: >>>>>> As I expressed already, long term we should really get rid of the arm64 >>>>>> variant and rather special-case the generic one. Then we won't go out of >>>>>> sync - just as it happened with ZONE_DEVICE handling here. >>>>> >>>>> Why does this have to be long term? This ZONE_DEVICE stuff could be the >>>>> carrot on the stick :) >>>> >>>> Yes, I suggested to do it now, but Anshuman convinced me that doing a >>>> simple fix upfront might be cleaner --- for example when it comes to >>>> backporting :) >>> >>> Right. The current pfn_valid() breaks for ZONE_DEVICE memory and this fixes >>> the problem in the present context which can be easily backported if required. >>> >>> Changing or rather overhauling the generic code with new configs as proposed >>> earlier (which I am planning to work on subsequently) would definitely be an >>> improvement for the current pfn_valid() situation in terms of maintainability >>> but then it should not stop us from fixing the problem now. >> >> Alright, I've mulled this over a bit. I don't agree that this patch helps >> with maintainability (quite the opposite, in fact), but perfection is the >> enemy of the good so I'll queue the series for 5.12. However, I'll revert >> the changes at the first sign of a problem, so please do work towards a >> generic solution which can replace this in the medium term. > > ... and dropped. These patches appear to be responsible for a boot > regression reported by CKI: Ahh, boot regression ? These patches only change the behaviour for non boot memory only. > > https://lore.kernel.org/r/cki.8D1CB60FEC.K6NJMEFQPV@redhat.com Will look into the logs and see if there is something pointing to the problem.
On Thu, Feb 11, 2021 at 05:40:35PM +0530, Anshuman Khandual wrote: > On 2/11/21 5:23 PM, Will Deacon wrote: > > On Fri, Feb 05, 2021 at 06:55:53PM +0000, Will Deacon wrote: > >> On Wed, Feb 03, 2021 at 09:20:39AM +0530, Anshuman Khandual wrote: > >>> On 2/2/21 6:26 PM, David Hildenbrand wrote: > >>>> On 02.02.21 13:51, Will Deacon wrote: > >>>>> On Tue, Feb 02, 2021 at 01:39:29PM +0100, David Hildenbrand wrote: > >>>>>> As I expressed already, long term we should really get rid of the arm64 > >>>>>> variant and rather special-case the generic one. Then we won't go out of > >>>>>> sync - just as it happened with ZONE_DEVICE handling here. > >>>>> > >>>>> Why does this have to be long term? This ZONE_DEVICE stuff could be the > >>>>> carrot on the stick :) > >>>> > >>>> Yes, I suggested to do it now, but Anshuman convinced me that doing a > >>>> simple fix upfront might be cleaner --- for example when it comes to > >>>> backporting :) > >>> > >>> Right. The current pfn_valid() breaks for ZONE_DEVICE memory and this fixes > >>> the problem in the present context which can be easily backported if required. > >>> > >>> Changing or rather overhauling the generic code with new configs as proposed > >>> earlier (which I am planning to work on subsequently) would definitely be an > >>> improvement for the current pfn_valid() situation in terms of maintainability > >>> but then it should not stop us from fixing the problem now. > >> > >> Alright, I've mulled this over a bit. I don't agree that this patch helps > >> with maintainability (quite the opposite, in fact), but perfection is the > >> enemy of the good so I'll queue the series for 5.12. However, I'll revert > >> the changes at the first sign of a problem, so please do work towards a > >> generic solution which can replace this in the medium term. > > > > ... and dropped. These patches appear to be responsible for a boot > > regression reported by CKI: > > Ahh, boot regression ? These patches only change the behaviour > for non boot memory only. Sure, but this thing is horribly fragile, which is why I was nervous about touching it in the first place ;) > > https://lore.kernel.org/r/cki.8D1CB60FEC.K6NJMEFQPV@redhat.com > > Will look into the logs and see if there is something pointing to > the problem. We don't have a log yet, but I've asked whether earlycon works on the problematic machine (the failure seems to be specific to a certain TX2). Either way, this is too late for 5.12 now. Will
On 11.02.21 13:10, Anshuman Khandual wrote: > > > On 2/11/21 5:23 PM, Will Deacon wrote: >> On Fri, Feb 05, 2021 at 06:55:53PM +0000, Will Deacon wrote: >>> On Wed, Feb 03, 2021 at 09:20:39AM +0530, Anshuman Khandual wrote: >>>> On 2/2/21 6:26 PM, David Hildenbrand wrote: >>>>> On 02.02.21 13:51, Will Deacon wrote: >>>>>> On Tue, Feb 02, 2021 at 01:39:29PM +0100, David Hildenbrand wrote: >>>>>>> As I expressed already, long term we should really get rid of the arm64 >>>>>>> variant and rather special-case the generic one. Then we won't go out of >>>>>>> sync - just as it happened with ZONE_DEVICE handling here. >>>>>> >>>>>> Why does this have to be long term? This ZONE_DEVICE stuff could be the >>>>>> carrot on the stick :) >>>>> >>>>> Yes, I suggested to do it now, but Anshuman convinced me that doing a >>>>> simple fix upfront might be cleaner --- for example when it comes to >>>>> backporting :) >>>> >>>> Right. The current pfn_valid() breaks for ZONE_DEVICE memory and this fixes >>>> the problem in the present context which can be easily backported if required. >>>> >>>> Changing or rather overhauling the generic code with new configs as proposed >>>> earlier (which I am planning to work on subsequently) would definitely be an >>>> improvement for the current pfn_valid() situation in terms of maintainability >>>> but then it should not stop us from fixing the problem now. >>> >>> Alright, I've mulled this over a bit. I don't agree that this patch helps >>> with maintainability (quite the opposite, in fact), but perfection is the >>> enemy of the good so I'll queue the series for 5.12. However, I'll revert >>> the changes at the first sign of a problem, so please do work towards a >>> generic solution which can replace this in the medium term. >> >> ... and dropped. These patches appear to be responsible for a boot >> regression reported by CKI: > > Ahh, boot regression ? These patches only change the behaviour > for non boot memory only. > >> >> https://lore.kernel.org/r/cki.8D1CB60FEC.K6NJMEFQPV@redhat.com > > Will look into the logs and see if there is something pointing to > the problem. > It's strange. One thing I can imagine is a mis-detection of early sections. However, I don't see that happening: In sparse_init_nid(), we: 1. Initialize the memmap 2. Set SECTION_IS_EARLY | SECTION_HAS_MEM_MAP via sparse_init_one_section() Only hotplugged sections (DIMMs, dax/kmem) set SECTION_HAS_MEM_MAP without SECTION_IS_EARLY - which is correct, because these are not early. So once we know that we have valid_section() -- SECTION_HAS_MEM_MAP is set -- early_section() should be correct. Even if someone would be doing a pfn_valid() after memblocks_present()->memory_present() but before sparse_init_nid(), we should be fine (!valid_section() -> return 0). As it happens early during boot, I doubt that some NVDIMMs that get detected and added early during boot as system RAM (via dax/kmem). Are the problem.
On Thu, Feb 11, 2021 at 01:35:56PM +0100, David Hildenbrand wrote: > On 11.02.21 13:10, Anshuman Khandual wrote: > > On 2/11/21 5:23 PM, Will Deacon wrote: > > > ... and dropped. These patches appear to be responsible for a boot > > > regression reported by CKI: > > > > Ahh, boot regression ? These patches only change the behaviour > > for non boot memory only. > > > > > https://lore.kernel.org/r/cki.8D1CB60FEC.K6NJMEFQPV@redhat.com > > > > Will look into the logs and see if there is something pointing to > > the problem. > > It's strange. One thing I can imagine is a mis-detection of early sections. > However, I don't see that happening: > > In sparse_init_nid(), we: > 1. Initialize the memmap > 2. Set SECTION_IS_EARLY | SECTION_HAS_MEM_MAP via > sparse_init_one_section() > > Only hotplugged sections (DIMMs, dax/kmem) set SECTION_HAS_MEM_MAP without > SECTION_IS_EARLY - which is correct, because these are not early. > > So once we know that we have valid_section() -- SECTION_HAS_MEM_MAP is set > -- early_section() should be correct. > > Even if someone would be doing a pfn_valid() after > memblocks_present()->memory_present() but before > sparse_init_nid(), we should be fine (!valid_section() -> return 0). I couldn't figure out how this could fail with Anshuman's patches. Will's suspicion is that some invalid/null pointer gets dereferenced before being initialised but the only case I see is somewhere in pfn_section_valid() (ms->usage) if valid_section() && !early_section(). Assuming that we do get a valid_section(ms) && !early_section(ms), is there a case where ms->usage is not initialised? I guess races with section_deactivate() are not possible this early. Another situation could be that pfn_valid() returns true when no memory is mapped for that pfn. > As it happens early during boot, I doubt that some NVDIMMs that get detected > and added early during boot as system RAM (via dax/kmem) are the problem. It is indeed very early, we can't even get the early console output. Debugging this is even harder as it's only misbehaving on a board we don't have access to. On the logic in this patch, is the hot-added memory always covering a full subsection? The arm64 pfn_valid() currently relies on memblock_is_map_memory() but the patch changes it to pfn_section_valid(). So if hot-added memory doesn't cover the full subsection, it may return true even if the pfn is not mapped. Regarding the robustness of the pfn_valid for ZONE_DEVICE memory, could we instead have a SECTION_IS_DEVICE flag and only check for that as not to disturb the hotplugged memory check via memblock_is_map_memory()?
On 03.03.21 20:04, Catalin Marinas wrote: > On Thu, Feb 11, 2021 at 01:35:56PM +0100, David Hildenbrand wrote: >> On 11.02.21 13:10, Anshuman Khandual wrote: >>> On 2/11/21 5:23 PM, Will Deacon wrote: >>>> ... and dropped. These patches appear to be responsible for a boot >>>> regression reported by CKI: >>> >>> Ahh, boot regression ? These patches only change the behaviour >>> for non boot memory only. >>> >>>> https://lore.kernel.org/r/cki.8D1CB60FEC.K6NJMEFQPV@redhat.com >>> >>> Will look into the logs and see if there is something pointing to >>> the problem. >> >> It's strange. One thing I can imagine is a mis-detection of early sections. >> However, I don't see that happening: >> >> In sparse_init_nid(), we: >> 1. Initialize the memmap >> 2. Set SECTION_IS_EARLY | SECTION_HAS_MEM_MAP via >> sparse_init_one_section() >> >> Only hotplugged sections (DIMMs, dax/kmem) set SECTION_HAS_MEM_MAP without >> SECTION_IS_EARLY - which is correct, because these are not early. >> >> So once we know that we have valid_section() -- SECTION_HAS_MEM_MAP is set >> -- early_section() should be correct. >> >> Even if someone would be doing a pfn_valid() after >> memblocks_present()->memory_present() but before >> sparse_init_nid(), we should be fine (!valid_section() -> return 0). > > I couldn't figure out how this could fail with Anshuman's patches. > Will's suspicion is that some invalid/null pointer gets dereferenced > before being initialised but the only case I see is somewhere in > pfn_section_valid() (ms->usage) if valid_section() && !early_section(). Indeed, it looks like a latent bug. > > Assuming that we do get a valid_section(ms) && !early_section(ms), is > there a case where ms->usage is not initialised? I guess races with > section_deactivate() are not possible this early. > Do you have access to that machine? We could identify which path is taken quite easily. > Another situation could be that pfn_valid() returns true when no memory > is mapped for that pfn. > >> As it happens early during boot, I doubt that some NVDIMMs that get detected >> and added early during boot as system RAM (via dax/kmem) are the problem. > > It is indeed very early, we can't even get the early console output. So even before any hotplug really happens. All sections should be early at that point I guess. > Debugging this is even harder as it's only misbehaving on a board we > don't have access to. > > On the logic in this patch, is the hot-added memory always covering a > full subsection? The arm64 pfn_valid() currently relies on > memblock_is_map_memory() but the patch changes it to > pfn_section_valid(). So if hot-added memory doesn't cover the full > subsection, it may return true even if the pfn is not mapped. Hotplugged System RAM always covers full sections. Hotplugged ZONE_DEVICE always covers full subsections. pfn_section_valid() properly handles both cases. (see generic pfn_valid())
On Wed, Mar 03, 2021 at 07:04:33PM +0000, Catalin Marinas wrote: > On Thu, Feb 11, 2021 at 01:35:56PM +0100, David Hildenbrand wrote: > > On 11.02.21 13:10, Anshuman Khandual wrote: > > > On 2/11/21 5:23 PM, Will Deacon wrote: > > > > ... and dropped. These patches appear to be responsible for a boot > > > > regression reported by CKI: > > > > > > Ahh, boot regression ? These patches only change the behaviour > > > for non boot memory only. > > > > > > > https://lore.kernel.org/r/cki.8D1CB60FEC.K6NJMEFQPV@redhat.com > > > > > > Will look into the logs and see if there is something pointing to > > > the problem. > > > > It's strange. One thing I can imagine is a mis-detection of early sections. > > However, I don't see that happening: > > > > In sparse_init_nid(), we: > > 1. Initialize the memmap > > 2. Set SECTION_IS_EARLY | SECTION_HAS_MEM_MAP via > > sparse_init_one_section() > > > > Only hotplugged sections (DIMMs, dax/kmem) set SECTION_HAS_MEM_MAP without > > SECTION_IS_EARLY - which is correct, because these are not early. > > > > So once we know that we have valid_section() -- SECTION_HAS_MEM_MAP is set > > -- early_section() should be correct. > > > > Even if someone would be doing a pfn_valid() after > > memblocks_present()->memory_present() but before > > sparse_init_nid(), we should be fine (!valid_section() -> return 0). > > I couldn't figure out how this could fail with Anshuman's patches. > Will's suspicion is that some invalid/null pointer gets dereferenced > before being initialised but the only case I see is somewhere in > pfn_section_valid() (ms->usage) if valid_section() && !early_section(). > > Assuming that we do get a valid_section(ms) && !early_section(ms), is > there a case where ms->usage is not initialised? I guess races with > section_deactivate() are not possible this early. > > Another situation could be that pfn_valid() returns true when no memory > is mapped for that pfn. The case I wondered about was __pfn_to_section() with a bogus pfn, since with patch 2/2 we call that *before* checking that pfn_to_section_nr() is sane. Will
On 3/4/21 2:54 AM, Will Deacon wrote: > On Wed, Mar 03, 2021 at 07:04:33PM +0000, Catalin Marinas wrote: >> On Thu, Feb 11, 2021 at 01:35:56PM +0100, David Hildenbrand wrote: >>> On 11.02.21 13:10, Anshuman Khandual wrote: >>>> On 2/11/21 5:23 PM, Will Deacon wrote: >>>>> ... and dropped. These patches appear to be responsible for a boot >>>>> regression reported by CKI: >>>> >>>> Ahh, boot regression ? These patches only change the behaviour >>>> for non boot memory only. >>>> >>>>> https://lore.kernel.org/r/cki.8D1CB60FEC.K6NJMEFQPV@redhat.com >>>> >>>> Will look into the logs and see if there is something pointing to >>>> the problem. >>> >>> It's strange. One thing I can imagine is a mis-detection of early sections. >>> However, I don't see that happening: >>> >>> In sparse_init_nid(), we: >>> 1. Initialize the memmap >>> 2. Set SECTION_IS_EARLY | SECTION_HAS_MEM_MAP via >>> sparse_init_one_section() >>> >>> Only hotplugged sections (DIMMs, dax/kmem) set SECTION_HAS_MEM_MAP without >>> SECTION_IS_EARLY - which is correct, because these are not early. >>> >>> So once we know that we have valid_section() -- SECTION_HAS_MEM_MAP is set >>> -- early_section() should be correct. >>> >>> Even if someone would be doing a pfn_valid() after >>> memblocks_present()->memory_present() but before >>> sparse_init_nid(), we should be fine (!valid_section() -> return 0). >> >> I couldn't figure out how this could fail with Anshuman's patches. >> Will's suspicion is that some invalid/null pointer gets dereferenced >> before being initialised but the only case I see is somewhere in >> pfn_section_valid() (ms->usage) if valid_section() && !early_section(). >> >> Assuming that we do get a valid_section(ms) && !early_section(ms), is >> there a case where ms->usage is not initialised? I guess races with >> section_deactivate() are not possible this early. >> >> Another situation could be that pfn_valid() returns true when no memory >> is mapped for that pfn. > > The case I wondered about was __pfn_to_section() with a bogus pfn, since > with patch 2/2 we call that *before* checking that pfn_to_section_nr() is > sane. Right, that is problematic. __pfn_to_section() should not be called without first validating pfn_to_section_nr(), as it could cause out-of-bound access on mem_section buffer. Will fix that order but as there is no test scenario which is definitive for this reported regression, how should we ensure that it fixes the problem ?
On 04.03.21 04:31, Anshuman Khandual wrote: > > > On 3/4/21 2:54 AM, Will Deacon wrote: >> On Wed, Mar 03, 2021 at 07:04:33PM +0000, Catalin Marinas wrote: >>> On Thu, Feb 11, 2021 at 01:35:56PM +0100, David Hildenbrand wrote: >>>> On 11.02.21 13:10, Anshuman Khandual wrote: >>>>> On 2/11/21 5:23 PM, Will Deacon wrote: >>>>>> ... and dropped. These patches appear to be responsible for a boot >>>>>> regression reported by CKI: >>>>> >>>>> Ahh, boot regression ? These patches only change the behaviour >>>>> for non boot memory only. >>>>> >>>>>> https://lore.kernel.org/r/cki.8D1CB60FEC.K6NJMEFQPV@redhat.com >>>>> >>>>> Will look into the logs and see if there is something pointing to >>>>> the problem. >>>> >>>> It's strange. One thing I can imagine is a mis-detection of early sections. >>>> However, I don't see that happening: >>>> >>>> In sparse_init_nid(), we: >>>> 1. Initialize the memmap >>>> 2. Set SECTION_IS_EARLY | SECTION_HAS_MEM_MAP via >>>> sparse_init_one_section() >>>> >>>> Only hotplugged sections (DIMMs, dax/kmem) set SECTION_HAS_MEM_MAP without >>>> SECTION_IS_EARLY - which is correct, because these are not early. >>>> >>>> So once we know that we have valid_section() -- SECTION_HAS_MEM_MAP is set >>>> -- early_section() should be correct. >>>> >>>> Even if someone would be doing a pfn_valid() after >>>> memblocks_present()->memory_present() but before >>>> sparse_init_nid(), we should be fine (!valid_section() -> return 0). >>> >>> I couldn't figure out how this could fail with Anshuman's patches. >>> Will's suspicion is that some invalid/null pointer gets dereferenced >>> before being initialised but the only case I see is somewhere in >>> pfn_section_valid() (ms->usage) if valid_section() && !early_section(). >>> >>> Assuming that we do get a valid_section(ms) && !early_section(ms), is >>> there a case where ms->usage is not initialised? I guess races with >>> section_deactivate() are not possible this early. >>> >>> Another situation could be that pfn_valid() returns true when no memory >>> is mapped for that pfn. >> >> The case I wondered about was __pfn_to_section() with a bogus pfn, since >> with patch 2/2 we call that *before* checking that pfn_to_section_nr() is >> sane. > > Right, that is problematic. __pfn_to_section() should not be called without > first validating pfn_to_section_nr(), as it could cause out-of-bound access > on mem_section buffer. Will fix that order but as there is no test scenario > which is definitive for this reported regression, how should we ensure that > it fixes the problem ? Oh, right, I missed that in patch #2. (and when comparing to generic pfn_valid()). I thought bisecting pointed at patch #1, that's why I didn't even have another look at patch #2. Makes sense.
On Thu, Mar 04, 2021 at 09:12:31AM +0100, David Hildenbrand wrote: > On 04.03.21 04:31, Anshuman Khandual wrote: > > On 3/4/21 2:54 AM, Will Deacon wrote: > > > On Wed, Mar 03, 2021 at 07:04:33PM +0000, Catalin Marinas wrote: > > > > On Thu, Feb 11, 2021 at 01:35:56PM +0100, David Hildenbrand wrote: > > > > > On 11.02.21 13:10, Anshuman Khandual wrote: > > > > > > On 2/11/21 5:23 PM, Will Deacon wrote: > > > > > > > ... and dropped. These patches appear to be responsible for a boot > > > > > > > regression reported by CKI: > > > > > > > > > > > > Ahh, boot regression ? These patches only change the behaviour > > > > > > for non boot memory only. > > > > > > > > > > > > > https://lore.kernel.org/r/cki.8D1CB60FEC.K6NJMEFQPV@redhat.com > > > > > > > > > > > > Will look into the logs and see if there is something pointing to > > > > > > the problem. > > > > > > > > > > It's strange. One thing I can imagine is a mis-detection of early sections. > > > > > However, I don't see that happening: > > > > > > > > > > In sparse_init_nid(), we: > > > > > 1. Initialize the memmap > > > > > 2. Set SECTION_IS_EARLY | SECTION_HAS_MEM_MAP via > > > > > sparse_init_one_section() > > > > > > > > > > Only hotplugged sections (DIMMs, dax/kmem) set SECTION_HAS_MEM_MAP without > > > > > SECTION_IS_EARLY - which is correct, because these are not early. > > > > > > > > > > So once we know that we have valid_section() -- SECTION_HAS_MEM_MAP is set > > > > > -- early_section() should be correct. > > > > > > > > > > Even if someone would be doing a pfn_valid() after > > > > > memblocks_present()->memory_present() but before > > > > > sparse_init_nid(), we should be fine (!valid_section() -> return 0). > > > > > > > > I couldn't figure out how this could fail with Anshuman's patches. > > > > Will's suspicion is that some invalid/null pointer gets dereferenced > > > > before being initialised but the only case I see is somewhere in > > > > pfn_section_valid() (ms->usage) if valid_section() && !early_section(). > > > > > > > > Assuming that we do get a valid_section(ms) && !early_section(ms), is > > > > there a case where ms->usage is not initialised? I guess races with > > > > section_deactivate() are not possible this early. > > > > > > > > Another situation could be that pfn_valid() returns true when no memory > > > > is mapped for that pfn. > > > > > > The case I wondered about was __pfn_to_section() with a bogus pfn, since > > > with patch 2/2 we call that *before* checking that pfn_to_section_nr() is > > > sane. > > > > Right, that is problematic. __pfn_to_section() should not be called without > > first validating pfn_to_section_nr(), as it could cause out-of-bound access > > on mem_section buffer. Will fix that order but as there is no test scenario > > which is definitive for this reported regression, how should we ensure that > > it fixes the problem ? > > Oh, right, I missed that in patch #2. (and when comparing to generic > pfn_valid()). > > I thought bisecting pointed at patch #1, that's why I didn't even have > another look at patch #2. Makes sense. I don't think we ever bisected it beyond these two patches, so it could be either of them. Anshuman -- please work with Veronika on this, as she has access to the problematic machine and was really helpful in debugging this last time. Will
On 3/4/21 3:06 PM, Will Deacon wrote: > On Thu, Mar 04, 2021 at 09:12:31AM +0100, David Hildenbrand wrote: >> On 04.03.21 04:31, Anshuman Khandual wrote: >>> On 3/4/21 2:54 AM, Will Deacon wrote: >>>> On Wed, Mar 03, 2021 at 07:04:33PM +0000, Catalin Marinas wrote: >>>>> On Thu, Feb 11, 2021 at 01:35:56PM +0100, David Hildenbrand wrote: >>>>>> On 11.02.21 13:10, Anshuman Khandual wrote: >>>>>>> On 2/11/21 5:23 PM, Will Deacon wrote: >>>>>>>> ... and dropped. These patches appear to be responsible for a boot >>>>>>>> regression reported by CKI: >>>>>>> >>>>>>> Ahh, boot regression ? These patches only change the behaviour >>>>>>> for non boot memory only. >>>>>>> >>>>>>>> https://lore.kernel.org/r/cki.8D1CB60FEC.K6NJMEFQPV@redhat.com >>>>>>> >>>>>>> Will look into the logs and see if there is something pointing to >>>>>>> the problem. >>>>>> >>>>>> It's strange. One thing I can imagine is a mis-detection of early sections. >>>>>> However, I don't see that happening: >>>>>> >>>>>> In sparse_init_nid(), we: >>>>>> 1. Initialize the memmap >>>>>> 2. Set SECTION_IS_EARLY | SECTION_HAS_MEM_MAP via >>>>>> sparse_init_one_section() >>>>>> >>>>>> Only hotplugged sections (DIMMs, dax/kmem) set SECTION_HAS_MEM_MAP without >>>>>> SECTION_IS_EARLY - which is correct, because these are not early. >>>>>> >>>>>> So once we know that we have valid_section() -- SECTION_HAS_MEM_MAP is set >>>>>> -- early_section() should be correct. >>>>>> >>>>>> Even if someone would be doing a pfn_valid() after >>>>>> memblocks_present()->memory_present() but before >>>>>> sparse_init_nid(), we should be fine (!valid_section() -> return 0). >>>>> >>>>> I couldn't figure out how this could fail with Anshuman's patches. >>>>> Will's suspicion is that some invalid/null pointer gets dereferenced >>>>> before being initialised but the only case I see is somewhere in >>>>> pfn_section_valid() (ms->usage) if valid_section() && !early_section(). >>>>> >>>>> Assuming that we do get a valid_section(ms) && !early_section(ms), is >>>>> there a case where ms->usage is not initialised? I guess races with >>>>> section_deactivate() are not possible this early. >>>>> >>>>> Another situation could be that pfn_valid() returns true when no memory >>>>> is mapped for that pfn. >>>> >>>> The case I wondered about was __pfn_to_section() with a bogus pfn, since >>>> with patch 2/2 we call that *before* checking that pfn_to_section_nr() is >>>> sane. >>> >>> Right, that is problematic. __pfn_to_section() should not be called without >>> first validating pfn_to_section_nr(), as it could cause out-of-bound access >>> on mem_section buffer. Will fix that order but as there is no test scenario >>> which is definitive for this reported regression, how should we ensure that >>> it fixes the problem ? >> >> Oh, right, I missed that in patch #2. (and when comparing to generic >> pfn_valid()). >> >> I thought bisecting pointed at patch #1, that's why I didn't even have >> another look at patch #2. Makes sense. > > I don't think we ever bisected it beyond these two patches, so it could > be either of them. Anshuman -- please work with Veronika on this, as she > has access to the problematic machine and was really helpful in debugging > this last time. Sure, will respin the patch series with a fix for [PATCH 2/2] as discussed and then follow up with Veronika to recreate the problem.
diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c index 709d98fea90c..1141075e4d53 100644 --- a/arch/arm64/mm/init.c +++ b/arch/arm64/mm/init.c @@ -230,6 +230,18 @@ int pfn_valid(unsigned long pfn) if (!valid_section(__pfn_to_section(pfn))) return 0; + + /* + * ZONE_DEVICE memory does not have the memblock entries. + * memblock_is_map_memory() check for ZONE_DEVICE based + * addresses will always fail. Even the normal hotplugged + * memory will never have MEMBLOCK_NOMAP flag set in their + * memblock entries. Skip memblock search for all non early + * memory sections covering all of hotplug memory including + * both normal and ZONE_DEVICE based. + */ + if (!early_section(__pfn_to_section(pfn))) + return pfn_section_valid(__pfn_to_section(pfn), pfn); #endif return memblock_is_map_memory(addr); }