lkml.org 
[lkml]   [2003]   [Jul]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: 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/

\
 
 \ /
  Last update: 2005-03-22 13:46    [W:0.072 / U:0.160 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site