Re: Linux 5.0-rc1 (test results)

[Guo Ren]

Hi Linus,
On Mon, Jan 07, 2019 at 03:21:45PM -0800, Linus Torvalds wrote:
> On Mon, Jan 7, 2019 at 11:26 AM Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
> >
> > Bisect points to commit 4cf58924951ef ("mm: treewide: remove unused address
> > argument from pte_alloc functions"). Interesting - wasn't that supposed
> > to be automatic ?
> >
> > csky does use the the removed address argument, so I won't even try to
> > provide a patch. Copying csky maintainer instead.
> 
> Hmm. Interesting. The csky code seems to have some odd "poison pte
> contents with ones if the address has the high bit set".
PTE contents is the only _PAGE_GLOBAL bit which could let mmu ignore the
ASID. One tlb entry is composed of 2 pfns and there is one GLOBAL bit in
the tlb entry. When C-SKY MMU hard-refill a tlb entry into the TLB buffer,
it will get pfn0-GLOBAL & pfn1-GLOBAL from the memory and put the result
into the TLB buffer.

If pfn0 is valid & pfn1 is invalid, we also must keep invalid pte_t with GLOBAL
bit set for C-SKY MMU.

Also see pte_clear() and pte_none() in pgtable.h.

> 
> Which makes little or no sense. The "high bit set" case is for kernel
> page tables, but that's exactly the "pte_alloc_one()" vs
> "pte_alloc_one_kernel()" distinction.
> 
> So testing the address seems entirely wrong.
Yes, testing address is no necessary, I'll remove it.

> 
> But there's other strangeness in there too. For example,
> pte_alloc_one_kernel() will just write directly to the page. And
> pte_alloc_one() will do a "kmap_atomic()" on the page it allocates,
> except since it uses GFP_KERNEL, that's entirely pointless.
> 
> Is the alloc_pages() in pte_alloc_one() perhaps meant to use
> GFP_HIGHUSER instead?
No GFP_HIGHUSER, kmap_atomic should be removed and I'll use __GFP_ZERO
instead.

> Is this perhaps some copy-paste issue?
:P

> 
> So I *think* the removal of the 'address' use in csky should be
> simple, but yes, this needs a csky maintainer to look at.
> 
Here is my new implementation:

static inline pte_t *pte_alloc_one_kernel(struct mm_struct *mm)
{
	pte_t *pte;
	unsigned long i;

	pte = (pte_t *) __get_free_page(GFP_KERNEL | __GFP_RETRY_MAYFAIL);
						     ^^^^^^^^^^^^^^^^^^^
						     It's necessary ?
						     x86 & arm don't use
						     it.
	if (!pte)
		return NULL;

	for (i = 0; i < PAGE_SIZE/sizeof(pte_t); i++)
		(pte + i)->pte_low = _PAGE_GLOBAL;

	return pte;
}

static inline struct page *pte_alloc_one(struct mm_struct *mm)
{
	struct page *pte;

	pte = alloc_pages(GFP_KERNEL | __GFP_RETRY_MAYFAIL | __GFP_ZERO, 0);
	if (!pte)
		return NULL;

	if (!pgtable_page_ctor(pte)) {
		__free_page(pte);
		return NULL;
	}

	return pte;
}

Best Regards
 Guo Ren