On Tue, Oct 08, 2019 at 10:06:26AM +0530, Anshuman Khandual wrote:

On 10/07/2019 07:47 PM, Catalin Marinas wrote:

quoted

On Mon, Sep 23, 2019 at 11:13:45AM +0530, Anshuman Khandual wrote:

quoted

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.

quoted

quoted

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

quoted

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

quoted

quoted

+	} while (addr += PAGE_SIZE, addr < end);
+}

[...]

quoted

+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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel