| Message ID | 11B98DF8-2B98-414B-88E5-14C97ADAA60E@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, Dec 20, 2014 at 12:49:40AM +0000, Jungseok Lee wrote: > This patch adds pgd_page definition in order to keep supporting > HAVE_GENERIC_RCU_GUP configuration. In addition, it changes pud_page > expression to align with pmd_page for readability. > > An introduction of pgd_page resolves the following build breakage > under 4KB + 4Level memory management combo. > > mm/gup.c: In function 'gup_huge_pgd': > mm/gup.c:889:2: error: implicit declaration of function 'pgd_page' [-Werror=implicit-function-declaration] > head = pgd_page(orig); > ^ > mm/gup.c:889:7: warning: assignment makes pointer from integer without a cast > head = pgd_page(orig); > > Cc: Catalin Marinas <catalin.marinas@arm.com> > Cc: Will Deacon <will.deacon@arm.com> > Cc: Steve Capper <steve.capper@linaro.org> > Signed-off-by: Jungseok Lee <jungseoklee85@gmail.com> > --- > arch/arm64/include/asm/pgtable.h | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h > index df22314..a1fe927 100644 > --- a/arch/arm64/include/asm/pgtable.h > +++ b/arch/arm64/include/asm/pgtable.h > @@ -401,7 +401,7 @@ static inline pmd_t *pmd_offset(pud_t *pud, unsigned long addr) > return (pmd_t *)pud_page_vaddr(*pud) + pmd_index(addr); > } > > -#define pud_page(pud) pmd_page(pud_pmd(pud)) > +#define pud_page(pud) pfn_to_page(__phys_to_pfn(pud_val(pud) & PHYS_MASK)) It would be cleaner to use pud_pfn here, otherwise we end up passing a physical address with the lower attributes present to __phys_to_pfn, which "knows" to shift them away. > #endif /* CONFIG_ARM64_PGTABLE_LEVELS > 2 */ > > @@ -437,6 +437,8 @@ static inline pud_t *pud_offset(pgd_t *pgd, unsigned long addr) > return (pud_t *)pgd_page_vaddr(*pgd) + pud_index(addr); > } > > +#define pgd_page(pgd) pfn_to_page(__phys_to_pfn(pgd_val(pgd) & PHYS_MASK)) Then you'd need to define pgd_pfn for this guy. With that change: Acked-by: Will Deacon <will.deacon@arm.com> Will
On Sun, Dec 21, 2014 at 10:56:33AM +0000, Will Deacon wrote: > On Sat, Dec 20, 2014 at 12:49:40AM +0000, Jungseok Lee wrote: > > This patch adds pgd_page definition in order to keep supporting > > HAVE_GENERIC_RCU_GUP configuration. In addition, it changes pud_page > > expression to align with pmd_page for readability. > > > > An introduction of pgd_page resolves the following build breakage > > under 4KB + 4Level memory management combo. > > > > mm/gup.c: In function 'gup_huge_pgd': > > mm/gup.c:889:2: error: implicit declaration of function 'pgd_page' [-Werror=implicit-function-declaration] > > head = pgd_page(orig); > > ^ > > mm/gup.c:889:7: warning: assignment makes pointer from integer without a cast > > head = pgd_page(orig); > > > > Cc: Catalin Marinas <catalin.marinas@arm.com> > > Cc: Will Deacon <will.deacon@arm.com> > > Cc: Steve Capper <steve.capper@linaro.org> > > Signed-off-by: Jungseok Lee <jungseoklee85@gmail.com> > > --- > > arch/arm64/include/asm/pgtable.h | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h > > index df22314..a1fe927 100644 > > --- a/arch/arm64/include/asm/pgtable.h > > +++ b/arch/arm64/include/asm/pgtable.h > > @@ -401,7 +401,7 @@ static inline pmd_t *pmd_offset(pud_t *pud, unsigned long addr) > > return (pmd_t *)pud_page_vaddr(*pud) + pmd_index(addr); > > } > > > > -#define pud_page(pud) pmd_page(pud_pmd(pud)) > > +#define pud_page(pud) pfn_to_page(__phys_to_pfn(pud_val(pud) & PHYS_MASK)) > > It would be cleaner to use pud_pfn here, otherwise we end up passing a > physical address with the lower attributes present to __phys_to_pfn, which > "knows" to shift them away. It looks like we did the same with pmd_page() but not with pte_page(), the latter using pte_pfn(). It's not a problem with lower attributes because they are within the first 12 bits, so the shifting gets rid of them. For pmd/pud/pgd, bits above 12 and up to the actual address must be 0. I agree with changing all of them to use pte/pmd/pud/pgd_pfn but I'm inclined to merge this patch now to fix the kernel build and we'll look at the clean-up after Christmas.
On Dec 22, 2014, at 9:30 PM, Catalin Marinas wrote: > On Sun, Dec 21, 2014 at 10:56:33AM +0000, Will Deacon wrote: >> On Sat, Dec 20, 2014 at 12:49:40AM +0000, Jungseok Lee wrote: >>> This patch adds pgd_page definition in order to keep supporting >>> HAVE_GENERIC_RCU_GUP configuration. In addition, it changes pud_page >>> expression to align with pmd_page for readability. >>> >>> An introduction of pgd_page resolves the following build breakage >>> under 4KB + 4Level memory management combo. >>> >>> mm/gup.c: In function 'gup_huge_pgd': >>> mm/gup.c:889:2: error: implicit declaration of function 'pgd_page' [-Werror=implicit-function-declaration] >>> head = pgd_page(orig); >>> ^ >>> mm/gup.c:889:7: warning: assignment makes pointer from integer without a cast >>> head = pgd_page(orig); >>> >>> Cc: Catalin Marinas <catalin.marinas@arm.com> >>> Cc: Will Deacon <will.deacon@arm.com> >>> Cc: Steve Capper <steve.capper@linaro.org> >>> Signed-off-by: Jungseok Lee <jungseoklee85@gmail.com> >>> --- >>> arch/arm64/include/asm/pgtable.h | 4 +++- >>> 1 file changed, 3 insertions(+), 1 deletion(-) >>> >>> diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h >>> index df22314..a1fe927 100644 >>> --- a/arch/arm64/include/asm/pgtable.h >>> +++ b/arch/arm64/include/asm/pgtable.h >>> @@ -401,7 +401,7 @@ static inline pmd_t *pmd_offset(pud_t *pud, unsigned long addr) >>> return (pmd_t *)pud_page_vaddr(*pud) + pmd_index(addr); >>> } >>> >>> -#define pud_page(pud) pmd_page(pud_pmd(pud)) >>> +#define pud_page(pud) pfn_to_page(__phys_to_pfn(pud_val(pud) & PHYS_MASK)) >> >> It would be cleaner to use pud_pfn here, otherwise we end up passing a >> physical address with the lower attributes present to __phys_to_pfn, which >> "knows" to shift them away. > > It looks like we did the same with pmd_page() but not with pte_page(), > the latter using pte_pfn(). > > It's not a problem with lower attributes because they are within the > first 12 bits, so the shifting gets rid of them. For pmd/pud/pgd, bits > above 12 and up to the actual address must be 0. > > I agree with changing all of them to use pte/pmd/pud/pgd_pfn but I'm > inclined to merge this patch now to fix the kernel build and we'll look > at the clean-up after Christmas. Either way, I'm fine. Best Regards Jungseok Lee
On Mon, Dec 22, 2014 at 03:11:37PM +0000, Jungseok Lee wrote: > On Dec 22, 2014, at 9:30 PM, Catalin Marinas wrote: > > On Sun, Dec 21, 2014 at 10:56:33AM +0000, Will Deacon wrote: > >> On Sat, Dec 20, 2014 at 12:49:40AM +0000, Jungseok Lee wrote: > >>> This patch adds pgd_page definition in order to keep supporting > >>> HAVE_GENERIC_RCU_GUP configuration. In addition, it changes pud_page > >>> expression to align with pmd_page for readability. > >>> > >>> An introduction of pgd_page resolves the following build breakage > >>> under 4KB + 4Level memory management combo. > >>> > >>> mm/gup.c: In function 'gup_huge_pgd': > >>> mm/gup.c:889:2: error: implicit declaration of function 'pgd_page' [-Werror=implicit-function-declaration] > >>> head = pgd_page(orig); > >>> ^ > >>> mm/gup.c:889:7: warning: assignment makes pointer from integer without a cast > >>> head = pgd_page(orig); > >>> > >>> Cc: Catalin Marinas <catalin.marinas@arm.com> > >>> Cc: Will Deacon <will.deacon@arm.com> > >>> Cc: Steve Capper <steve.capper@linaro.org> > >>> Signed-off-by: Jungseok Lee <jungseoklee85@gmail.com> > >>> --- > >>> arch/arm64/include/asm/pgtable.h | 4 +++- > >>> 1 file changed, 3 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h > >>> index df22314..a1fe927 100644 > >>> --- a/arch/arm64/include/asm/pgtable.h > >>> +++ b/arch/arm64/include/asm/pgtable.h > >>> @@ -401,7 +401,7 @@ static inline pmd_t *pmd_offset(pud_t *pud, unsigned long addr) > >>> return (pmd_t *)pud_page_vaddr(*pud) + pmd_index(addr); > >>> } > >>> > >>> -#define pud_page(pud) pmd_page(pud_pmd(pud)) > >>> +#define pud_page(pud) pfn_to_page(__phys_to_pfn(pud_val(pud) & PHYS_MASK)) > >> > >> It would be cleaner to use pud_pfn here, otherwise we end up passing a > >> physical address with the lower attributes present to __phys_to_pfn, which > >> "knows" to shift them away. > > > > It looks like we did the same with pmd_page() but not with pte_page(), > > the latter using pte_pfn(). > > > > It's not a problem with lower attributes because they are within the > > first 12 bits, so the shifting gets rid of them. For pmd/pud/pgd, bits > > above 12 and up to the actual address must be 0. > > > > I agree with changing all of them to use pte/pmd/pud/pgd_pfn but I'm > > inclined to merge this patch now to fix the kernel build and we'll look > > at the clean-up after Christmas. > > Either way, I'm fine. Unless you post an updated patch quickly enough ;)
On Mon, Dec 22, 2014 at 03:31:12PM +0000, Catalin Marinas wrote: > On Mon, Dec 22, 2014 at 03:11:37PM +0000, Jungseok Lee wrote: > > On Dec 22, 2014, at 9:30 PM, Catalin Marinas wrote: > > > On Sun, Dec 21, 2014 at 10:56:33AM +0000, Will Deacon wrote: > > >> On Sat, Dec 20, 2014 at 12:49:40AM +0000, Jungseok Lee wrote: > > >>> This patch adds pgd_page definition in order to keep supporting > > >>> HAVE_GENERIC_RCU_GUP configuration. In addition, it changes pud_page > > >>> expression to align with pmd_page for readability. > > >>> > > >>> An introduction of pgd_page resolves the following build breakage > > >>> under 4KB + 4Level memory management combo. > > >>> > > >>> mm/gup.c: In function 'gup_huge_pgd': > > >>> mm/gup.c:889:2: error: implicit declaration of function 'pgd_page' [-Werror=implicit-function-declaration] > > >>> head = pgd_page(orig); > > >>> ^ > > >>> mm/gup.c:889:7: warning: assignment makes pointer from integer without a cast > > >>> head = pgd_page(orig); > > >>> > > >>> Cc: Catalin Marinas <catalin.marinas@arm.com> > > >>> Cc: Will Deacon <will.deacon@arm.com> > > >>> Cc: Steve Capper <steve.capper@linaro.org> > > >>> Signed-off-by: Jungseok Lee <jungseoklee85@gmail.com> > > >>> --- > > >>> arch/arm64/include/asm/pgtable.h | 4 +++- > > >>> 1 file changed, 3 insertions(+), 1 deletion(-) > > >>> > > >>> diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h > > >>> index df22314..a1fe927 100644 > > >>> --- a/arch/arm64/include/asm/pgtable.h > > >>> +++ b/arch/arm64/include/asm/pgtable.h > > >>> @@ -401,7 +401,7 @@ static inline pmd_t *pmd_offset(pud_t *pud, unsigned long addr) > > >>> return (pmd_t *)pud_page_vaddr(*pud) + pmd_index(addr); > > >>> } > > >>> > > >>> -#define pud_page(pud) pmd_page(pud_pmd(pud)) > > >>> +#define pud_page(pud) pfn_to_page(__phys_to_pfn(pud_val(pud) & PHYS_MASK)) > > >> > > >> It would be cleaner to use pud_pfn here, otherwise we end up passing a > > >> physical address with the lower attributes present to __phys_to_pfn, which > > >> "knows" to shift them away. > > > > > > It looks like we did the same with pmd_page() but not with pte_page(), > > > the latter using pte_pfn(). > > > > > > It's not a problem with lower attributes because they are within the > > > first 12 bits, so the shifting gets rid of them. For pmd/pud/pgd, bits > > > above 12 and up to the actual address must be 0. > > > > > > I agree with changing all of them to use pte/pmd/pud/pgd_pfn but I'm > > > inclined to merge this patch now to fix the kernel build and we'll look > > > at the clean-up after Christmas. > > > > Either way, I'm fine. > > Unless you post an updated patch quickly enough ;) Actually, don't worry, I'll fold Will's suggestions into your patch. I found another problem with pmd_page being defined twice. Thanks.
On Sun, Dec 21, 2014 at 10:56:33AM +0000, Will Deacon wrote: > On Sat, Dec 20, 2014 at 12:49:40AM +0000, Jungseok Lee wrote: > > This patch adds pgd_page definition in order to keep supporting > > HAVE_GENERIC_RCU_GUP configuration. In addition, it changes pud_page > > expression to align with pmd_page for readability. > > > > An introduction of pgd_page resolves the following build breakage > > under 4KB + 4Level memory management combo. > > > > mm/gup.c: In function 'gup_huge_pgd': > > mm/gup.c:889:2: error: implicit declaration of function 'pgd_page' [-Werror=implicit-function-declaration] > > head = pgd_page(orig); > > ^ > > mm/gup.c:889:7: warning: assignment makes pointer from integer without a cast > > head = pgd_page(orig); > > > > Cc: Catalin Marinas <catalin.marinas@arm.com> > > Cc: Will Deacon <will.deacon@arm.com> > > Cc: Steve Capper <steve.capper@linaro.org> > > Signed-off-by: Jungseok Lee <jungseoklee85@gmail.com> > > --- > > arch/arm64/include/asm/pgtable.h | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h > > index df22314..a1fe927 100644 > > --- a/arch/arm64/include/asm/pgtable.h > > +++ b/arch/arm64/include/asm/pgtable.h > > @@ -401,7 +401,7 @@ static inline pmd_t *pmd_offset(pud_t *pud, unsigned long addr) > > return (pmd_t *)pud_page_vaddr(*pud) + pmd_index(addr); > > } > > > > -#define pud_page(pud) pmd_page(pud_pmd(pud)) > > +#define pud_page(pud) pfn_to_page(__phys_to_pfn(pud_val(pud) & PHYS_MASK)) > > It would be cleaner to use pud_pfn here, otherwise we end up passing a > physical address with the lower attributes present to __phys_to_pfn, which > "knows" to shift them away. OK, I tried this, together with aligning pmd_page to use pmd_pfn, and after debugging I realised why it is a bad idea. I had a feeling that I tried this before but didn't remember the details. I'll take the pmd_page() example, it is used in two scenarios: a) getting the first page structure of a huge page b) getting the page structure of the pte page pointed at by the pmd In the first case, we know that the pmd value is aligned to PMD_SIZE as it points to the physical address of a huge page, so pmd_pfn() masking is fine. In the second case, such masking is not, a pte page can be just PAGE_SIZE aligned. So using pmd_pfn to mask out the bits above 12 in the pmd val breaks case (b) above. While we don't have such use-case (b) for pud_page(), I would rather keep them consistent with pmd_page. A better fix would be to change such pmd_page() usage to pmd_pgtable() in the core mm code and have a different implementation for the latter. I guess we can leave this exercise to Steve ;). In the meantime, I'll merge Jungseok's original patch.
On Dec 24, 2014, at 12:25 AM, Catalin Marinas wrote: > On Sun, Dec 21, 2014 at 10:56:33AM +0000, Will Deacon wrote: >> On Sat, Dec 20, 2014 at 12:49:40AM +0000, Jungseok Lee wrote: >>> This patch adds pgd_page definition in order to keep supporting >>> HAVE_GENERIC_RCU_GUP configuration. In addition, it changes pud_page >>> expression to align with pmd_page for readability. >>> >>> An introduction of pgd_page resolves the following build breakage >>> under 4KB + 4Level memory management combo. >>> >>> mm/gup.c: In function 'gup_huge_pgd': >>> mm/gup.c:889:2: error: implicit declaration of function 'pgd_page' [-Werror=implicit-function-declaration] >>> head = pgd_page(orig); >>> ^ >>> mm/gup.c:889:7: warning: assignment makes pointer from integer without a cast >>> head = pgd_page(orig); >>> >>> Cc: Catalin Marinas <catalin.marinas@arm.com> >>> Cc: Will Deacon <will.deacon@arm.com> >>> Cc: Steve Capper <steve.capper@linaro.org> >>> Signed-off-by: Jungseok Lee <jungseoklee85@gmail.com> >>> --- >>> arch/arm64/include/asm/pgtable.h | 4 +++- >>> 1 file changed, 3 insertions(+), 1 deletion(-) >>> >>> diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h >>> index df22314..a1fe927 100644 >>> --- a/arch/arm64/include/asm/pgtable.h >>> +++ b/arch/arm64/include/asm/pgtable.h >>> @@ -401,7 +401,7 @@ static inline pmd_t *pmd_offset(pud_t *pud, unsigned long addr) >>> return (pmd_t *)pud_page_vaddr(*pud) + pmd_index(addr); >>> } >>> >>> -#define pud_page(pud) pmd_page(pud_pmd(pud)) >>> +#define pud_page(pud) pfn_to_page(__phys_to_pfn(pud_val(pud) & PHYS_MASK)) >> >> It would be cleaner to use pud_pfn here, otherwise we end up passing a >> physical address with the lower attributes present to __phys_to_pfn, which >> "knows" to shift them away. > > OK, I tried this, together with aligning pmd_page to use pmd_pfn, and > after debugging I realised why it is a bad idea. I had a feeling that I > tried this before but didn't remember the details. > > I'll take the pmd_page() example, it is used in two scenarios: > > a) getting the first page structure of a huge page > b) getting the page structure of the pte page pointed at by the pmd > > In the first case, we know that the pmd value is aligned to PMD_SIZE as > it points to the physical address of a huge page, so pmd_pfn() masking > is fine. In the second case, such masking is not, a pte page can be just > PAGE_SIZE aligned. So using pmd_pfn to mask out the bits above 12 in the > pmd val breaks case (b) above. > > While we don't have such use-case (b) for pud_page(), I would rather > keep them consistent with pmd_page. > > A better fix would be to change such pmd_page() usage to pmd_pgtable() > in the core mm code and have a different implementation for the latter. > I guess we can leave this exercise to Steve ;). > > In the meantime, I'll merge Jungseok's original patch. After reading this mail, I could imagine what happen to pmd_page(). Thanks Jungseok Lee
diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h index df22314..a1fe927 100644 --- a/arch/arm64/include/asm/pgtable.h +++ b/arch/arm64/include/asm/pgtable.h @@ -401,7 +401,7 @@ static inline pmd_t *pmd_offset(pud_t *pud, unsigned long addr) return (pmd_t *)pud_page_vaddr(*pud) + pmd_index(addr); } -#define pud_page(pud) pmd_page(pud_pmd(pud)) +#define pud_page(pud) pfn_to_page(__phys_to_pfn(pud_val(pud) & PHYS_MASK)) #endif /* CONFIG_ARM64_PGTABLE_LEVELS > 2 */ @@ -437,6 +437,8 @@ static inline pud_t *pud_offset(pgd_t *pgd, unsigned long addr) return (pud_t *)pgd_page_vaddr(*pgd) + pud_index(addr); } +#define pgd_page(pgd) pfn_to_page(__phys_to_pfn(pgd_val(pgd) & PHYS_MASK)) + #endif /* CONFIG_ARM64_PGTABLE_LEVELS > 3 */ #define pgd_ERROR(pgd) __pgd_error(__FILE__, __LINE__, pgd_val(pgd))
This patch adds pgd_page definition in order to keep supporting HAVE_GENERIC_RCU_GUP configuration. In addition, it changes pud_page expression to align with pmd_page for readability. An introduction of pgd_page resolves the following build breakage under 4KB + 4Level memory management combo. mm/gup.c: In function 'gup_huge_pgd': mm/gup.c:889:2: error: implicit declaration of function 'pgd_page' [-Werror=implicit-function-declaration] head = pgd_page(orig); ^ mm/gup.c:889:7: warning: assignment makes pointer from integer without a cast head = pgd_page(orig); Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: Will Deacon <will.deacon@arm.com> Cc: Steve Capper <steve.capper@linaro.org> Signed-off-by: Jungseok Lee <jungseoklee85@gmail.com> --- arch/arm64/include/asm/pgtable.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)