Messages in this thread | | | Date | Fri, 22 Mar 2013 07:51:31 -0700 | From | Dave Hansen <> | Subject | Re: [PATCHv2, RFC 08/30] thp, mm: rewrite add_to_page_cache_locked() to support huge pages |
| |
On 03/22/2013 03:34 AM, Kirill A. Shutemov wrote: > Dave Hansen wrote: >> On 03/14/2013 10:50 AM, Kirill A. Shutemov wrote: >>> + error = radix_tree_insert(&mapping->page_tree, >>> + offset + i, page + i); >>> + if (error) { >>> + page_cache_release(page + i); >>> + break; >>> + } >>> } >> >> Throughout all this new code, I'd really challenge you to try as much as >> possible to minimize the code stuck under "if (PageTransHuge(page))". > > I put thp-related code under the 'if' intentionally to be able to optimize > it out if !CONFIG_TRANSPARENT_HUGEPAGE. The config option is disabled by > default.
You've gotta give the compiler some credit! :) In this function's case, the compiler should be able to realize that nr=1 is constant if !TRANSPARENT_HUGEPAGE. It'll realize that all your for loops are:
for (i = 0; i < 1; i++) {
and will end up generating _very_ similar code to what you get with the explicit #ifdefs. You already _have_ #ifdefs, but they're just up in the headers around PageTransHuge()'s definition.
The advantages for you are *huge* since it means that folks will have a harder time inadvertently breaking your CONFIG_TRANSPARENT_HUGEPAGE code.
>> Does the cgroup code know how to handle these large pages internally >> somehow? It looks like the charge/uncharge is only being done for the >> head page. > > It can. We only need to remove PageCompound() check there. Patch is in > git.
OK, cool. This _might_ deserve a comment or something here. Again, it looks asymmetric and fishy to someone just reading the code.
| |