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
_______________________________________________
[email protected] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "[email protected]"
