Re: [PATCH V2 2/2] arm64/mm: Enable memory hot remove

[Anshuman Khandual]

On 04/23/2019 01:21 PM, David Hildenbrand wrote:
> On 23.04.19 09:45, Anshuman Khandual wrote:
>>
>>
>> On 04/23/2019 01:07 PM, David Hildenbrand wrote:
>>> On 23.04.19 09:31, Anshuman Khandual wrote:
>>>>
>>>>
>>>> On 04/18/2019 10:58 AM, Anshuman Khandual wrote:
>>>>> On 04/17/2019 11:09 PM, Mark Rutland wrote:
>>>>>> On Wed, Apr 17, 2019 at 10:15:35PM +0530, Anshuman Khandual wrote:
>>>>>>> On 04/17/2019 07:51 PM, Mark Rutland wrote:
>>>>>>>> On Wed, Apr 17, 2019 at 03:28:18PM +0530, Anshuman Khandual wrote:
>>>>>>>>> On 04/15/2019 07:18 PM, Mark Rutland wrote:
>>>>>>>>>> On Sun, Apr 14, 2019 at 11:29:13AM +0530, Anshuman Khandual wrote:
>>>>>>
>>>>>>>>>>> +	spin_unlock(&init_mm.page_table_lock);
>>>>>>>>>>
>>>>>>>>>> What precisely is the page_table_lock intended to protect?
>>>>>>>>>
>>>>>>>>> Concurrent modification to kernel page table (init_mm) while clearing entries.
>>>>>>>>
>>>>>>>> Concurrent modification by what code?
>>>>>>>>
>>>>>>>> If something else can *modify* the portion of the table that we're
>>>>>>>> manipulating, then I don't see how we can safely walk the table up to
>>>>>>>> this point without holding the lock, nor how we can safely add memory.
>>>>>>>>
>>>>>>>> Even if this is to protect something else which *reads* the tables,
>>>>>>>> other code in arm64 which modifies the kernel page tables doesn't take
>>>>>>>> the lock.
>>>>>>>>
>>>>>>>> Usually, if you can do a lockless walk you have to verify that things
>>>>>>>> didn't change once you've taken the lock, but we don't follow that
>>>>>>>> pattern here.
>>>>>>>>
>>>>>>>> As things stand it's not clear to me whether this is necessary or
>>>>>>>> sufficient.
>>>>>>>
>>>>>>> Hence lets take more conservative approach and wrap the entire process of
>>>>>>> remove_pagetable() under init_mm.page_table_lock which looks safe unless
>>>>>>> in the worst case when free_pages() gets stuck for some reason in which
>>>>>>> case we have bigger memory problem to deal with than a soft lock up.
>>>>>>
>>>>>> Sorry, but I'm not happy with _any_ solution until we understand where
>>>>>> and why we need to take the init_mm ptl, and have made some effort to
>>>>>> ensure that the kernel correctly does so elsewhere. It is not sufficient
>>>>>> to consider this code in isolation.
>>>>>
>>>>> We will have to take the kernel page table lock to prevent assumption regarding
>>>>> present or future possible kernel VA space layout. Wrapping around the entire
>>>>> remove_pagetable() will be at coarse granularity but I dont see why it should
>>>>> not sufficient atleast from this particular tear down operation regardless of
>>>>> how this might affect other kernel pgtable walkers.
>>>>>
>>>>> IIUC your concern is regarding other parts of kernel code (arm64/generic) which
>>>>> assume that kernel page table wont be changing and hence they normally walk the
>>>>> table without holding pgtable lock. Hence those current pgtabe walker will be
>>>>> affected after this change.
>>>>>
>>>>>>
>>>>>> IIUC, before this patch we never clear non-leaf entries in the kernel
>>>>>> page tables, so readers don't presently need to take the ptl in order to
>>>>>> safely walk down to a leaf entry.
>>>>>
>>>>> Got it. Will look into this.
>>>>>
>>>>>>
>>>>>> For example, the arm64 ptdump code never takes the ptl, and as of this
>>>>>> patch it will blow up if it races with a hot-remove, regardless of
>>>>>> whether the hot-remove code itself holds the ptl.
>>>>>
>>>>> Got it. Are there there more such examples where this can be problematic. I
>>>>> will be happy to investigate all such places and change/add locking scheme
>>>>> in there to make them work with memory hot remove.
>>>>>
>>>>>>
>>>>>> Note that the same applies to the x86 ptdump code; we cannot assume that
>>>>>> just because x86 does something that it happens to be correct.
>>>>>
>>>>> I understand. Will look into other non-x86 platforms as well on how they are
>>>>> dealing with this.
>>>>>
>>>>>>
>>>>>> I strongly suspect there are other cases that would fall afoul of this,
>>>>>> in both arm64 and generic code.
>>>>
>>>> On X86
>>>>
>>>> - kernel_physical_mapping_init() takes the lock for pgtable page allocations as well
>>>>   as all leaf level entries including large mappings.
>>>>
>>>> On Power
>>>>
>>>> - remove_pagetable() take an overall high level init_mm.page_table_lock as I had
>>>>   suggested before. __map_kernel_page() calls [pud|pmd|pte]_alloc_[kernel] which
>>>>   allocates page table pages are protected with init_mm.page_table_lock but then
>>>>   the actual setting of the leaf entries are not (unlike x86)
>>>>
>>>> 	arch_add_memory()
>>>> 		create_section_mapping()
>>>> 			radix__create_section_mapping()
>>>> 				create_physical_mapping()
>>>> 					__map_kernel_page()
>>>> On arm64.
>>>>
>>>> Both kernel page table dump and linear mapping (__create_pgd_mapping on init_mm)
>>>> will require init_mm.page_table_lock to be really safe against this new memory
>>>> hot remove code. I will do the necessary changes as part of this series next time
>>>> around. IIUC there is no equivalent generic feature for ARM64_PTDUMP_CORE/DEBUGFS.
>>>> 	 > 
>>>>> Will start looking into all such possible cases both on arm64 and generic.
>>>>> Mean while more such pointers would be really helpful.
>>>>
>>>> Generic usage for init_mm.pagetable_lock
>>>>
>>>> Unless I have missed something else these are the generic init_mm kernel page table
>>>> modifiers at runtime (at least which uses init_mm.page_table_lock)
>>>>
>>>> 	1. ioremap_page_range()		/* Mapped I/O memory area */
>>>> 	2. apply_to_page_range()	/* Change existing kernel linear map */
>>>> 	3. vmap_page_range()		/* Vmalloc area */
>>>>
>>>> A. IOREMAP
>>>>
>>>> ioremap_page_range()
>>>> 	ioremap_p4d_range()
>>>> 		p4d_alloc()
>>>> 		ioremap_try_huge_p4d() -> p4d_set_huge() -> set_p4d()
>>>> 		ioremap_pud_range()
>>>> 			pud_alloc()
>>>> 			ioremap_try_huge_pud() -> pud_set_huge() -> set_pud()
>>>> 			ioremap_pmd_range()
>>>> 				pmd_alloc()
>>>> 				ioremap_try_huge_pmd() -> pmd_set_huge() -> set_pmd()
>>>> 				ioremap_pte_range()
>>>> 					pte_alloc_kernel()
>>>> 						set_pte_at() -> set_pte()
>>>> B. APPLY_TO_PAGE_RANGE
>>>>
>>>> apply_to_page_range()
>>>> 	apply_to_p4d_range()
>>>> 		p4d_alloc()
>>>> 		apply_to_pud_range()
>>>> 			pud_alloc()
>>>> 			apply_to_pmd_range()
>>>> 				pmd_alloc()
>>>> 				apply_to_pte_range()
>>>> 					pte_alloc_kernel()
>>>>
>>>> C. VMAP_PAGE_RANGE
>>>>
>>>> vmap_page_range()
>>>> vmap_page_range_noflush()
>>>> 	vmap_p4d_range()
>>>> 		p4d_alloc()
>>>> 		vmap_pud_range()
>>>> 			pud_alloc()
>>>> 			vmap_pmd_range()
>>>> 				pmd_alloc()
>>>> 				vmap_pte_range()
>>>> 					pte_alloc_kernel()
>>>> 					set_pte_at()
>>>>
>>>> In all of the above.
>>>>
>>>> - Page table pages [p4d|pud|pmd|pte]_alloc_[kernel] settings are protected with init_mm.page_table_lock
>>>> - Should not it require init_mm.page_table_lock for all leaf level (PUD|PMD|PTE) modification as well ?
>>>> - Should not this require init_mm.page_table_lock for page table walk itself ?
>>>>
>>>> Not taking an overall lock for all these three operations will potentially race with an ongoing memory
>>>> hot remove operation which takes an overall lock as proposed. Wondering if this has this been safe till
>>>> now ?
>>>>
>>>
>>> All memory add/remove operations are currently guarded by
>>> mem_hotplug_lock as far as I know.
>>
>> Right but it seems like it guards against concurrent memory hot add or remove operations with
>> respect to memory block, sections, sysfs etc. But does it cover with respect to other init_mm
>> modifiers or accessors ?
>>
> 
> Don't think so, it's purely for memory add/remove via
> add_memory/remove_memory/devm_memremap_pages, not anything beyond that.
> Whoever uses get_online_mems/put_online_mems is save in respect to that
> - mostly slab/slub.

There are two distinct locking requirements here

* Protect against clearing of intermediate pgtable pages to prevent unhandled aborts while walking
  the kernel page table.

	- pgtable page clearing happens only through memory hot-remove not with any other modifiers
	- At minimum we would require get_online_mems() for all concurrent walkers of kernel page table

		- Kernel page table dump on arm64 (with ARM64_PTDUMP_CORE_[DEBUGFS])
		- Existing generic MM modifiers. Without get_online_mems() not sure how these can be
		  really safe against memory hot-remove.

			1. ioremap_page_range()
			2. apply_to_page_range()
			3. vmap_page_range()

* Protect against each other's accesses (apart from get_online_mems)

	- Take overall init_mm.page_table_lock for [ioremap|apply_to|vmap]_page_range()
	- Take overall lock while creating/destroying linear mapping on each platform
	- Hotplug lock through get_online_mems only protects against clearing/adding of intermediate pgtable
	  pages not general modifications by non-hotplug accessors.

Will appreciate if folks could review the assumptions made above as I might have missed something.