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

[David Hildenbrand]

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.

-- 

Thanks,

David / dhildenb