On Sun, Jan 07, 2018 at 06:33:17PM +0800, Jike Song wrote:
Look at one of the code snippets:
162 if (pgd_none(*pgd)) { 163 unsigned long new_p4d_page = __get_free_page(gfp); 164 if (!new_p4d_page) 165 return NULL; 166 167 if (pgd_none(*pgd)) { 168 set_pgd(pgd, __pgd(_KERNPG_TABLE | __pa(new_p4d_page))); 169 new_p4d_page = 0; 170 } 171 if (new_p4d_page) 172 free_page(new_p4d_page); 173 }There can't be any difference between two pgd_none(*pgd) at L162 and L167, so it's always false at L171.
I think this is a remnant from the kaiser version which did this:
if (pud_none(*pud)) { unsigned long new_pmd_page = __get_free_page(gfp); if (!new_pmd_page) return NULL; spin_lock(&shadow_table_allocation_lock); if (pud_none(*pud)) set_pud(pud, __pud(_KERNPG_TABLE | __pa(new_pmd_page))); else free_page(new_pmd_page); spin_unlock(&shadow_table_allocation_lock); }
I was wondering too, why the duplicated checks.
Which has this explanation about the need for the locking:
/* * At runtime, the only things we map are some things for CPU * hotplug, and stacks for new processes. No two CPUs will ever * be populating the same addresses, so we only need to ensure * that we protect between two CPUs trying to allocate and * populate the same page table page. * * Only take this lock when doing a set_p[4um]d(), but it is not * needed for doing a set_pte(). We assume that only the *owner* * of a given allocation will be doing this for _their_ * allocation. * * This ensures that once a system has been running for a while * and there have been stacks all over and these page tables * are fully populated, there will be no further acquisitions of * this lock. */ static DEFINE_SPINLOCK(shadow_table_allocation_lock);
Now I have my suspicions why that's not needed anymore upstream but I'd let tglx explain better.