pud_bad vs pud_bad

I'm looking at unifying the 32 and 64-bit versions of pud_bad.

32-bits defines it as:

static inline int pud_bad(pud_t pud)

{
	return (pud_val(pud) & ~(PTE_PFN_MASK | _KERNPG_TABLE | _PAGE_USER)) != 0;
}

and 64 as:

static inline int pud_bad(pud_t pud)

{
	return (pud_val(pud) & ~(PTE_PFN_MASK | _PAGE_USER)) != _KERNPG_TABLE;
}

I'm inclined to go with the 64-bit version, but I'm wondering if there's something subtle I'm missing here.

Quoted 17 lines · Jeremy Fitzhardinge

I'm looking at unifying the 32 and 64-bit versions of pud_bad.

32-bits defines it as:

static inline int pud_bad(pud_t pud)

{
	return (pud_val(pud) & ~(PTE_PFN_MASK | _KERNPG_TABLE | _PAGE_USER)) != 0;
}

and 64 as:

static inline int pud_bad(pud_t pud)

{
	return (pud_val(pud) & ~(PTE_PFN_MASK | _PAGE_USER)) != _KERNPG_TABLE;
}

Quoted 2 lines

I'm inclined to go with the 64-bit version, but I'm wondering if there's something subtle I'm missing here.

Why go with the 64-bit version? The 32-bit check looks more compact and should result in smaller code.

Quoted 24 lines · Ingo Molnar

Quoted 20 lines · Jeremy Fitzhardinge

I'm looking at unifying the 32 and 64-bit versions of pud_bad.

32-bits defines it as:

static inline int pud_bad(pud_t pud)

{
	return (pud_val(pud) & ~(PTE_PFN_MASK | _KERNPG_TABLE | _PAGE_USER)) != 0;
}

and 64 as:

static inline int pud_bad(pud_t pud)

{
	return (pud_val(pud) & ~(PTE_PFN_MASK | _PAGE_USER)) != _KERNPG_TABLE;
}

I'm inclined to go with the 64-bit version, but I'm wondering if there's something subtle I'm missing here.

Quoted 3 lines

Why go with the 64-bit version? The 32-bit check looks more compact and should result in smaller code.

Well, its stricter. But I don't really understand what condition its actually testing for.

But the 32-bit check does the exact same thing but via a single binary operation: it checks whether any bits outside of those bits are zero - just via a simpler test that compiles to more compact code.

So i'd go with the 32-bit version. (unless there are some sign-extension complications i'm missing - but i think we got rid of those already.)

Quoted 7 lines · Ingo Molnar

But the 32-bit check does the exact same thing but via a single binary
operation: it checks whether any bits outside of those bits are zero - just via a simpler test that compiles to more compact code.

So i'd go with the 32-bit version. (unless there are some sign-extension complications i'm missing - but i think we got rid of those already.)

OK, fair enough. I wouldn't be surprised if gcc does that transform anyway, but we may as well be consistent about it.

Quoted 24 lines · Jeremy Fitzhardinge

Quoted 23 lines · Ingo Molnar

Quoted 20 lines · Jeremy Fitzhardinge

I'm looking at unifying the 32 and 64-bit versions of pud_bad.

32-bits defines it as:

static inline int pud_bad(pud_t pud)

{
	return (pud_val(pud) & ~(PTE_PFN_MASK | _KERNPG_TABLE | _PAGE_USER)) != 0;
}

and 64 as:

static inline int pud_bad(pud_t pud)

{
	return (pud_val(pud) & ~(PTE_PFN_MASK | _PAGE_USER)) != _KERNPG_TABLE;
}

I'm inclined to go with the 64-bit version, but I'm wondering if there's something subtle I'm missing here.

Quoted 6 lines

Quoted 3 lines

Why go with the 64-bit version? The 32-bit check looks more compact and should result in smaller code.

Well, its stricter. But I don't really understand what condition its actually testing for.

Well it tests: "beyond the bits covered by PTE_PFN|_PAGE_USER, the rest must only be _KERNPG_TABLE".

The _KERNPG_TABLE bits are disjunct from PTE_PFN|_PAGE_USER bits, so this makes sense.

But the 32-bit check does the exact same thing but via a single binary
operation: it checks whether any bits outside of those bits are zero - just via a simpler test that compiles to more compact code.

Quoted 38 lines · Ingo Molnar

Quoted 24 lines · Jeremy Fitzhardinge

Quoted 23 lines · Ingo Molnar

Quoted 20 lines · Jeremy Fitzhardinge

I'm looking at unifying the 32 and 64-bit versions of pud_bad.

32-bits defines it as:

static inline int pud_bad(pud_t pud)

{
	return (pud_val(pud) & ~(PTE_PFN_MASK | _KERNPG_TABLE | _PAGE_USER)) != 0;
}

and 64 as:

static inline int pud_bad(pud_t pud)

{
	return (pud_val(pud) & ~(PTE_PFN_MASK | _PAGE_USER)) != _KERNPG_TABLE;
}

I'm inclined to go with the 64-bit version, but I'm wondering if there's something subtle I'm missing here.

Quoted 6 lines

Quoted 3 lines

Why go with the 64-bit version? The 32-bit check looks more compact and should result in smaller code.

Well, its stricter. But I don't really understand what condition its actually testing for.

Well it tests: "beyond the bits covered by PTE_PFN|_PAGE_USER, the rest must only be _KERNPG_TABLE".

The _KERNPG_TABLE bits are disjunct from PTE_PFN|_PAGE_USER bits, so this makes sense.

Quoted 3 lines

But the 32-bit check does the exact same thing but via a single binary
operation: it checks whether any bits outside of those bits are zero - just via a simpler test that compiles to more compact code.

