[CRIU] [PATCH 1/6] Replace PAGE_SIZE in vaddr_to_pfn

[Christopher Covington]

On 10/08/2014 11:05 AM, Pavel Emelyanov wrote:
> On 10/08/2014 06:58 PM, Cyrill Gorcunov wrote:
>> On Wed, Oct 08, 2014 at 10:42:29AM -0400, Christopher Covington wrote:
>>>>
>>>> or something like that. SO we don't have to change PAGE_SIZE all over
>>>> the criu code at all, but in one place only. Hmm? What do you think?
>>>
>>> I did try that and I'd like to end up with something mostly equivalent.
>>> However, some existing code assumes PAGE_SIZE is constant, using it for
>>> compile-time buffer sizes. My instinct is to not turn all of these into
>>> run-time allocations.
>>>
>>> (As a brief aside, I it's a reasonable assumption to think PAGE_SIZE or
>>> ANY_MACRO_REALLY is a compile-time constant. In my opinion, using a function
>>> such as sysconf(_SC_PAGESIZE) or lowercase variable name such as page_size
>>> best communicates to those reading and writing the common code that the value
>>> cannot be relied upon to be a compile time constant across all systems.)
>>>
>>> Here are the different possible uses of PAGE_SIZE and corresponding
>>> replacements that I came up with:
>>>
>>> PAGE_OR_LESS -- (local) compile-time constant minimum page size, guaranteed to
>>> be less than or equal to one page but not necessarily page aligned
>>>
>>> EXEC_PAGESIZE -- (libc) compile-time constant maximum page size, guaranteed to
>>> be greater than or equal to one page and page aligned
>>>
>>> sysconf(_SC_PAGESIZE) or page_size -- (libc or local) run-time determined
>>> value of the current page size
>>>
>>> (And maybe also something like BUFFER_SIZE, if I run into uses that really
>>> have no functional or performance connection to the page size:
>>> http://lists.busybox.net/pipermail/busybox/2011-June/075864.html)
>>
>> I see. Indeed we've a number of places where PAGE_SIZE is assumed to be
>> a constant expanded at compile time. I like the idea of introducing
>> these variables. Pavel?
> 
> I've grep-ed around all the '\[PAGE_SIZE' lines. It looks like there's only
> one place that requires a page -- in memory restorer. All other places just
> declare a buffer big enough to fit what it needs (log message, proc file line,
> etc.).  That said, yes I agree that we might need another set of constants
> for such cases, but in all but one of them has nothing to do with a page :)

I wish it were that well contained. If we say that patches 5 and 6 (and the
error in pie/restorer.c that I have yet to fully diagnose) are memory restorer
related, that leaves the following cases where the page size does matter.

(Accidentally omitted in v1) Replace PAGE_SIZE in kerndat_get_shmemdev
In the sprintf, the second variable must be exactly a page larger than the
first or it'll get the filename in /proc/self/map_files wrong.

[PATCH 1/6] Replace PAGE_SIZE in vaddr_to_pfn
off must use the exact page size or read() could happen at a multiple of the
intended value, in some cases returning 0 (EOF).

[PATCH 3/6] Replace PAGE_SIZE in parasite_map_exchange
Same as the accidentally omitted patch, the exact page size is necessary to
get the filename in /proc/self/map_files right.

[PATCH 4/6] Replace PAGE_SIZE in prepare_restorer_blob
restorer_len must be page size aligned or mmap() will fail.

Christopher

P.S. Here's the commit message for the accidentally omitted patch, which I'll
be sure to include in v2.

Replace PAGE_SIZE in kerndat_get_shmemdev

In AArch64, pages may be 4K or 64K depending on kernel configuration.
Introduce a new macro, PAGE_OR_LESS, that is guaranteed to not be
bigger than a single page. Use PAGE_OR_LESS when it doesn't matter,
neither for correctness nor efficiency, what the size is as long as it
can be mapped with a single page. When the precise size of a page is
required, follow the recommendation of the GNU C Library
documentation, which states that, "the correct interface to query
about the page size is sysconf ... with the parameter _SC_PAGESIZE."

https://www.gnu.org/software/libc/manual/html_node/Query-Memory-Parameters.html

Comparing the strace output of a plain hello world versus a test
program that prints sysconf(_SC_PAGESIZE), it appears that calls to
sysconf(_SC_PAGESIZE) don't cause extra system calls. One presumes the
value is cached by the C library.

-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by the Linux Foundation.