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

[Pavel Emelyanov]

On 10/08/2014 10:16 PM, Christopher Covington wrote:
> 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.

Of course, the PAGE_SIZE is used all over the code. When telling the PAGE_SIZE
matters in a few places I meant only those, where this value is used as the size
of a buffer. Sorry for not being accurate enough.

BTW, if grep for all the places, a HUGE amount of PAGE_SIZE-s is used in the
memory C/R code -- these are the blocks CRIU uses to fetch pages from tasks,
to track the memory changes, to deduplicate them, etc. Will 4k value work on
AArch64 in these places?

> 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.

OK, why not define the PAGE_SIZE as just the _sysconf(_SC_PAGESIZE) then?
In most of the places this would be a code that stopped using the pre-defined
constant and switched to the system requested one, in the other places these
are buffers for temporary data store, and, as I see, we can just use arbitrary 
large enough value for these cases.

Thanks,
Pavel