On 2012?02?29? 18:32, Catalin Marinas wrote:

On Mon, Feb 13, 2012 at 09:44:22AM +0000, Bill Carson wrote:

quoted

--- /dev/null
+++ b/arch/arm/include/asm/hugetlb.h

...

quoted

+#include<asm/page.h>
+#include<asm/pgtable-2level.h>

Just include asm/pgtable.h, it includes the right 2level.h file
automatically (and I plan to add LPAE support as well).

quoted

+static inline void set_huge_pte_at(struct mm_struct *mm, unsigned long addr,
+				   pte_t *ptep, pte_t pte)
+{
+	pgd_t *pgd;
+	pud_t *pud;
+	pmd_t *pmd;
+	pte_t *linuxpte = mm->context.huge_linux_pte;
+
+	BUG_ON(linuxpte == NULL);
+	BUG_ON(HUGE_LINUX_PTE_INDEX(addr)>= HUGE_LINUX_PTE_COUNT);
+	BUG_ON(ptep !=&linuxpte[HUGE_LINUX_PTE_INDEX(addr)]);
+
+	/* set huge linux pte first */
+	*ptep = pte;
+	
+	/* then set hardware pte */
+	addr&= HPAGE_MASK;
+	pgd = pgd_offset(mm, addr);
+	pud = pud_offset(pgd, addr);
+	pmd = pmd_offset(pud, addr);
+	set_hugepte_at(mm, addr, pmd, pte);

You may want to add a comment here that we only have two levels of page
tables (and there is no need for pud_none() checks).

I will add a comment to clarify this.

Also I would say the set_hugepte_at function name is easily confused
with set_huge_pte_at(), maybe change it to __set_huge_pte_at() or
something else.

yes, I will make that change.

quoted

+static inline pte_t huge_ptep_get_and_clear(struct mm_struct *mm,
+					    unsigned long addr, pte_t *ptep)
+{
+	pte_t pte = *ptep;
+	pte_t fake = L_PTE_YOUNG;
+	pgd_t *pgd;
+	pud_t *pud;
+	pmd_t *pmd;
+
+	/* clear linux pte */
+	*ptep = 0;
+
+	/* let set_hugepte_at clear HW entry */
+	addr&= HPAGE_MASK;
+	pgd = pgd_offset(mm, addr);
+	pud = pud_offset(pgd, addr);
+	pmd = pmd_offset(pud, addr);
+	set_hugepte_at(mm, addr, pmd, fake);

Same here.

quoted

+static inline void huge_ptep_set_wrprotect(struct mm_struct *mm,
+					   unsigned long addr, pte_t *ptep)
+{
+	pte_t old_pte = *ptep;
+	set_huge_pte_at(mm, addr, ptep, pte_wrprotect(old_pte));
+}

You could use the generic ptep_set_wrprotect()

I'm a bit of confused about this.

generic ptep_set_wrprotect() can not set huge pte, that's why
set_huge_pte_at is used instead here.

quoted

+static inline int huge_ptep_set_access_flags(struct vm_area_struct *vma,
+					     unsigned long addr,
+					     pte_t *ptep, pte_t pte,
+					     int dirty)
+{
+	int changed = !pte_same(huge_ptep_get(ptep), pte);
+	if (changed) {
+		set_huge_pte_at(vma->vm_mm, addr, ptep, pte);
+		huge_ptep_clear_flush(vma, addr,&pte);
+	}
+
+	return changed;
+}

I could also use the generic ptep_set_access_flags().

Same as above.

IMHO, cannot use generic hooks here, cause we are setting huge pte
with a different set pte API than set_pte_at.

quoted

--- /dev/null
+++ b/arch/arm/mm/hugetlb.c

...

quoted

+pte_t *huge_pte_alloc(struct mm_struct *mm, unsigned long addr,
+		      unsigned long sz)
+{
+	pte_t *linuxpte = mm->context.huge_linux_pte;
+	int index;
+
+	if (linuxpte == NULL) {
+		linuxpte = kzalloc(HUGE_LINUX_PTE_SIZE, GFP_ATOMIC);
+		if (linuxpte == NULL) {
+			printk(KERN_ERR "Cannot allocate memory for huge linux pte\n");

pr_err()?

Yes, pr_err should be used in here.
thanks

quoted

+			return NULL;
+		}
+		mm->context.huge_linux_pte = linuxpte;
+	}
+	/* huge page mapping only cover user space address */
+	BUG_ON(HUGE_LINUX_PTE_INDEX(addr)>= HUGE_LINUX_PTE_COUNT);
+	index = HUGE_LINUX_PTE_INDEX(addr);
+	return&linuxpte[HUGE_LINUX_PTE_INDEX(addr)];
+}
+
+pte_t *huge_pte_offset(struct mm_struct *mm, unsigned long addr)
+{
+	pgd_t *pgd;
+	pud_t *pud;
+	pmd_t *pmd = NULL;
+	pte_t *linuxpte = mm->context.huge_linux_pte;
+
+	/* check this mapping exist at pmd level */
+	pgd = pgd_offset(mm, addr);
+	if (pgd_present(*pgd)) {
+		pud = pud_offset(pgd, addr);
+		pmd = pmd_offset(pud, addr);
+		if (!pmd_present(*pmd))
+			return NULL;
+	}

You could add checks for the pud as well, they would be optimised out by
the compiler but it would be easier to add support for LPAE as well. In
my LPAE hugetlb implementation, I have something like this:

	pgd = pgd_offset(mm, addr);
	if (pgd_present(*pgd)) {
		pud = pud_offset(pgd, addr);
		if (pud_present(*pud))
			pmd = pmd_offset(pud, addr);
	}

Ok, I will add the pud checks as per your comment.

quoted

+	BUG_ON(HUGE_LINUX_PTE_INDEX(addr)>= HUGE_LINUX_PTE_COUNT);
+	BUG_ON((*pmd&  PMD_TYPE_MASK) != PMD_TYPE_SECT);
+	return&linuxpte[HUGE_LINUX_PTE_INDEX(addr)];

You could add a macro to make it easier for LPAE:

#define huge_pte(mm, pmd, addr)		\
	(&mm->context.huge_linux_pte(HUGE_LINUX_PTE_INDEX(addr)))

Nice, I will keep this in V3.

With LPAE, it would simply be a (pte_t *)pmd cast.

quoted

+int pud_huge(pud_t pud)
+{
+	return  0; } struct page * follow_huge_pmd(struct mm_struct *mm, unsigned long address, pmd_t *pmd, int write)

Something went wrong around here.

crap! I will make it cleaner next time. I promise!

quoted

+{
+	struct page *page = NULL;

You don't need to initialise page here.

OK, I will drop the "NULL".

quoted

+	unsigned long pfn;
+
+	BUG_ON((pmd_val(*pmd)&  PMD_TYPE_MASK) != PMD_TYPE_SECT);
+	pfn = ((pmd_val(*pmd)&  HPAGE_MASK)>>  PAGE_SHIFT);
+	page = pfn_to_page(pfn);
+	return page;
+}
+
+static int __init add_huge_page_size(unsigned long long size)
+{
+	int shift = __ffs(size);
+	u32 mmfr3 = 0;
+
+	/* Check that it is a page size supported by the hardware and
+	 * that it fits within pagetable and slice limits. */
+	if (!is_power_of_2(size) || (shift != HPAGE_SHIFT))
+		return -EINVAL;

You could use get_order() instead of __ffs(), the latter just finds the
first bit set.

With all due respect, I'm afraid I can't agree with you on this.
here, we should use __ffs to return this "shift" not the order.

For "hugepagesz=2M", hpage_shift/HPAGE_SHIFT should be set to 21,
*not* the order 9(21-12), that's what HUGETLB_PAGE_ORDER for.

But here you should have set hpage_shift.

-- 
I am a slow learner
but I will keep trying to fight for my dreams!

--bill