lkml.org 
[lkml]   [2008]   [Feb]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [RFC][PATCH] make /proc/pid/pagemap work with huge pages and return page size
    From
    Date
    (I've been mostly just reading along with this thread, as I haven't
    spent much time investigating huge page handling in general)

    On Thu, 2008-02-28 at 16:49 -0800, Dave Hansen wrote:
    > On Thu, 2008-02-28 at 13:00 +0100, Hans Rosenfeld wrote:
    > > On Wed, Feb 27, 2008 at 09:44:04AM -0800, Dave Hansen wrote:
    > > > On Tue, 2008-02-26 at 21:25 +0100, Hans Rosenfeld wrote:
    > > > I'm just worried that once we establish the format, we can't really
    > > > change it. We have enough room in the pseudo-pte now, but what happens
    > > > when the next group of people pop up that want something else from this
    > > > interface. Right now we have normal memory, swap, and hugetlb pages.
    > > >
    > > > What if people want migration ptes marked next? I'm not sure those fit
    > > > into what you have here.
    > > >
    > > > It all fits today, I'm just worried about tomorrow. :(
    > >
    > > We could change the interface to return just a pfn (which is aligned to
    > > the pshift returned), as it was before. That would free up some bits
    > > that we could reserve for future use.
    >
    > Yeah, I think we should do that. No reason to zero-pad it.

    Indeed.

    > > > > @@ -574,7 +581,7 @@ static int pagemap_pte_hole(unsigned long start, unsigned long end,
    > > > > u64 swap_pte_to_pagemap_entry(pte_t pte)
    > > > > {
    > > > > swp_entry_t e = pte_to_swp_entry(pte);
    > > > > - return PM_SWAP | swp_type(e) | (swp_offset(e) << MAX_SWAPFILES_SHIFT);
    > > > > + return swp_type(e) | (swp_offset(e) << MAX_SWAPFILES_SHIFT);
    > > > > }
    > > >
    > > > Is there any way to do unions of bitfields? It seems a bit silly that
    > > > we have this bitfield, and then subdivide the bitfield for the swap
    > > > entries.
    > >
    > > Having a union of bitfields is allowed, but having a union in a
    > > struct of bitfields or vice-versa will probably cause the compiler not
    > > to put all of this together in a single 64 bit entity.
    > >
    > > This whole swap thing still needs some thought. The swap file offset
    > > can take 59 bits, so there is a possibilty that this will break once
    > > someone uses a really huge swap file. I doubt that this will happen, but
    > > that doesn't mean it can't happen. Maybe there should be some completely
    > > different interface for the swap stuff, like /proc/pid/swapmap or
    > > something like that.
    >
    > I wouldn't worry about overflowing it. I think there are plenty of
    > block layer things that will break, first. :)

    I expect the trend for swap is that it'll be a rather small multiple of
    total memory size for the foreseeable future.

    > > > > static int pagemap_pte_range(pmd_t *pmd, unsigned long addr, unsigned long end,
    > > > > @@ -584,16 +591,23 @@ static int pagemap_pte_range(pmd_t *pmd, unsigned long addr, unsigned long end,
    > > > > pte_t *pte;
    > > > > int err = 0;
    > > > >
    > > > > - for (; addr != end; addr += PAGE_SIZE) {
    > > > > - u64 pfn = PM_NOT_PRESENT;
    > > > > + if (pmd_huge(*pmd))
    > > > > + add_huge_to_pagemap(addr, end, pmd_to_ppte(pmd), pm);
    > > >
    > > > Could you make this work with other architectures' large pages as well?
    > > > I'd hate to leave ia64, MIPS and powerpc out in the cold. powerpc at
    > > > least has large pmds, it just doesn't really expose them to generic
    > > > code.
    > >
    > > Well, if some powerpc guy would implement pmd_huge() and pmd_pfn() for
    > > powerpc, the x86 specific pmd_to_ppte() won't be that x86 specific no
    > > more. I didn't know there were huge pmds on powerpc, as pmd_huge() is
    > > defined as zero for everything but x86.
    >
    > OK, I'm now convinced that doing this with pmds is actually completely
    > wrong. :(
    >
    > Take a look at this code from mm/hugetlb.c:
    >
    > for (address = start; address < end; address += HPAGE_SIZE) {
    > ptep = huge_pte_offset(mm, address);
    > if (!ptep)
    > continue;
    >
    > if (huge_pmd_unshare(mm, &address, ptep))
    > continue;
    >
    > pte = huge_ptep_get_and_clear(mm, address, ptep);
    > if (pte_none(pte))
    > continue;
    >
    > page = pte_page(pte);
    > if (pte_dirty(pte))
    > set_page_dirty(page);
    > list_add(&page->lru, &page_list);
    > }
    >
    > The arch code is completely responsible for taking the mm and address
    > and giving you back a pte that you can do pte_page() on. This is a
    > nice, arch-abstracted interface that everybody can use regardless of how
    > their arch actually does it internally.
    >
    > The only issue is that this is *after* the code has decided that a
    > particular virtual area is for huge pages. The best arch-generic
    > interface I know for that is: is_vm_hugetlb_page(), but that is
    > VMA-based. Perhaps we should change the pagemap walk to pass the VMA
    > around.

    I'd rather avoid that. Requiring a VMA to poke at these things shouldn't
    -really- be necessary.

    > We should probably use a similar approach in the pagemap code. Or, if
    > we're really smart, we can actually share that code with the hugetlb
    > code.
    >
    > > Does it have huge puds as well? Once we support 1G pages for x86 a new
    > > function has to be added to this file to handle that special case, too.
    >
    > Yes, it does (or will soon have) huge puds. But, they're nicely wrapped
    > up in that pte_t interface I showed above like the rest of the large
    > pages.
    >
    > > > > pte = pte_offset_map(pmd, addr);
    > > > > - if (is_swap_pte(*pte))
    > > > > - pfn = swap_pte_to_pagemap_entry(*pte);
    > > > > - else if (pte_present(*pte))
    > > > > - pfn = pte_pfn(*pte);
    > > > > + if (is_swap_pte(*pte)) {
    > > > > + ppte.swap = 1;
    > > > > + ppte.paddr = swap_pte_to_pagemap_entry(*pte);
    > > > > + } else if (pte_present(*pte)) {
    > > > > + ppte.present = 1;
    > > > > + ppte.pshift = PAGE_SHIFT;
    > > > > + ppte.paddr = pte_pfn(*pte) << PAGE_SHIFT;
    > > > > + }
    > >
    > > This is the place where those architectures that define the page size in
    > > the pte should test for a huge page and put the correct page size in the
    > > pshift field. I looked at some of them and did not find a function or a
    > > macro to do this test, no generic one and no arch-dependent one.
    >
    > To test a pte for its huge page size? Well, for now, we only support
    > one huge page size at a time, and that's HPAGE_SIZE.
    >
    > > > The bitfields are nice, and I do see they've spread to generic code.
    > > > So, I won't object to them, but please do double-check that they don't
    > > > cause any problems, especially with compilers that you might not be
    > > > using.
    > >
    > > The standard says the ordering of bitfields is "implementation defined".
    > > I'm currently unsure whether this means the implementation of a machine
    > > or of the compiler. In the latter case, using a different compiler for
    > > a user space program than the one that was used to compile the kernel
    > > could create problems.
    >
    > I'd hate to be the first ones to depend on a bitfield for a
    > user<->kernel interface. Can you look around for precedent in this
    > area, or convert them back?
    >
    > -- Dave
    --
    Mathematics is the supreme nostalgia of our time.



    \
     
     \ /
      Last update: 2008-02-29 22:43    [W:0.051 / U:58.556 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site