lkml.org 
[lkml]   [2010]   [May]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 1/7] hugetlb, rmap: add reverse mapping for hugepage
    Hi, Mel.

    Thank you for the review.

    On Tue, May 25, 2010 at 11:59:57AM +0100, Mel Gorman wrote:
    > ...
    > I'd have preferred to see the whole series but still...

    OK.

    > > diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
    > > index 78b4bc6..a574d09 100644
    > > --- a/include/linux/hugetlb.h
    > > +++ b/include/linux/hugetlb.h
    > > @@ -14,11 +14,6 @@ struct user_struct;
    > >
    > > int PageHuge(struct page *page);
    > >
    > > -static inline int is_vm_hugetlb_page(struct vm_area_struct *vma)
    > > -{
    > > - return vma->vm_flags & VM_HUGETLB;
    > > -}
    > > -
    > > void reset_vma_resv_huge_pages(struct vm_area_struct *vma);
    > > int hugetlb_sysctl_handler(struct ctl_table *, int, void __user *, size_t *, loff_t *);
    > > int hugetlb_overcommit_handler(struct ctl_table *, int, void __user *, size_t *, loff_t *);
    > > @@ -77,11 +72,6 @@ static inline int PageHuge(struct page *page)
    > > return 0;
    > > }
    > >
    > > -static inline int is_vm_hugetlb_page(struct vm_area_struct *vma)
    > > -{
    > > - return 0;
    > > -}
    > > -
    >
    > You collapse two functions into one here and move them to another
    > header. Is there a reason why pagemap.h could not include hugetlb.h?

    Yes, hugetlb.h includes pagemap.h through mempolicy.h.
    I didn't make pagemap.h depend on hugetlb.h because it makes cyclic dependency
    among pagemap.h, mempolicy.h and hugetlb.h.

    > It adds another header dependency which is bad but moving hugetlb stuff
    > into mm.h seems bad too.

    I have another choice to move the definition of is_vm_hugetlb_page() into
    mm/hugetlb.c and introduce declaration of this function to pagemap.h,
    but this needed a bit ugly #ifdefs and I didn't like it.
    If putting hugetlb code in mm.h is worse, I'll take the second choice
    in the next post.

    > > @@ -2268,6 +2277,50 @@ static int unmap_ref_private(struct mm_struct *mm, struct vm_area_struct *vma,
    > > return 1;
    > > }
    > >
    > > +/*
    > > + * The following three functions are counterparts of ones in mm/rmap.c.
    > > + * Unlike them, these functions don't have accounting code nor lru code,
    > > + * because we handle hugepages differently from common anonymous pages.
    > > + */
    > > +static void __hugepage_set_anon_rmap(struct page *page,
    > > + struct vm_area_struct *vma, unsigned long address, int exclusive)
    > > +{
    > > + struct anon_vma *anon_vma = vma->anon_vma;
    > > + BUG_ON(!anon_vma);
    > > + if (!exclusive) {
    > > + struct anon_vma_chain *avc;
    > > + avc = list_entry(vma->anon_vma_chain.prev,
    > > + struct anon_vma_chain, same_vma);
    > > + anon_vma = avc->anon_vma;
    > > + }
    > > + anon_vma = (void *) anon_vma + PAGE_MAPPING_ANON;
    > > + page->mapping = (struct address_space *) anon_vma;
    > > + page->index = linear_page_index(vma, address);
    > > +}
    > > +
    > > +static void hugepage_add_anon_rmap(struct page *page,
    > > + struct vm_area_struct *vma, unsigned long address)
    > > +{
    > > + struct anon_vma *anon_vma = vma->anon_vma;
    > > + int first;
    > > + BUG_ON(!anon_vma);
    > > + BUG_ON(address < vma->vm_start || address >= vma->vm_end);
    > > + first = atomic_inc_and_test(&page->_mapcount);
    > > + if (first)
    > > + __hugepage_set_anon_rmap(page, vma, address, 0);
    > > +}
    > > +
    > > +void hugepage_add_new_anon_rmap(struct page *page,
    > > + struct vm_area_struct *vma, unsigned long address)
    > > +{
    > > + BUG_ON(address < vma->vm_start || address >= vma->vm_end);
    > > + atomic_set(&page->_mapcount, 0);
    > > + __hugepage_set_anon_rmap(page, vma, address, 1);
    > > +}
    > > +
    >
    > Is it possible to move these to mm/rmap.c so all the anon rmap adding
    > code is in the same place? In the event that __page_set_anon_rmap() is
    > updated, there would be a greater chance the hugepage equivalent will be
    > noticed and updated.

    Sounds reasonable, I'll do it.

    > I didn't spot anything particularly bad after this. If these minor issues
    > could be addressed and the full series reposted, I'll test the hugetlb side
    > of things further just to be sure.

    OK, thanks you :)

    Thanks,
    Naoya Horiguchi


    \
     
     \ /
      Last update: 2010-05-26 08:57    [W:0.027 / U:2.012 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site