dynamically calculating NKPT [was: Re: huge ktr buffer]

[Andriy Gapon]

on 05/02/2013 01:05 Neel Natu said the following:
> Hi,
> 
> I have a patch to dynamically calculate NKPT for amd64 kernels. This
> should fix the various issues that people pointed out in the email
> thread.
> 
> Please review and let me know if there are any objections to committing this.
> 
> Also, thanks to Alan (alc@) for reviewing and providing feedback on
> the initial version of the patch.
> 
> Patch (also available at http://people.freebsd.org/~neel/patches/nkpt_diff.txt):

It seems I am a little bit late with a review, but not too late :-)
Some comments below:

> Index: sys/amd64/include/pmap.h
> ===================================================================
> --- sys/amd64/include/pmap.h	(revision 246277)
> +++ sys/amd64/include/pmap.h	(working copy)
> @@ -113,13 +113,7 @@
>  	((unsigned long)(l2) << PDRSHIFT) | \
>  	((unsigned long)(l1) << PAGE_SHIFT))
> 
> -/* Initial number of kernel page tables. */
> -#ifndef NKPT
> -#define	NKPT		32
> -#endif

I think that we could still keep this, if the below code is done slightly different:

[snip]
> +/* number of kernel PDP slots */
> +#define	NKPDPE(ptpgs)		howmany((ptpgs), NPDEPG)
> +
>  static void
> +nkpt_init(vm_paddr_t addr)
> +{
> +	int pt_pages;
> +	
> +#ifdef NKPT
> +	pt_pages = NKPT;
> +#else
> +	pt_pages = howmany(addr, 1 << PDRSHIFT);

A very minor cosmetic note: perhaps NBPDR would look more concise here.

> +	pt_pages += NKPDPE(pt_pages);
> +
> +	/*
> +	 * Add some slop beyond the bare minimum required for bootstrapping
> +	 * the kernel.
> +	 *
> +	 * This is quite important when allocating KVA for kernel modules.
> +	 * The modules are required to be linked in the negative 2GB of
> +	 * the address space.  If we run out of KVA in this region then
> +	 * pmap_growkernel() will need to allocate page table pages to map
> +	 * the entire 512GB of KVA space which is an unnecessary tax on
> +	 * physical memory.
> +	 */
> +	pt_pages += 4;		/* 8MB additional slop for kernel modules */
> +#endif
> +	nkpt = pt_pages;
> +}

I would slightly re-organize this code so that it uses NKPT, if defined, as a
default value for nkpt.  Then, only if the calculated value is greater then it
would override the default.
There are tradeoffs, of course.  So I am just voicing my opinion/preference.

The "slack" thing is a little bit imperfect, but I am not a perfectionist :-)

Thank you very much for this great feature.

> +static void
>  create_pagetables(vm_paddr_t *firstaddr)
>  {
> -	int i, j, ndm1g;
> +	int i, j, ndm1g, nkpdpe;
> 
> -	/* Allocate pages */
> -	KPTphys = allocpages(firstaddr, NKPT);
> -	KPML4phys = allocpages(firstaddr, 1);
> -	KPDPphys = allocpages(firstaddr, NKPML4E);
> -	KPDphys = allocpages(firstaddr, NKPDPE);
> -
> +	/* Allocate page table pages for the direct map */
>  	ndmpdp = (ptoa(Maxmem) + NBPDP - 1) >> PDPSHIFT;
>  	if (ndmpdp < 4)		/* Minimum 4GB of dirmap */
>  		ndmpdp = 4;
> @@ -517,6 +546,22 @@
>  		DMPDphys = allocpages(firstaddr, ndmpdp - ndm1g);
>  	dmaplimit = (vm_paddr_t)ndmpdp << PDPSHIFT;
> 
> +	/* Allocate pages */
> +	KPML4phys = allocpages(firstaddr, 1);
> +	KPDPphys = allocpages(firstaddr, NKPML4E);
> +
> +	/*
> +	 * Allocate the initial number of kernel page table pages required to
> +	 * bootstrap.  We defer this until after all memory-size dependent
> +	 * allocations are done (e.g. direct map), so that we don't have to
> +	 * build in too much slop in our estimate.
> +	 */
> +	nkpt_init(*firstaddr);
> +	nkpdpe = NKPDPE(nkpt);
> +
> +	KPTphys = allocpages(firstaddr, nkpt);
> +	KPDphys = allocpages(firstaddr, nkpdpe);
> +
>  	/* Fill in the underlying page table pages */
>  	/* Read-only from zero to physfree */
>  	/* XXX not fully used, underneath 2M pages */


-- 
Andriy Gapon