[CRIU] [PATCH 2/2] include/common: Default to build host's page size

[Dmitry Safonov]

Hi Christopher,

2016-11-03 0:56 GMT+03:00  <cov at codeaurora.org>:
> Hi Dmitry,
>
> On 2016-11-02 14:29, Dmitry Safonov wrote:
>>
>> 2016-11-02 20:15 GMT+03:00 Christopher Covington <cov at codeaurora.org>:
>>>
>>> On PowerPC or arm64, different distributions may configure their
>>> kernel to provide different page sizes (4k versus 64k for example).
>>> CRIU must currently be built for a specific page size. Default to
>>> using the build system's page size, to make native building work by
>>> default. For cross-compiling or other situations, the default can
>>> be overriden like so:
>>>
>>>   rm criu/include/config.h
>>>   make PAGE_SIZE=65536
>>>
>>> Signed-off-by: Christopher Covington <cov at codeaurora.org>
>>
>>
>> Hi,
>>
>> I have a dumb question :)
>> Why do we have PAGE_SIZE constant *and* page_size() func?
>
>
> Statically allocated buffers/arrays need a size and the usual choice
> appears to be PAGE_SIZE. We could call it PAGE_SIZE_MIN or BUF_SIZE or
> whatever. Or we could dynamically allocate all the arrays.
>
> Buffers/arrays used in several system calls must have actual run time page
> size alignment. Or, assuming the page size options are multiples of each
> other, we could use a PAGE_SIZE_MAX #define.
>
> I think there other cases like values in /proc where the actual run time
> page size must be used with no hard-coded alternative available as far
> as I can tell.
>
> I've tried and failed in the past to categorize all the PAGE_SIZE usage
> into the above three PAGE_SIZE_MIN, PAGE_SIZE_MAX, and page_size() cases.
>
> Instead, for years I've just hobbled along building once for 4K and once
> for 64K (with a local change removing the BUILD_BUG_ON).
>
> This is an attempt to at least get upstream to the level of functionality
> I'm at locally--the ability to at least build a 64K CRIU for arm64. I
> unfortunately am not sure that I'll be able to accomplish the original
> proper categorization approach any time soon.

I must to say, I'm not objecting the patch -- just to clearify why we have
PAGE_SIZE and page_size() and when to use each.
Thanks for this work!

>> I mean, I don't completely get why in some places we use one over
>> the other alternative. I see two cons for using page_size() everywhere:
>> 1. perf cost for calling sysconf()
>> 2. using PAGE_SIZE in BUILD_BUG_ON macros.
>>
>> So, I guess, if we introduce some kdat variable, that will get initialized
>> with sysconf() and use it everywhere, we will solve (1) problem and
>> even gain some free perf in places where page_size() is used now.
>
>
> I've looked at how glibc caches the value, and it seems pretty optimal. I
> don't have profile information handy to calculate whether the library call
> overhead is significant or negligible.

Ok, so I was dazzeled by description of
commit cefe22bdaca2 ("Use run-time page size where it matters")
which introduces page_size() as sysconf() for arm64, but nor for other archs.
Why don't just use page_size() as sysconf then?

>> For (2), we can rename PAGE_SIZE to PAGE_SIZE_MIN or something,
>> so we keep all BUILD_BUG_ONs in places and review, where we
>> could use kdat var instead. Maybe even if there is a chance to rewrite
>> those build-checks without arch page size, we could remove this
>> macro completely.
>
>
> What BUILD_BUG_ON macro would you like to keep? The one I've removed
> prevents users from building CRIU in a manner that is compatible with
> several perfectly valid kernel configurations (4K, 16K, 256K pages on
> ppc64, 16K, 64K pages on arm64). And this isn't just theoretical, Fedora,
> Red Hat, and CentOS for arm64 use 64k pages. Debian and Ubuntu for arm64
> use 4k pages.

No, I'm not against this deletion - I meant that we could keep some more
descriptive macro like PAGE_SIZE_{MIN/MAX} as compile-time constant for
BUILD_BUG_ON checks or static buffer's sizes.

>> With kdat var we also will fix page_size() on platforms where is just
>> returns PAGE_SIZE -- AFAIK, x86 also has optional huge pages.
>
>
> As far as I know, in terms of basic functionality, userspace doesn't
> care whether huge pages are being used or not. I don't think you ever
> have to align system call buffers to huge page size, for example. Perhaps
> there are performance considerations, however.

Ok.

So, again, I'm not against this patch - just wanted to have it clear, why
we have PAGE_SIZE vs page_size() leapfrog.

Thanks,
             Dmitry