* [PATCH 12/31] huge tmpfs: extend get_user_pages_fast to shmem pmd [not found] <alpine.LSU.2.11.1604051403210.5965@eggly.anvils> @ 2016-04-05 21:33 ` Hugh Dickins 2016-04-05 21:33 ` Hugh Dickins 2016-04-06 7:00 ` Ingo Molnar 0 siblings, 2 replies; 7+ messages in thread From: Hugh Dickins @ 2016-04-05 21:33 UTC (permalink / raw) To: Andrew Morton Cc: Kirill A. Shutemov, Andrea Arcangeli, Andres Lagar-Cavilla, Yang Shi, Ning Qu, Ralf Baechle, Martin Schwidefsky, David Miller, Ingo Molnar, linux-kernel, linux-arch, linux-mm The arch-specific get_user_pages_fast() has a gup_huge_pmd() designed to optimize the refcounting on anonymous THP and hugetlbfs pages, with one atomic addition to compound head's common refcount. That optimization must be avoided on huge tmpfs team pages, which use normal separate page refcounting. We could combine the PageTeam and PageCompound cases into a single simple loop, but would lose the compound optimization that way. One cannot go through these functions without wondering why some arches (x86, mips) like to SetPageReferenced, while the rest do not: an x86 optimization that missed being propagated to the other architectures? No, see commit 8ee53820edfd ("thp: mmu_notifier_test_young"): it's a KVM GRU EPT thing, maybe not useful beyond x86. I've just followed the established practice in each architecture. Signed-off-by: Hugh Dickins <hughd@google.com> --- Cc'ed to arch maintainers as an FYI: this patch is not expected to go into the tree in the next few weeks, and depends upon a PageTeam definition not yet available outside this huge tmpfs patchset. Please refer to linux-mm or linux-kernel for more context. arch/mips/mm/gup.c | 15 ++++++++++++++- arch/s390/mm/gup.c | 19 ++++++++++++++++++- arch/sparc/mm/gup.c | 19 ++++++++++++++++++- arch/x86/mm/gup.c | 15 ++++++++++++++- mm/gup.c | 19 ++++++++++++++++++- 5 files changed, 82 insertions(+), 5 deletions(-) --- a/arch/mips/mm/gup.c +++ b/arch/mips/mm/gup.c @@ -81,9 +81,22 @@ static int gup_huge_pmd(pmd_t pmd, unsig VM_BUG_ON(pte_special(pte)); VM_BUG_ON(!pfn_valid(pte_pfn(pte))); - refs = 0; head = pte_page(pte); page = head + ((addr & ~PMD_MASK) >> PAGE_SHIFT); + + if (PageTeam(head)) { + /* Handle a huge tmpfs team with normal refcounting. */ + do { + get_page(page); + SetPageReferenced(page); + pages[*nr] = page; + (*nr)++; + page++; + } while (addr += PAGE_SIZE, addr != end); + return 1; + } + + refs = 0; do { VM_BUG_ON(compound_head(page) != head); pages[*nr] = page; --- a/arch/s390/mm/gup.c +++ b/arch/s390/mm/gup.c @@ -66,9 +66,26 @@ static inline int gup_huge_pmd(pmd_t *pm return 0; VM_BUG_ON(!pfn_valid(pmd_val(pmd) >> PAGE_SHIFT)); - refs = 0; head = pmd_page(pmd); page = head + ((addr & ~PMD_MASK) >> PAGE_SHIFT); + + if (PageTeam(head)) { + /* Handle a huge tmpfs team with normal refcounting. */ + do { + if (!page_cache_get_speculative(page)) + return 0; + if (unlikely(pmd_val(pmd) != pmd_val(*pmdp))) { + put_page(page); + return 0; + } + pages[*nr] = page; + (*nr)++; + page++; + } while (addr += PAGE_SIZE, addr != end); + return 1; + } + + refs = 0; do { VM_BUG_ON(compound_head(page) != head); pages[*nr] = page; --- a/arch/sparc/mm/gup.c +++ b/arch/sparc/mm/gup.c @@ -77,9 +77,26 @@ static int gup_huge_pmd(pmd_t *pmdp, pmd if (write && !pmd_write(pmd)) return 0; - refs = 0; head = pmd_page(pmd); page = head + ((addr & ~PMD_MASK) >> PAGE_SHIFT); + + if (PageTeam(head)) { + /* Handle a huge tmpfs team with normal refcounting. */ + do { + if (!page_cache_get_speculative(page)) + return 0; + if (unlikely(pmd_val(pmd) != pmd_val(*pmdp))) { + put_page(page); + return 0; + } + pages[*nr] = page; + (*nr)++; + page++; + } while (addr += PAGE_SIZE, addr != end); + return 1; + } + + refs = 0; do { VM_BUG_ON(compound_head(page) != head); pages[*nr] = page; --- a/arch/x86/mm/gup.c +++ b/arch/x86/mm/gup.c @@ -196,9 +196,22 @@ static noinline int gup_huge_pmd(pmd_t p /* hugepages are never "special" */ VM_BUG_ON(pmd_flags(pmd) & _PAGE_SPECIAL); - refs = 0; head = pmd_page(pmd); page = head + ((addr & ~PMD_MASK) >> PAGE_SHIFT); + + if (PageTeam(head)) { + /* Handle a huge tmpfs team with normal refcounting. */ + do { + get_page(page); + SetPageReferenced(page); + pages[*nr] = page; + (*nr)++; + page++; + } while (addr += PAGE_SIZE, addr != end); + return 1; + } + + refs = 0; do { VM_BUG_ON_PAGE(compound_head(page) != head, page); pages[*nr] = page; --- a/mm/gup.c +++ b/mm/gup.c @@ -1247,9 +1247,26 @@ static int gup_huge_pmd(pmd_t orig, pmd_ if (write && !pmd_write(orig)) return 0; - refs = 0; head = pmd_page(orig); page = head + ((addr & ~PMD_MASK) >> PAGE_SHIFT); + + if (PageTeam(head)) { + /* Handle a huge tmpfs team with normal refcounting. */ + do { + if (!page_cache_get_speculative(page)) + return 0; + if (unlikely(pmd_val(orig) != pmd_val(*pmdp))) { + put_page(page); + return 0; + } + pages[*nr] = page; + (*nr)++; + page++; + } while (addr += PAGE_SIZE, addr != end); + return 1; + } + + refs = 0; do { VM_BUG_ON_PAGE(compound_head(page) != head, page); pages[*nr] = page; -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 12/31] huge tmpfs: extend get_user_pages_fast to shmem pmd 2016-04-05 21:33 ` [PATCH 12/31] huge tmpfs: extend get_user_pages_fast to shmem pmd Hugh Dickins @ 2016-04-05 21:33 ` Hugh Dickins 2016-04-06 7:00 ` Ingo Molnar 1 sibling, 0 replies; 7+ messages in thread From: Hugh Dickins @ 2016-04-05 21:33 UTC (permalink / raw) To: Andrew Morton Cc: Kirill A. Shutemov, Andrea Arcangeli, Andres Lagar-Cavilla, Yang Shi, Ning Qu, Ralf Baechle, Martin Schwidefsky, David Miller, Ingo Molnar, linux-kernel, linux-arch, linux-mm The arch-specific get_user_pages_fast() has a gup_huge_pmd() designed to optimize the refcounting on anonymous THP and hugetlbfs pages, with one atomic addition to compound head's common refcount. That optimization must be avoided on huge tmpfs team pages, which use normal separate page refcounting. We could combine the PageTeam and PageCompound cases into a single simple loop, but would lose the compound optimization that way. One cannot go through these functions without wondering why some arches (x86, mips) like to SetPageReferenced, while the rest do not: an x86 optimization that missed being propagated to the other architectures? No, see commit 8ee53820edfd ("thp: mmu_notifier_test_young"): it's a KVM GRU EPT thing, maybe not useful beyond x86. I've just followed the established practice in each architecture. Signed-off-by: Hugh Dickins <hughd@google.com> --- Cc'ed to arch maintainers as an FYI: this patch is not expected to go into the tree in the next few weeks, and depends upon a PageTeam definition not yet available outside this huge tmpfs patchset. Please refer to linux-mm or linux-kernel for more context. arch/mips/mm/gup.c | 15 ++++++++++++++- arch/s390/mm/gup.c | 19 ++++++++++++++++++- arch/sparc/mm/gup.c | 19 ++++++++++++++++++- arch/x86/mm/gup.c | 15 ++++++++++++++- mm/gup.c | 19 ++++++++++++++++++- 5 files changed, 82 insertions(+), 5 deletions(-) --- a/arch/mips/mm/gup.c +++ b/arch/mips/mm/gup.c @@ -81,9 +81,22 @@ static int gup_huge_pmd(pmd_t pmd, unsig VM_BUG_ON(pte_special(pte)); VM_BUG_ON(!pfn_valid(pte_pfn(pte))); - refs = 0; head = pte_page(pte); page = head + ((addr & ~PMD_MASK) >> PAGE_SHIFT); + + if (PageTeam(head)) { + /* Handle a huge tmpfs team with normal refcounting. */ + do { + get_page(page); + SetPageReferenced(page); + pages[*nr] = page; + (*nr)++; + page++; + } while (addr += PAGE_SIZE, addr != end); + return 1; + } + + refs = 0; do { VM_BUG_ON(compound_head(page) != head); pages[*nr] = page; --- a/arch/s390/mm/gup.c +++ b/arch/s390/mm/gup.c @@ -66,9 +66,26 @@ static inline int gup_huge_pmd(pmd_t *pm return 0; VM_BUG_ON(!pfn_valid(pmd_val(pmd) >> PAGE_SHIFT)); - refs = 0; head = pmd_page(pmd); page = head + ((addr & ~PMD_MASK) >> PAGE_SHIFT); + + if (PageTeam(head)) { + /* Handle a huge tmpfs team with normal refcounting. */ + do { + if (!page_cache_get_speculative(page)) + return 0; + if (unlikely(pmd_val(pmd) != pmd_val(*pmdp))) { + put_page(page); + return 0; + } + pages[*nr] = page; + (*nr)++; + page++; + } while (addr += PAGE_SIZE, addr != end); + return 1; + } + + refs = 0; do { VM_BUG_ON(compound_head(page) != head); pages[*nr] = page; --- a/arch/sparc/mm/gup.c +++ b/arch/sparc/mm/gup.c @@ -77,9 +77,26 @@ static int gup_huge_pmd(pmd_t *pmdp, pmd if (write && !pmd_write(pmd)) return 0; - refs = 0; head = pmd_page(pmd); page = head + ((addr & ~PMD_MASK) >> PAGE_SHIFT); + + if (PageTeam(head)) { + /* Handle a huge tmpfs team with normal refcounting. */ + do { + if (!page_cache_get_speculative(page)) + return 0; + if (unlikely(pmd_val(pmd) != pmd_val(*pmdp))) { + put_page(page); + return 0; + } + pages[*nr] = page; + (*nr)++; + page++; + } while (addr += PAGE_SIZE, addr != end); + return 1; + } + + refs = 0; do { VM_BUG_ON(compound_head(page) != head); pages[*nr] = page; --- a/arch/x86/mm/gup.c +++ b/arch/x86/mm/gup.c @@ -196,9 +196,22 @@ static noinline int gup_huge_pmd(pmd_t p /* hugepages are never "special" */ VM_BUG_ON(pmd_flags(pmd) & _PAGE_SPECIAL); - refs = 0; head = pmd_page(pmd); page = head + ((addr & ~PMD_MASK) >> PAGE_SHIFT); + + if (PageTeam(head)) { + /* Handle a huge tmpfs team with normal refcounting. */ + do { + get_page(page); + SetPageReferenced(page); + pages[*nr] = page; + (*nr)++; + page++; + } while (addr += PAGE_SIZE, addr != end); + return 1; + } + + refs = 0; do { VM_BUG_ON_PAGE(compound_head(page) != head, page); pages[*nr] = page; --- a/mm/gup.c +++ b/mm/gup.c @@ -1247,9 +1247,26 @@ static int gup_huge_pmd(pmd_t orig, pmd_ if (write && !pmd_write(orig)) return 0; - refs = 0; head = pmd_page(orig); page = head + ((addr & ~PMD_MASK) >> PAGE_SHIFT); + + if (PageTeam(head)) { + /* Handle a huge tmpfs team with normal refcounting. */ + do { + if (!page_cache_get_speculative(page)) + return 0; + if (unlikely(pmd_val(orig) != pmd_val(*pmdp))) { + put_page(page); + return 0; + } + pages[*nr] = page; + (*nr)++; + page++; + } while (addr += PAGE_SIZE, addr != end); + return 1; + } + + refs = 0; do { VM_BUG_ON_PAGE(compound_head(page) != head, page); pages[*nr] = page; ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 12/31] huge tmpfs: extend get_user_pages_fast to shmem pmd 2016-04-05 21:33 ` [PATCH 12/31] huge tmpfs: extend get_user_pages_fast to shmem pmd Hugh Dickins 2016-04-05 21:33 ` Hugh Dickins @ 2016-04-06 7:00 ` Ingo Molnar 2016-04-07 2:53 ` Hugh Dickins 1 sibling, 1 reply; 7+ messages in thread From: Ingo Molnar @ 2016-04-06 7:00 UTC (permalink / raw) To: Hugh Dickins Cc: Andrew Morton, Kirill A. Shutemov, Andrea Arcangeli, Andres Lagar-Cavilla, Yang Shi, Ning Qu, Ralf Baechle, Martin Schwidefsky, David Miller, linux-kernel, linux-arch, linux-mm * Hugh Dickins <hughd@google.com> wrote: > The arch-specific get_user_pages_fast() has a gup_huge_pmd() designed to > optimize the refcounting on anonymous THP and hugetlbfs pages, with one > atomic addition to compound head's common refcount. That optimization > must be avoided on huge tmpfs team pages, which use normal separate page > refcounting. We could combine the PageTeam and PageCompound cases into > a single simple loop, but would lose the compound optimization that way. > > One cannot go through these functions without wondering why some arches > (x86, mips) like to SetPageReferenced, while the rest do not: an x86 > optimization that missed being propagated to the other architectures? > No, see commit 8ee53820edfd ("thp: mmu_notifier_test_young"): it's a > KVM GRU EPT thing, maybe not useful beyond x86. I've just followed > the established practice in each architecture. > > Signed-off-by: Hugh Dickins <hughd@google.com> > --- > Cc'ed to arch maintainers as an FYI: this patch is not expected to > go into the tree in the next few weeks, and depends upon a PageTeam > definition not yet available outside this huge tmpfs patchset. > Please refer to linux-mm or linux-kernel for more context. > > arch/mips/mm/gup.c | 15 ++++++++++++++- > arch/s390/mm/gup.c | 19 ++++++++++++++++++- > arch/sparc/mm/gup.c | 19 ++++++++++++++++++- > arch/x86/mm/gup.c | 15 ++++++++++++++- > mm/gup.c | 19 ++++++++++++++++++- > 5 files changed, 82 insertions(+), 5 deletions(-) > > --- a/arch/mips/mm/gup.c > +++ b/arch/mips/mm/gup.c > @@ -81,9 +81,22 @@ static int gup_huge_pmd(pmd_t pmd, unsig > VM_BUG_ON(pte_special(pte)); > VM_BUG_ON(!pfn_valid(pte_pfn(pte))); > > - refs = 0; > head = pte_page(pte); > page = head + ((addr & ~PMD_MASK) >> PAGE_SHIFT); > + > + if (PageTeam(head)) { > + /* Handle a huge tmpfs team with normal refcounting. */ > + do { > + get_page(page); > + SetPageReferenced(page); > + pages[*nr] = page; > + (*nr)++; > + page++; > + } while (addr += PAGE_SIZE, addr != end); > + return 1; > + } > + > + refs = 0; > do { > VM_BUG_ON(compound_head(page) != head); > pages[*nr] = page; > --- a/arch/s390/mm/gup.c > +++ b/arch/s390/mm/gup.c > @@ -66,9 +66,26 @@ static inline int gup_huge_pmd(pmd_t *pm > return 0; > VM_BUG_ON(!pfn_valid(pmd_val(pmd) >> PAGE_SHIFT)); > > - refs = 0; > head = pmd_page(pmd); > page = head + ((addr & ~PMD_MASK) >> PAGE_SHIFT); > + > + if (PageTeam(head)) { > + /* Handle a huge tmpfs team with normal refcounting. */ > + do { > + if (!page_cache_get_speculative(page)) > + return 0; > + if (unlikely(pmd_val(pmd) != pmd_val(*pmdp))) { > + put_page(page); > + return 0; > + } > + pages[*nr] = page; > + (*nr)++; > + page++; > + } while (addr += PAGE_SIZE, addr != end); > + return 1; > + } > + > + refs = 0; > do { > VM_BUG_ON(compound_head(page) != head); > pages[*nr] = page; > --- a/arch/sparc/mm/gup.c > +++ b/arch/sparc/mm/gup.c > @@ -77,9 +77,26 @@ static int gup_huge_pmd(pmd_t *pmdp, pmd > if (write && !pmd_write(pmd)) > return 0; > > - refs = 0; > head = pmd_page(pmd); > page = head + ((addr & ~PMD_MASK) >> PAGE_SHIFT); > + > + if (PageTeam(head)) { > + /* Handle a huge tmpfs team with normal refcounting. */ > + do { > + if (!page_cache_get_speculative(page)) > + return 0; > + if (unlikely(pmd_val(pmd) != pmd_val(*pmdp))) { > + put_page(page); > + return 0; > + } > + pages[*nr] = page; > + (*nr)++; > + page++; > + } while (addr += PAGE_SIZE, addr != end); > + return 1; > + } > + > + refs = 0; > do { > VM_BUG_ON(compound_head(page) != head); > pages[*nr] = page; > --- a/arch/x86/mm/gup.c > +++ b/arch/x86/mm/gup.c > @@ -196,9 +196,22 @@ static noinline int gup_huge_pmd(pmd_t p > /* hugepages are never "special" */ > VM_BUG_ON(pmd_flags(pmd) & _PAGE_SPECIAL); > > - refs = 0; > head = pmd_page(pmd); > page = head + ((addr & ~PMD_MASK) >> PAGE_SHIFT); > + > + if (PageTeam(head)) { > + /* Handle a huge tmpfs team with normal refcounting. */ > + do { > + get_page(page); > + SetPageReferenced(page); > + pages[*nr] = page; > + (*nr)++; > + page++; > + } while (addr += PAGE_SIZE, addr != end); > + return 1; > + } > + > + refs = 0; > do { > VM_BUG_ON_PAGE(compound_head(page) != head, page); > pages[*nr] = page; > --- a/mm/gup.c > +++ b/mm/gup.c > @@ -1247,9 +1247,26 @@ static int gup_huge_pmd(pmd_t orig, pmd_ > if (write && !pmd_write(orig)) > return 0; > > - refs = 0; > head = pmd_page(orig); > page = head + ((addr & ~PMD_MASK) >> PAGE_SHIFT); > + > + if (PageTeam(head)) { > + /* Handle a huge tmpfs team with normal refcounting. */ > + do { > + if (!page_cache_get_speculative(page)) > + return 0; > + if (unlikely(pmd_val(orig) != pmd_val(*pmdp))) { > + put_page(page); > + return 0; > + } > + pages[*nr] = page; > + (*nr)++; > + page++; > + } while (addr += PAGE_SIZE, addr != end); > + return 1; > + } > + > + refs = 0; > do { > VM_BUG_ON_PAGE(compound_head(page) != head, page); > pages[*nr] = page; Ouch! Looks like there are two main variants - so these kinds of repetitive patterns very much call for some sort of factoring out of common code, right? Then the fix could be applied to the common portion(s) only, which will cut down this gigantic diffstat: > 5 files changed, 82 insertions(+), 5 deletions(-) Thanks, Ingo ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 12/31] huge tmpfs: extend get_user_pages_fast to shmem pmd 2016-04-06 7:00 ` Ingo Molnar @ 2016-04-07 2:53 ` Hugh Dickins 2016-04-07 2:53 ` Hugh Dickins 2016-04-13 8:58 ` Ingo Molnar 0 siblings, 2 replies; 7+ messages in thread From: Hugh Dickins @ 2016-04-07 2:53 UTC (permalink / raw) To: Ingo Molnar Cc: Hugh Dickins, Andrew Morton, Kirill A. Shutemov, Andrea Arcangeli, Andres Lagar-Cavilla, Yang Shi, Ning Qu, Ralf Baechle, Martin Schwidefsky, David Miller, linux-kernel, linux-arch, linux-mm On Wed, 6 Apr 2016, Ingo Molnar wrote: > * Hugh Dickins <hughd@google.com> wrote: > > > --- > > Cc'ed to arch maintainers as an FYI: this patch is not expected to > > go into the tree in the next few weeks, and depends upon a PageTeam > > definition not yet available outside this huge tmpfs patchset. > > Please refer to linux-mm or linux-kernel for more context. Actually, Andrew took it and the rest into mmotm yesterday, to give them better exposure through linux-next, so they should appear there soon. > > > > arch/mips/mm/gup.c | 15 ++++++++++++++- > > arch/s390/mm/gup.c | 19 ++++++++++++++++++- > > arch/sparc/mm/gup.c | 19 ++++++++++++++++++- > > arch/x86/mm/gup.c | 15 ++++++++++++++- > > mm/gup.c | 19 ++++++++++++++++++- > > 5 files changed, 82 insertions(+), 5 deletions(-) ... > > Ouch! Oh sorry, I didn't mean to hurt you ;) > > Looks like there are two main variants - so these kinds of repetitive patterns > very much call for some sort of factoring out of common code, right? Hmm. I'm still struggling between the two extremes, of (a) agreeing completely with you, and saying, yeah, I'll take on the job of refactoring every architecture's get_user_pages_as_fast_as_you_can(), without much likelihood of testing more than one, and (b) running a mile, and pointing out that we have a tradition of using arch/x86/mm/gup.c as a template for the others, and here I've just added a few more lines to that template (which never gets built more than once into any kernel). Both are appealing in their different ways, but I think you can tell which I'm leaning towards... Honestly, I am still struggling between those two; but I think the patch as it stands is one thing, and cleanup for commonality should be another however weaselly that sounds ("I'll come back to it" - yeah, right). Hugh > > Then the fix could be applied to the common portion(s) only, which will cut down > this gigantic diffstat: > > > 5 files changed, 82 insertions(+), 5 deletions(-) > > Thanks, > > Ingo -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 12/31] huge tmpfs: extend get_user_pages_fast to shmem pmd 2016-04-07 2:53 ` Hugh Dickins @ 2016-04-07 2:53 ` Hugh Dickins 2016-04-13 8:58 ` Ingo Molnar 1 sibling, 0 replies; 7+ messages in thread From: Hugh Dickins @ 2016-04-07 2:53 UTC (permalink / raw) To: Ingo Molnar Cc: Hugh Dickins, Andrew Morton, Kirill A. Shutemov, Andrea Arcangeli, Andres Lagar-Cavilla, Yang Shi, Ning Qu, Ralf Baechle, Martin Schwidefsky, David Miller, linux-kernel, linux-arch, linux-mm On Wed, 6 Apr 2016, Ingo Molnar wrote: > * Hugh Dickins <hughd@google.com> wrote: > > > --- > > Cc'ed to arch maintainers as an FYI: this patch is not expected to > > go into the tree in the next few weeks, and depends upon a PageTeam > > definition not yet available outside this huge tmpfs patchset. > > Please refer to linux-mm or linux-kernel for more context. Actually, Andrew took it and the rest into mmotm yesterday, to give them better exposure through linux-next, so they should appear there soon. > > > > arch/mips/mm/gup.c | 15 ++++++++++++++- > > arch/s390/mm/gup.c | 19 ++++++++++++++++++- > > arch/sparc/mm/gup.c | 19 ++++++++++++++++++- > > arch/x86/mm/gup.c | 15 ++++++++++++++- > > mm/gup.c | 19 ++++++++++++++++++- > > 5 files changed, 82 insertions(+), 5 deletions(-) ... > > Ouch! Oh sorry, I didn't mean to hurt you ;) > > Looks like there are two main variants - so these kinds of repetitive patterns > very much call for some sort of factoring out of common code, right? Hmm. I'm still struggling between the two extremes, of (a) agreeing completely with you, and saying, yeah, I'll take on the job of refactoring every architecture's get_user_pages_as_fast_as_you_can(), without much likelihood of testing more than one, and (b) running a mile, and pointing out that we have a tradition of using arch/x86/mm/gup.c as a template for the others, and here I've just added a few more lines to that template (which never gets built more than once into any kernel). Both are appealing in their different ways, but I think you can tell which I'm leaning towards... Honestly, I am still struggling between those two; but I think the patch as it stands is one thing, and cleanup for commonality should be another however weaselly that sounds ("I'll come back to it" - yeah, right). Hugh > > Then the fix could be applied to the common portion(s) only, which will cut down > this gigantic diffstat: > > > 5 files changed, 82 insertions(+), 5 deletions(-) > > Thanks, > > Ingo ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 12/31] huge tmpfs: extend get_user_pages_fast to shmem pmd 2016-04-07 2:53 ` Hugh Dickins 2016-04-07 2:53 ` Hugh Dickins @ 2016-04-13 8:58 ` Ingo Molnar 2016-04-13 8:58 ` Ingo Molnar 1 sibling, 1 reply; 7+ messages in thread From: Ingo Molnar @ 2016-04-13 8:58 UTC (permalink / raw) To: Hugh Dickins Cc: Andrew Morton, Kirill A. Shutemov, Andrea Arcangeli, Andres Lagar-Cavilla, Yang Shi, Ning Qu, Ralf Baechle, Martin Schwidefsky, David Miller, linux-kernel, linux-arch, linux-mm * Hugh Dickins <hughd@google.com> wrote: > > > arch/mips/mm/gup.c | 15 ++++++++++++++- > > > arch/s390/mm/gup.c | 19 ++++++++++++++++++- > > > arch/sparc/mm/gup.c | 19 ++++++++++++++++++- > > > arch/x86/mm/gup.c | 15 ++++++++++++++- > > > mm/gup.c | 19 ++++++++++++++++++- > > > 5 files changed, 82 insertions(+), 5 deletions(-) > ... > > Looks like there are two main variants - so these kinds of repetitive patterns > > very much call for some sort of factoring out of common code, right? > > Hmm. I'm still struggling between the two extremes, of > > (a) agreeing completely with you, and saying, yeah, I'll take on the job > of refactoring every architecture's get_user_pages_as_fast_as_you_can(), > without much likelihood of testing more than one, > > and > > (b) running a mile, and pointing out that we have a tradition of using > arch/x86/mm/gup.c as a template for the others, and here I've just > added a few more lines to that template (which never gets built more > than once into any kernel). > > Both are appealing in their different ways, but I think you can tell > which I'm leaning towards... > > Honestly, I am still struggling between those two; but I think the patch > as it stands is one thing, and cleanup for commonality should be another > however weaselly that sounds ("I'll come back to it" - yeah, right). Yeah, so my worry is this: your patch for example roughly doubles the algorithmic complexity of mm/gup.c and arch/*/mm/gup.c's ::gup_huge_pmd(). And you want this to add a new feature! So it really looks like to me this is the last sane chance to unify cheaply, then add the feature you want. Everyone else in the future will be able to refer to your example to chicken out! ;-) Thanks, Ingo -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 12/31] huge tmpfs: extend get_user_pages_fast to shmem pmd 2016-04-13 8:58 ` Ingo Molnar @ 2016-04-13 8:58 ` Ingo Molnar 0 siblings, 0 replies; 7+ messages in thread From: Ingo Molnar @ 2016-04-13 8:58 UTC (permalink / raw) To: Hugh Dickins Cc: Andrew Morton, Kirill A. Shutemov, Andrea Arcangeli, Andres Lagar-Cavilla, Yang Shi, Ning Qu, Ralf Baechle, Martin Schwidefsky, David Miller, linux-kernel, linux-arch, linux-mm * Hugh Dickins <hughd@google.com> wrote: > > > arch/mips/mm/gup.c | 15 ++++++++++++++- > > > arch/s390/mm/gup.c | 19 ++++++++++++++++++- > > > arch/sparc/mm/gup.c | 19 ++++++++++++++++++- > > > arch/x86/mm/gup.c | 15 ++++++++++++++- > > > mm/gup.c | 19 ++++++++++++++++++- > > > 5 files changed, 82 insertions(+), 5 deletions(-) > ... > > Looks like there are two main variants - so these kinds of repetitive patterns > > very much call for some sort of factoring out of common code, right? > > Hmm. I'm still struggling between the two extremes, of > > (a) agreeing completely with you, and saying, yeah, I'll take on the job > of refactoring every architecture's get_user_pages_as_fast_as_you_can(), > without much likelihood of testing more than one, > > and > > (b) running a mile, and pointing out that we have a tradition of using > arch/x86/mm/gup.c as a template for the others, and here I've just > added a few more lines to that template (which never gets built more > than once into any kernel). > > Both are appealing in their different ways, but I think you can tell > which I'm leaning towards... > > Honestly, I am still struggling between those two; but I think the patch > as it stands is one thing, and cleanup for commonality should be another > however weaselly that sounds ("I'll come back to it" - yeah, right). Yeah, so my worry is this: your patch for example roughly doubles the algorithmic complexity of mm/gup.c and arch/*/mm/gup.c's ::gup_huge_pmd(). And you want this to add a new feature! So it really looks like to me this is the last sane chance to unify cheaply, then add the feature you want. Everyone else in the future will be able to refer to your example to chicken out! ;-) Thanks, Ingo ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2016-04-13 8:58 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <alpine.LSU.2.11.1604051403210.5965@eggly.anvils>
2016-04-05 21:33 ` [PATCH 12/31] huge tmpfs: extend get_user_pages_fast to shmem pmd Hugh Dickins
2016-04-05 21:33 ` Hugh Dickins
2016-04-06 7:00 ` Ingo Molnar
2016-04-07 2:53 ` Hugh Dickins
2016-04-07 2:53 ` Hugh Dickins
2016-04-13 8:58 ` Ingo Molnar
2016-04-13 8:58 ` Ingo Molnar
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).