Simpler and more compact, but not as strict: in particular, a value of 0 or 1 is identified as bad by that 64-bit test, but not by the 32-bit.

Quoted 2 lines

Simpler and more compact, but not as strict: in particular, a value of 0 or 1 is identified as bad by that 64-bit test, but not by the 32-bit.

yes, indeed you are right - the 64-bit test does not allow the KERNPG_TABLE bits to go zero.
Those are the present, rw, accessed and dirty bits. Do they really matter that much? If a toplevel entry goes !present or readonly, we notice that _fast_, without any checks. If it goes !access or !dirty - does that matter? These checks are done all the time, and even a single instruction can count. The bits that are checked are enough to notice random memory corruption. ( albeit these days with large RAM sizes pagetable corruption is quite rare
and only happens if it's specifically corrupting the pagetable - and then it's not just a single bit. Most of the memory corruption goes into the pagecache. )

The RW bit needs to be allowed to become read-only for hugetlb COW. Changing it over to the 32-bit method is a bugfix by that token.

Quoted 2 lines

The RW bit needs to be allowed to become read-only for hugetlb COW. Changing it over to the 32-bit method is a bugfix by that token.

If there's a bugfix to be made there, of course I'm in favour:
but how come we've never seen such a bug? hugetlb COW has been around for a year or two by now, hasn't it?

Quoted 2 lines · On Thu, 5 Feb 2009, [email protected] wrote:

Quoted 2 lines

The RW bit needs to be allowed to become read-only for hugetlb COW. Changing it over to the 32-bit method is a bugfix by that token.

Quoted 3 lines · Hugh Dickins

If there's a bugfix to be made there, of course I'm in favour:
but how come we've never seen such a bug? hugetlb COW has been around for a year or two by now, hasn't it?

We can tell from the code that a write-protected pte mapping of a 1GB hugetlb page would be flagged as bad. It must not be called on ptes mapping hugetlb pages if they're not getting flagged.

Quoted 3 lines · Hugh Dickins

Simpler and more compact, but not as strict: in particular, a value of 0 or 1 is identified as bad by that 64-bit test, but not by the 32-bit.

yes, indeed you are right - the 64-bit test does not allow the KERNPG_TABLE bits to go zero.

Those are the present, rw, accessed and dirty bits. Do they really matter that much? If a toplevel entry goes !present or readonly, we notice that _fast_, without any checks. If it goes !access or !dirty - does that matter?

These checks are done all the time, and even a single instruction can count. The bits that are checked are enough to notice random memory corruption.

However... I forget how the folding works out. The pgd in the 32-bit PAE case used to have just the pfn and the present bit set in that little array of four entries: if pud_bad() ends up getting applied to that, I guess it will blow up.

If so, my preferred answer would actually be to make those 4 entries look more like real ptes; but you may think I'm being a bit silly.

Quoted 4 lines · Hugh Dickins

However... I forget how the folding works out. The pgd in the 32-bit PAE case used to have just the pfn and the present bit set in that little array of four entries: if pud_bad() ends up getting applied to that, I guess it will blow up.

Ah, that's a good point.

Quoted 2 lines

If so, my preferred answer would actually be to make those 4 entries look more like real ptes; but you may think I'm being a bit silly.

Hardware doesn't allow it. It will explode (well, trap) if you set anything other than P in the top level.

By the by, what are the chances we'll be able to deprecate non-PAE 32-bit?

Quoted 3 lines

Hardware doesn't allow it. It will explode (well, trap) if you set anything other than P in the top level.

Oh, interesting, I'd never realized that.

Quoted 2 lines

By the by, what are the chances we'll be able to deprecate non-PAE 32-bit?

I sincerely hope 0! I shed no tears at losing support for NUMAQ, but why should we be forced to double all the 32-bit ptes? You want us all to be using NX? Or you just want to cut your test/edit matrix - that I can well understand!

Quoted 4 lines

I sincerely hope 0! I shed no tears at losing support for NUMAQ, but why should we be forced to double all the 32-bit ptes? You want us all to be using NX? Or you just want to cut your test/edit matrix - that I can well understand!

Yes, that's the gist of it. We could simplify things by having only one pte format and only have to parameterise with 3/4 level pagetables. We'd lose support for non-PAE cpus, including the first Pentium M (which is probably still in fairly wide use, unfortunately).

We'd also lose a fair bit of performance (not to mention the pagetable footprint doubling that Hugh already mentioned) on 32-bit PAE capable systems that dont actually have RAM above 4G physical.

Quoted 3 lines · Ingo Molnar

We'd also lose a fair bit of performance (not to mention the pagetable footprint doubling that Hugh already mentioned) on 32-bit PAE capable systems that dont actually have RAM above 4G physical.

Why's that? Do you mean directly from using PAE, or as a side-effect of highmem?

just the act of using PAE was measured to cause multi-percent slowdown in fork() and exec() latencies, etc. The pagetables are twice as large so is that really surprising?

Quoted 4 lines · Ingo Molnar

just the act of using PAE was measured to cause multi-percent slowdown in fork() and exec() latencies, etc. The pagetables are twice as large so is that really surprising?

Is there a similar slowdown running the CPU in 32 vs 64 bit mode? Or does having more/wider registers mitigate it?

Quoted 5 lines · Hugh Dickins

However... I forget how the folding works out. The pgd in the 32-bit PAE case used to have just the pfn and the present bit set in that little array of four entries: if pud_bad() ends up getting applied to that, I guess it will blow up.

Ah, that's a good point.

Quoted 2 lines

If so, my preferred answer would actually be to make those 4 entries look more like real ptes; but you may think I'm being a bit silly.

Hardware doesn't allow it. It will explode (well, trap) if you set anything other than P in the top level.

By the by, what are the chances we'll be able to deprecate non-PAE 32-bit?