Re: [PATCH v5 3/4] powerpc/mm: add radix__remove_section_mapping()

[Reza Arbab <arbab@linux.vnet.ibm.com>]

On Tue, Jan 17, 2017 at 12:52:51PM +0530, Balbir Singh wrote:
>Shouldn't most of these functions have __meminit?

I don't think so. The mapping functions are __meminit, but the unmapping 
functions are completely within #ifdef CONFIG_MEMORY_HOTPLUG already.

>On Mon, Jan 16, 2017 at 01:07:45PM -0600, Reza Arbab wrote:
>>  #ifdef CONFIG_MEMORY_HOTPLUG
>> +static void free_pte_table(pte_t *pte_start, pmd_t *pmd)
>> +{
>> +	pte_t *pte;
>> +	int i;
>> +
>> +	for (i = 0; i < PTRS_PER_PTE; i++) {
>> +		pte = pte_start + i;
>> +		if (!pte_none(*pte))
>> +			return;
>
>If !pte_none() we fail the hotplug? Or silently
>leave the allocated pte's around. I guess this is
>the same as x86

The latter--it's not a failure. If you provided remove_pagetable() an 
unaligned address range, there could be a pte left unremoved at either 
end.

>> +static void remove_pmd_table(pmd_t *pmd_start, unsigned long addr,
>> +			     unsigned long end)
>> +{
>> +	unsigned long next;
>> +	pte_t *pte_base;
>> +	pmd_t *pmd;
>> +
>> +	pmd = pmd_start + pmd_index(addr);
>> +	for (; addr < end; addr = next, pmd++) {
>> +		next = pmd_addr_end(addr, end);
>> +
>> +		if (!pmd_present(*pmd))
>> +			continue;
>> +
>> +		if (pmd_huge(*pmd)) {
>> +			pte_clear(&init_mm, addr, (pte_t *)pmd);
>
>pmd_clear()?

I used pte_clear() to mirror what happens in radix__map_kernel_page():

		if (map_page_size == PMD_SIZE) {
			ptep = (pte_t *)pmdp;
			goto set_the_pte;
		}

		[...]

	set_the_pte:
		set_pte_at(&init_mm, ea, ptep, pfn_pte(pa >> PAGE_SHIFT, flags));

Would pmd_clear() be equivalent, since the pointer got set like a pte?

>> +static void remove_pagetable(unsigned long start, unsigned long end)
>> +{
>> +	unsigned long addr, next;
>> +	pud_t *pud_base;
>> +	pgd_t *pgd;
>> +
>> +	spin_lock(&init_mm.page_table_lock);
>> +
>
>x86 does more granular lock acquisition only during
>clearing the relevant entries. I suppose we don't have
>to worry about it since its not fast path and frequent.

Yep. Ben thought the locking in remove_pte_table() was actually too 
granular, and Aneesh questioned what was being protected in the first 
place. So I left one lock/unlock in the outermost function for now.

-- 
Reza Arbab