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
