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

[Anshuman Khandual <anshuman.khandual@arm.com>]

On 10/08/2019 04:25 PM, Catalin Marinas wrote:
> On Tue, Oct 08, 2019 at 10:06:26AM +0530, Anshuman Khandual wrote:
>> On 10/07/2019 07:47 PM, Catalin Marinas wrote:
>>> On Mon, Sep 23, 2019 at 11:13:45AM +0530, Anshuman Khandual wrote:
>>>> The arch code for hot-remove must tear down portions of the linear map and
>>>> vmemmap corresponding to memory being removed. In both cases the page
>>>> tables mapping these regions must be freed, and when sparse vmemmap is in
>>>> use the memory backing the vmemmap must also be freed.
>>>>
>>>> This patch adds unmap_hotplug_range() and free_empty_tables() helpers which
>>>> can be used to tear down either region and calls it from vmemmap_free() and
>>>> ___remove_pgd_mapping(). The sparse_vmap argument determines whether the
>>>> backing memory will be freed.
>>>
>>> Can you change the 'sparse_vmap' name to something more meaningful which
>>> would suggest freeing of the backing memory?
>>
>> free_mapped_mem or free_backed_mem ? Even shorter forms like free_mapped or
>> free_backed might do as well. Do you have a particular preference here ? But
>> yes, sparse_vmap has been very much specific to vmemmap for these functions
>> which are now very generic in nature.
> 
> free_mapped would do.

Sure.

> 
>>>> +static void unmap_hotplug_pte_range(pmd_t *pmdp, unsigned long addr,
>>>> +				    unsigned long end, bool sparse_vmap)
>>>> +{
>>>> +	struct page *page;
>>>> +	pte_t *ptep, pte;
>>>> +
>>>> +	do {
>>>> +		ptep = pte_offset_kernel(pmdp, addr);
>>>> +		pte = READ_ONCE(*ptep);
>>>> +		if (pte_none(pte))
>>>> +			continue;
>>>> +
>>>> +		WARN_ON(!pte_present(pte));
>>>> +		page = sparse_vmap ? pte_page(pte) : NULL;
>>>> +		pte_clear(&init_mm, addr, ptep);
>>>> +		flush_tlb_kernel_range(addr, addr + PAGE_SIZE);
>>>> +		if (sparse_vmap)
>>>> +			free_hotplug_page_range(page, PAGE_SIZE);
>>>
>>> You could only set 'page' if sparse_vmap (or even drop 'page' entirely).
>>
>> I am afraid 'page' is being used to hold pte_page(pte) extraction which
>> needs to be freed (sparse_vmap) as we are going to clear the ptep entry
>> in the next statement and lose access to it for good.
> 
> You clear *ptep, not pte.

Ahh, missed that. We have already captured the contents with READ_ONCE().

> 
>> We will need some
>> where to hold onto pte_page(pte) across pte_clear() as we cannot free it
>> before clearing it's entry and flushing the TLB. Hence wondering how the
>> 'page' can be completely dropped.
>>
>>> The compiler is probably smart enough to optimise it but using a
>>> pointless ternary operator just makes the code harder to follow.
>>
>> Not sure I got this but are you suggesting for an 'if' statement here
>>
>> if (sparse_vmap)
>> 	page = pte_page(pte);
>>
>> instead of the current assignment ?
>>
>> page = sparse_vmap ? pte_page(pte) : NULL;
> 
> I suggest:
> 
> 	if (sparse_vmap)
> 		free_hotplug_pgtable_page(pte_page(pte), PAGE_SIZE);

Sure, will do.

> 
>>>> +	} while (addr += PAGE_SIZE, addr < end);
>>>> +}
>>> [...]
>>>> +static void free_empty_pte_table(pmd_t *pmdp, unsigned long addr,
>>>> +				 unsigned long end)
>>>> +{
>>>> +	pte_t *ptep, pte;
>>>> +
>>>> +	do {
>>>> +		ptep = pte_offset_kernel(pmdp, addr);
>>>> +		pte = READ_ONCE(*ptep);
>>>> +		WARN_ON(!pte_none(pte));
>>>> +	} while (addr += PAGE_SIZE, addr < end);
>>>> +}
>>>> +
>>>> +static void free_empty_pmd_table(pud_t *pudp, unsigned long addr,
>>>> +				 unsigned long end, unsigned long floor,
>>>> +				 unsigned long ceiling)
>>>> +{
>>>> +	unsigned long next;
>>>> +	pmd_t *pmdp, pmd;
>>>> +
>>>> +	do {
>>>> +		next = pmd_addr_end(addr, end);
>>>> +		pmdp = pmd_offset(pudp, addr);
>>>> +		pmd = READ_ONCE(*pmdp);
>>>> +		if (pmd_none(pmd))
>>>> +			continue;
>>>> +
>>>> +		WARN_ON(!pmd_present(pmd) || !pmd_table(pmd) || pmd_sect(pmd));
>>>> +		free_empty_pte_table(pmdp, addr, next);
>>>> +		free_pte_table(pmdp, addr, next, floor, ceiling);
>>>
>>> Do we need two closely named functions here? Can you not collapse
>>> free_empty_pud_table() and free_pte_table() into a single one? The same
>>> comment for the pmd/pud variants. I just find this confusing.
>>
>> The two functions could be collapsed into a single one. But just wanted to
>> keep free_pxx_table() part which checks floor/ceiling alignment, non-zero
>> entries clear off the actual page table walking.
> 
> With the pmd variant, they both take the floor/ceiling argument while
> the free_empty_pte_table() doesn't even free anything. So not entirely
> consistent.> 
> Can you not just copy the free_pgd_range() functions but instead of
> p*d_free_tlb() just do the TLB invalidation followed by page freeing?
> That seems to be an easier pattern to follow.
> 

Sure, will follow that pattern.