Re: [PATCH 1/5] Add pagetable allocation notifiers

[Chris Wright <chrisw@osdl.org>]

* Zachary Amsden (zach@vmware.com) wrote:
> Chris Wright wrote:
> 
> >* Zachary Amsden (zach@vmware.com) wrote:
> > 
> >
> >>--- linux-2.6.13.orig/arch/i386/mm/init.c	2005-08-24 
> >>09:31:05.000000000 -0700
> >>+++ linux-2.6.13/arch/i386/mm/init.c	2005-08-24 09:31:31.000000000 -0700
> >>@@ -79,6 +79,7 @@ static pte_t * __init one_page_table_ini
> >>{
> >>	if (pmd_none(*pmd)) {
> >>		pte_t *page_table = (pte_t *) 
> >>		alloc_bootmem_low_pages(PAGE_SIZE);
> >>+		SetPagePTE(virt_to_page(page_table));
> >>   
> >>
> >
> >Xen has this on one_md_table_init() as well for pmd.
> 
> I'll add that in another patch.  It's easy to miss some of the init time 
> call sites (we don't actually depend on them for correctness).

You could just respin this patch.

> >>	spin_lock_irqsave(&pgd_lock, flags);
> >>	pgd_list_del(pgd);
> >>	spin_unlock_irqrestore(&pgd_lock, flags);
> >>@@ -244,13 +246,16 @@ pgd_t *pgd_alloc(struct mm_struct *mm)
> >>		pmd_t *pmd = kmem_cache_alloc(pmd_cache, GFP_KERNEL);
> >>		if (!pmd)
> >>			goto out_oom;
> >>+		SetPagePDE(virt_to_page(pmd));
> >>		set_pgd(&pgd[i], __pgd(1 + __pa(pmd)));
> >>	}
> >>	return pgd;
> >>
> >>out_oom:
> >>-	for (i--; i >= 0; i--)
> >>+	for (i--; i >= 0; i--) {
> >>+		ClearPagePDE(pfn_to_page(pgd_val(pgd[i]) >> PAGE_SHIFT));
> >>   
> >>
> >
> >Is that the right pfn?  That -1 throws me off.
> >
> > 
> >
> >>		kmem_cache_free(pmd_cache, (void *)__va(pgd_val(pgd[i])-1));
> >>+	}
> >>	kmem_cache_free(pgd_cache, pgd);
> >>	return NULL;
> >>}
> >>@@ -261,8 +266,10 @@ void pgd_free(pgd_t *pgd)
> >>
> >>	/* in the PAE case user pgd entries are overwritten before usage */
> >>	if (PTRS_PER_PMD > 1)
> >>-		for (i = 0; i < USER_PTRS_PER_PGD; ++i)
> >>-			kmem_cache_free(pmd_cache, (void 
> >>*)__va(pgd_val(pgd[i])-1));
> >>+		for (i = 0; i < USER_PTRS_PER_PGD; ++i) {
> >>+			ClearPagePDE(pfn_to_page(pgd_val(pgd[i]) >> 
> >>PAGE_SHIFT));
> >>+			kmem_cache_free(pmd_cache, (void 
> >>*)__va(pgd_val(pgd[i]) & PAGE_MASK));
> >>   
> >>
> >
> >Why the switch of kmem_cache_free call?
> 
> Because pgd_val(pgd[i])-1 is confusing.

Yes, I agree it is.  That's why I was wondering about the ClearPagePDE calls
which don't use them.

> Using (pgd_val(pgd[i]) - 
> _PAGE_PRESENT) would be better, but the +/- 1s all over the place here 
> could use some general cleanup as well.  I smell a cleanup fit coming 
> on.  Using (pgd_val(pgd[i]) & PAGE_MASK) is a less error prone way to 
> get the physical frame bits, since it is not wrong if you turn on PCD or 
> PWD.

Please save all that for later cleanup.  As it stands it's just a conversion
of one random call site, which should be dropped from the patch.

thanks,
-chris