[lkml]   [2006]   [Jan]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
    SubjectRe: [PATCH/RFC] Shared page tables
    On Mon, 23 Jan 2006, Dave McCracken wrote:
    > --On Friday, January 20, 2006 21:24:08 +0000 Hugh Dickins
    > <> wrote:
    > I'll look into getting some profiles.

    Thanks, that will help everyone to judge the performance/complexity better.

    > The pmd level is important for ppc because it works in segments, which are
    > 256M in size and consist of a full pmd page. The current implementation of
    > the way ppc loads its tlb doesn't allow sharing at smaller than segment
    > size.

    Does that make pmd page sharing strictly necessary? The way you describe
    it, it sounds like it's merely that you "might as well" share pmd page,
    because otherwise would always just waste memory on PPC. But if it's
    significantly less complex not to go to that level, it may be worth that
    waste of memory (which would be a small fraction of what's now wasted at
    the pte level, wouldn't it?). Sorry for belabouring this point, which
    may just be a figment of my ignorance, but you've not convinced me yet.

    And maybe I'm exaggerating the additional complexity: you have, after
    all, been resolute in treating the levels in the same way. It's more
    a matter of offputting patch size than complexity: imagine splitting
    the patch into two, one to implement it at the pte level first, then
    a second to take it up to pmd level, that would be better.

    > I needed a function that returns a struct page for pgd and pud, defined in
    > each architecture. I decided the simplest way was to redefine pgd_page and
    > pud_page to match pmd_page and pte_page. Both functions are pretty much
    > used one place per architecture, so the change is trivial. I could come up
    > with new functions instead if you think it's an issue. I do have a bit of
    > a fetish about symmetry across levels :)

    Sounds to me like you made the right decision.

    > >> +#define pt_decrement_share(page)
    > >> +#define pt_shareable_vma(vma) (0)
    > >> +#define pt_unshare_range(vma, address, end)
    > >> +#endif /* CONFIG_PTSHARE */
    > >
    > > Please keep to "#define<space>MACRO<tab(s)definition" throughout:
    > > easiest thing would be to edit the patch itself.
    > Sorry. Done. I thought the standard was "#define<TAB>" like all the other
    > C code I've ever seen. I didn't realize Linux does it different.

    No, I can't claim "#define<space>" is a Linux standard: I happen to
    prefer it myself, and it seems to predominate in the header files I've
    most often added to; but I was only trying to remove the distracting
    inconsistency from your patch, whichever way round.

    > >> static inline int copy_pmd_range(struct mm_struct *dst_mm, struct
    > >> mm_struct *src_mm, pud_t *dst_pud, pud_t *src_pud, struct
    > >> vm_area_struct *vma,
    > >> - unsigned long addr, unsigned long end)
    > >> + unsigned long addr, unsigned long end, int shareable)
    > >> {
    > >
    > > I'd have preferred not to add the shareable argument at each level,
    > > and work it out here; or is that significantly less efficient?
    > My gut feeling is that the vma-level tests for shareability are significant
    > enough that we only want to do them once, then pass the result down the
    > call stack. I could change it if you disagree about the relative cost.

    I now think cut out completely your mods from copy_page_range and its
    subfunctions. Given Nick's "Don't copy ptes..." optimization there,
    what shareable areas will reach the lower levels? Only the VM_PFNMAP
    and VM_INSERTPAGE ones: which do exist, and may be large enough to
    qualify; but they're not the areas you're interested in targetting,
    so I'd say keep it simple and forget about shareability here. The
    simpler you keep your patch, the likelier it is to convince people.

    And that leads on to another issue which occurred to me today, in
    reading through your overnight replies. Notice the check on anon_vma
    in that optimization? None of your "shareable" tests check anon_vma:
    the vm_flags shared/write check may be enough in some contexts, but
    not in all. VM_WRITE can come and go, and even a VM_SHARED vma may
    have anon pages in it (through Linus' strange ptrace case): you must
    keep away from sharing pagetables once you've got anonymous pages in
    your vma (well, you could share those pagetables not yet anonymized,
    but that's just silly complexity). And in particular, the prio_tree
    loop next_shareable_vma needs to skip any vma with its anon_vma set:
    at present you're (unlikely but) liable to offer entirely unsuitable
    pagetables for sharing there.

    (In the course of writing this, I've discovered that 2.6.16-rc
    supports COWed hugepages: anonymous pages without any anon_vma.
    I'd better get up to speed on those: maybe we'll want to allocate
    an anon_vma just for the appropriate tests, maybe we'll want to
    set another VM_flag, don't know yet... for the moment you ignore
    them, and assume anon_vma is the thing to test for, as in 2.6.15.)

    One other thing I forgot to comment on your patch: too many largish
    inline functions - our latest fashion is only to say "inline" on the
    one-or-two liners.

    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to
    More majordomo info at
    Please read the FAQ at

     \ /
      Last update: 2006-01-24 18:52    [W:0.028 / U:3.140 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site