Messages in this thread | | | Date | Wed, 16 Jul 2003 13:31:39 +0100 (BST) | From | Hugh Dickins <> | Subject | Re: Question about free_one_pgd() changes in these 3.5G patches |
| |
On Tue, 15 Jul 2003, Ron Niles wrote: > > I'm trying to expand my kernel memory space to 3GB and am checking out these > patches. I'm kind of mystified by the changes to free_one_pgd in these > patches, both the one that Chuck Luciano recently posted and the one in AA > (00_3.5G-address-space-4). Both of these seem to change the loop from: > > int j; > ... > > for (j = 0; j < PTRS_PER_PMD ; j++) { > prefetchw(pmd+j+(PREFETCH_STRIDE/16)); > free_one_pmd(pmd+j); > } > to: > > pmd_t * pmd, * md, * emd; > ... > > /* > * Beware if changing the loop below. It once used int j, > * for (j = 0; j < PTRS_PER_PMD; j++) > * free_one_pmd(pmd+j); > * but some older i386 compilers (e.g. egcs-2.91.66, gcc-2.95.3) > * terminated the loop with a _signed_ address comparison > * using "jle", when configured for HIGHMEM64GB (X86_PAE). > * If also configured for 3GB of kernel virtual address space, > * if page at physical 0x3ffff000 virtual 0x7ffff000 is used as > * a pmd, when that mm exits the loop goes on to free "entries" > * found at 0x80000000 onwards. The loop below compiles instead > * to be terminated by unsigned address comparison using "jb". > > for (md = pmd, emd = pmd + PTRS_PER_PMD; md < emd; md++) { > prefetchw(md+(PREFETCH_STRIDE/16)); > free_one_pmd(md); > } > > The comment (found in the AA patch) makes no sense to me. Since j is an int, > you would expect the loop to exit with jle. If you want it to exit on jb, > just change j to unsigned, right? Also PTRS_PER_PMD is never very large, > around 512 I think, so it really doesn't matter unless PTRS_PER_PMD exceeds > 0x7fffffff, which is really far from reality.
That comment (and the rewritten loop) originally came from me. I thought it was a champion comment, I'm saddened that you disagree!
I've tried to cover the point by saying they terminated the loop with "a _signed_ address comparison": the loop got optimized in such a way that it wasn't testing int j as the C shows, but the address pmd+j.
Even so, it's conceivable that your proposed change, to unsigned j, might be enough to jolt those compilers into doing the right thing. But I never tried that, preferring to code the pointers explicitly.
> Secondly, the new code is dangerous if the pmd happens to be on the page at > the top of memory. In this case pmd is something like 0xfffff000, emd = pmd > + PTRS_PER_PMD rolls over to zero, and the loop never gets executed since md > is never less than zero. > > It seems to me the change is unnecessary, but if it is needed, it should > protect against rollover on the top of memory page, assuming PTRS_PER_PMD is > never zero: > > for (md = pmd, emd = pmd + PTRS_PER_PMD - 1; md <= emd; md++) > > Do we guarantee that the top of memory page is never used and the rollover > is impossible? Even so, am I missing something as to why this change is > necessary?
It is the case that we don't use the page-worth of virtual addresses at the very top of virtual memory (I'm trying to phrase that pedantically to make the point that it is virtual not physical addresses relevant here).
I know that to be true of i386, I believe it to be true of all arches, you're right to pose the question.
In 2.5 Linus did very briefly use it for the FIX_VSYSCALL page, but quickly accepted the value of keeping that page unmapped, and it's now dignified as FIX_HOLE. Not that we'd be using the fixmap area for pmds, which have to come from directly mapped lowmem (or be temporarily kmapped in wli's highmem pmd patch in -mm).
Hugh
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |