Closed Bug 982569 Opened 10 years ago Closed 10 years ago

Replace PAGE_SIZE by getpagesize()

Categories

(Core :: mozglue, defect)

All
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: tzimmermann, Assigned: gsvelto)

Details

Attachments

(1 file, 2 obsolete files)

PAGE_SIZE is a compile-time constant, but run-time page sizes might differ. We should replace the PAGE_SIZE constant by a run-time check, such as getpagesize
Let's get rid of the hard-coded page size altogether.
Assignee: nobody → gsvelto
Status: NEW → ASSIGNED
Attachment #8394050 - Flags: review?(khuey)
Comment on attachment 8394050 [details] [diff] [review]
[PATCH] Don't hard-code the page size

Review of attachment 8394050 [details] [diff] [review]:
-----------------------------------------------------------------

Good to see this getting fix. I couldn't resist on commenting on the patch though. ;)

::: mozglue/build/Nuwa.cpp
@@ +145,5 @@
> +#elif defined(_SC_PAGESIZE)
> +  return sysconf(_SC_PAGESIZE);
> +#else
> +  return 4096;
> +#endif

If I may suggest a change, it is to return PAGE_SIZE if possible and emit a warning for the hard-coded value. Should be more compatible and safer.

#if  /* GET_PAGESIZE */
...
#elif /* _SC_PAGESIZE */
...
#elif defined(PAGE_SIZE)
  return PAGE_SIZE;
#else
  #warning Hard-coding page size to 4096 byte
  return 4096
#endif

@@ +151,5 @@
> +
> +/**
> + * Align the pointer to the next page boundary unless it's already aligned
> + */
> +static uintptr_t roundToPage(uintptr_t aPtr) {

I'd call this function ceilToPage to make clear that it rounds towards to next higher page boundary.

@@ +159,5 @@
> +    return aPtr;
> +  }
> +
> +  return ((aPtr + pageSize - 1) / pageSize) * pageSize;
> +}

You can implement the ceil operation without branching, division and modulo like this:

uintptr_t pageAddr = aPtr & ~(pageSize-1); /* the RHS is the former PAGE_ALIGN_MASK */

return pageAddr + (pageAddr == aPtr) * pageSize;
> return pageAddr + (pageAddr == aPtr) * pageSize;

This is supposed to be (pageAddr != aPtr).
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #2)
> If I may suggest a change, it is to return PAGE_SIZE if possible and emit a
> warning for the hard-coded value. Should be more compatible and safer.

Good point, I'll do that.

> I'd call this function ceilToPage to make clear that it rounds towards to
> next higher page boundary.

Good point too.

> You can implement the ceil operation without branching, division and modulo
> like this:
> 
> uintptr_t pageAddr = aPtr & ~(pageSize-1); /* the RHS is the former
> PAGE_ALIGN_MASK */
> 
> return pageAddr + (pageAddr == aPtr) * pageSize;

Actually you made me realize that the if condition in my code was useless, the calculation would already output the right value. I'd stick with my method for the ceil calculation mostly because it would work even with non-power-of-2 page sizes (which I'm pretty sure won't ever happen but why write less robust code w/o a reason).
Attachment #8394050 - Attachment is obsolete: true
Attachment #8394050 - Flags: review?(khuey)
Attachment #8394211 - Flags: review?(khuey)
Hi

> Actually you made me realize that the if condition in my code was useless,
> the calculation would already output the right value. I'd stick with my
> method for the ceil calculation mostly because it would work even with
> non-power-of-2 page sizes (which I'm pretty sure won't ever happen but why
> write less robust code w/o a reason).

OK. The reason would be performance, but the code is almost never run, so who cares?

But this made me think whether a non-power-of-2 page table is even possible from a technical POV. I mean, how would a page table with 6-KiB-sized pages look like? And what would an MMU have to implement here? There seems to be no fast (i.e., bit-wise) operation to reduce such an address to its page directory. Even with a TLB in place, you'd probably need a full division on most memory operations.
Attachment 8394211 [details] [diff] doesn't apply to mozilla-central anymore, here's a refreshed patch that applies correctly. Carrying over the r+ flag, this is r=khuey.
Attachment #8394211 - Attachment is obsolete: true
Attachment #8409726 - Flags: review+
Ready to land, this is the try-run for the refreshed patch: https://tbpl.mozilla.org/?tree=Try&rev=a634cb03b22d
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/45ca9e143402
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.