From: Andi Kleen on

Mel, other than this nit are you happy with these changes now?

> > 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.

You could always create a new include file hugetlb-inlines.h

-Andi
--
ak(a)linux.intel.com -- Speaking for myself only.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Mel Gorman on
On Wed, May 26, 2010 at 11:19:58AM +0200, Andi Kleen wrote:
>
> Mel, other than this nit are you happy with these changes now?
>

Pretty much but I also want to test the series myself to be sure I haven't
missed something in review.

> > > 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.
>
> You could always create a new include file hugetlb-inlines.h
>

That would be another option. It'd need to be figured out what should
move from hugetlb.h to hugetlb-inlines.h in the future but ultimately it
would still be tidier than moving hugetlb stuff to mm.h (at least to
me).

--
Mel Gorman
Part-time Phd Student Linux Technology Center
University of Limerick IBM Dublin Software Lab
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Andi Kleen on
On Wed, May 26, 2010 at 10:44:43AM +0100, Mel Gorman wrote:
> On Wed, May 26, 2010 at 11:19:58AM +0200, Andi Kleen wrote:
> >
> > Mel, other than this nit are you happy with these changes now?
> >
>
> Pretty much but I also want to test the series myself to be sure I haven't
> missed something in review.

Thanks. I'll wait a bit more and if there is no negative reviews
I'll start queueing it in the hwpoison tree.

I should add this is probably only the first step, "early kill" support
and soft offline might need some more mm changes.

-Andi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Mel Gorman on
On Fri, May 28, 2010 at 09:29:16AM +0900, Naoya Horiguchi wrote:
> This patch adds reverse mapping feature for hugepage by introducing
> mapcount for shared/private-mapped hugepage and anon_vma for
> private-mapped hugepage.
>
> While hugepage is not currently swappable, reverse mapping can be useful
> for memory error handler.
>
> Without this patch, memory error handler cannot identify processes
> using the bad hugepage nor unmap it from them. That is:
> - for shared hugepage:
> we can collect processes using a hugepage through pagecache,
> but can not unmap the hugepage because of the lack of mapcount.
> - for privately mapped hugepage:
> we can neither collect processes nor unmap the hugepage.
> This patch solves these problems.
>
> This patch include the bug fix given by commit 23be7468e8, so reverts it.
>
> Dependency:
> "hugetlb: move definition of is_vm_hugetlb_page() to hugepage_inline.h"
>
> ChangeLog since May 24.
> - create hugetlb_inline.h and move is_vm_hugetlb_index() in it.
> - move functions setting up anon_vma for hugepage into mm/rmap.c.
>
> ChangeLog since May 13.
> - rebased to 2.6.34
> - fix logic error (in case that private mapping and shared mapping coexist)
> - move is_vm_hugetlb_page() into include/linux/mm.h to use this function
> from linear_page_index()
> - define and use linear_hugepage_index() instead of compound_order()
> - use page_move_anon_rmap() in hugetlb_cow()
> - copy exclusive switch of __set_page_anon_rmap() into hugepage counterpart.
> - revert commit 24be7468 completely
>
> Signed-off-by: Naoya Horiguchi <n-horiguchi(a)ah.jp.nec.com>
> Cc: Andi Kleen <andi(a)firstfloor.org>
> Cc: Andrew Morton <akpm(a)linux-foundation.org>
> Cc: Wu Fengguang <fengguang.wu(a)intel.com>
> Cc: Mel Gorman <mel(a)csn.ul.ie>
> Cc: Andrea Arcangeli <aarcange(a)redhat.com>
> Cc: Larry Woodman <lwoodman(a)redhat.com>
> Cc: Lee Schermerhorn <Lee.Schermerhorn(a)hp.com>

Ok, I could find no other problems with the hugetlb side of things in the
first two patches. I haven't looked at the hwpoison parts but I'm assuming
Andi has looked at those already. Thanks

Acked-by: Mel Gorman <mel(a)csn.ul.ie>

--
Mel Gorman
Part-time Phd Student Linux Technology Center
University of Limerick IBM Dublin Software Lab
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Andrew Morton on
On Fri, 28 May 2010 09:29:16 +0900
Naoya Horiguchi <n-horiguchi(a)ah.jp.nec.com> wrote:

> +#ifdef CONFIG_HUGETLBFS
> +/*
> + * The following three functions are for anonymous (private mapped) hugepages.
> + * Unlike common anonymous pages, anonymous hugepages have no accounting code
> + * and no lru code, because we handle hugepages differently from common 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);
> +}
> +
> +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);
> +}
> +#endif /* CONFIG_HUGETLBFS */

This code still make sense if CONFIG_HUGETLBFS=n, I think? Should it
instead depend on CONFIG_HUGETLB_PAGE?

I have a feeling that we make that confusion relatively often. Perhaps
CONFIG_HUGETLB_PAGE=y && CONFIG_HUGETLBFS=n makes no sense and we
should unify them...
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/