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

[Zachary Amsden <zach@vmware.com>]

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


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

Zach