lkml.org 
[lkml]   [2009]   [Sep]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 5/8] mm: follow_hugetlb_page flags
    On Sun, Sep 13, 2009 at 04:35:44PM +0100, Hugh Dickins wrote:
    > On Wed, 9 Sep 2009, Mel Gorman wrote:
    > > On Mon, Sep 07, 2009 at 10:37:14PM +0100, Hugh Dickins wrote:
    > > >
    > > > (Alternatively, since hugetlb pages aren't swapped out under pressure,
    > > > you could save more dump space by arguing that a page not yet faulted
    > > > into this process cannot be relevant to the dump; but that would be
    > > > more surprising.)
    > >
    > > It would be more surprising. It's an implementation detail that hugetlb
    > > pages cannot be swapped out and someone reading the dump shouldn't have
    > > to be aware of it. It's better to treat non-faulted pages as if they
    > > were zero-filled.
    >
    > Oh sure, I did mean that the non-faulted pages should be zero-filled,
    > just stored (on those filesystems which support them) by holes in the
    > file instead of zero-filled blocks (just as the dump tries to do with
    > other zero pages). It would mess up the alignment with ELF headers
    > to leave them out completely.
    >

    Oh right, now I get you.

    > But it would still be a change in convention which might surprise
    > someone (pages of hugetlb file in the dump appearing as zeroed where
    > the underlying hugetlb file is known to contain non-zero data), and
    > there's already hugetlb dump filters for saving space on those areas.
    > So I'm not anxious to pursue that parenthetical alternative, just
    > admitting that we've got a choice of what to do here.
    >

    Grand.

    > > > @@ -2016,6 +2016,23 @@ static struct page *hugetlbfs_pagecache_
    > > > return find_lock_page(mapping, idx);
    > > > }
    > > >
    > > > +/* Return whether there is a pagecache page to back given address within VMA */
    > > > +static bool hugetlbfs_backed(struct hstate *h,
    > > > + struct vm_area_struct *vma, unsigned long address)
    > > > +{
    > > > + struct address_space *mapping;
    > > > + pgoff_t idx;
    > > > + struct page *page;
    > > > +
    > > > + mapping = vma->vm_file->f_mapping;
    > > > + idx = vma_hugecache_offset(h, vma, address);
    > > > +
    > > > + page = find_get_page(mapping, idx);
    > > > + if (page)
    > > > + put_page(page);
    > > > + return page != NULL;
    > > > +}
    > > > +
    > >
    > > It's a total nit-pick, but this is very similar to
    > > hugetlbfs_pagecache_page(). It would have been nice to have them nearby
    >
    > Indeed! That's why I placed it just after hugetlbfs_pagecache_page ;)
    >

    Oops, sorry.

    > > and called something like hugetlbfs_pagecache_present()
    >
    > Can call it that if you prefer, either name suits me.
    >

    I don't feel strongly enough to ask for a new version. If this is not
    the final version that is merged, then a name-change would be nice.
    Otherwise, it's not worth the hassle.

    > > or else reuse
    > > the function and have the caller unlock_page but it's probably not worth
    > > addressing.
    >
    > I did originally want to do it that way, but the caller is holding
    > page_table_lock, so cannot lock_page there.
    >

    Gack, fair point. If there is another version, a comment to that effect
    wouldn't hurt.

    > > > int follow_hugetlb_page(struct mm_struct *mm, struct vm_area_struct *vma,
    > > > struct page **pages, struct vm_area_struct **vmas,
    > > > unsigned long *position, int *length, int i,
    > > > - int write)
    > > > + unsigned int flags)
    > >
    > > Total aside, but in line with gfp_t flags, is there a case for having
    > > foll_t type for FOLL_* ?
    >
    > Perhaps some case, but it's the wrong side of my boredom threshold!
    > Even get_user_pages is much less widely used than the functions where
    > gfp flags and page order were getting muddled up. (foll_t itself
    > would not have helped, but maybe such a change would have saved me time
    > debugging the hang in an earlier version of this patch: eventually I saw
    > I was passing VM_FAULT_WRITE instead of FAULT_FLAG_WRITE to hugetlb_fault.)
    >

    I guess it's something to have on the back-boiler. If bugs of that
    nature happen a few times, then the effort would be justified. As you
    say, the gfp flags are much wider used.

    > > > + /*
    > > > + * When coredumping, it suits get_dump_page if we just return
    > > > + * an error if there's a hole and no huge pagecache to back it.
    > > > + */
    > > > + if (absent &&
    > > > + ((flags & FOLL_DUMP) && !hugetlbfs_backed(h, vma, vaddr))) {
    > > > + remainder = 0;
    > > > + break;
    > > > + }
    > >
    > > Does this break an assumption of get_user_pages() whereby when there are
    > > holes, the corresponding pages are NULL but the following pages are still
    > > checked? I guess the caller is informed ultimately that the read was only
    > > partial but offhand I don't know if that's generally expected or not.
    >
    > Sorry, I don't understand. get_user_pages() doesn't return any NULL
    > pages within the count it says was successful - Kamezawa-san had a patch
    > and flag which did so, and we might go that way, but it's not the case
    > at present is it?

    No, it's not but for some reason, I thought it was. On re-examination,
    what you are doing makes sense for the current implementation.

    > And follow_hugetlb_page() seems to be setting every
    > pages[i] within the count to something non-NULL.
    >
    > >
    > > Or is your comment saying that because the only caller using FOLL_DUMP is
    > > get_dump_page() using an array of one page, it doesn't care and the case is
    > > just not worth dealing with?
    >
    > Yes, that's more like it, but what case? Oh, the case where first pages
    > are okay, then we hit a hole. Right, that case doesn't actually arise
    > with FOLL_DUMP because of its sole user.
    >

    And nothing else other than core dumping will be using FOLL_DUMP so
    there should be no assumptions broken.

    > Perhaps my comment confuses because at first I had BUG_ON(remainder != 1)
    > in there, and wrote that comment, and returned -EFAULT; then later moved
    > the "i? i: -EFAULT" business down to the bottom and couldn't see any need
    > to assume remainder 1 any more. But the comment on "error" rather than
    > "error or short count" remains. But if I do change that to "error or
    > short count" it'll be a bit odd, because in practice it is always error.
    >
    > But it does seem that we've confused each other: what to say instead?
    >

    /*
    * When core-dumping, it's suits the get_dump_page() if an error is
    * returned if there is a hole and no huge pagecache to back it.
    * get_dump_page() is concerned with individual pages and by
    * returning the page as an error, the core dump file still gets
    * zeros but a hugepage allocation is avoided.
    */

    ?

    Sorry for the noise, my review wasn't as careful as it should have been.

    --
    Mel Gorman
    Part-time Phd Student Linux Technology Center
    University of Limerick IBM Dublin Software Lab


    \
     
     \ /
      Last update: 2009-09-14 15:29    [W:0.058 / U:30.256 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site