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

[Catalin Marinas <catalin.marinas@arm.com>]

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.

> >> +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.

> 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);

> >> +	} 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.

-- 
Catalin