Re: [PATCH v3 2/2] mm: remove odd HAVE_PTE_SPECIAL
On Wed 11-04-18 12:32:07, Laurent Dufour wrote: [...] > Andrew, should I send a v4 or could you wipe the 2 __maybe_unsued when > applying > the patch ? A follow $patch-fix should be better rather than post this again and spam people with more emails. -- Michal Hocko SUSE Labs -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/2] mm: remove odd HAVE_PTE_SPECIAL
On 11/04/2018 11:09, Christophe LEROY wrote: > > > Le 11/04/2018 à 11:03, Laurent Dufour a écrit : >> >> >> On 11/04/2018 10:58, Christophe LEROY wrote: >>> >>> >>> Le 11/04/2018 à 10:03, Laurent Dufour a écrit : Remove the additional define HAVE_PTE_SPECIAL and rely directly on CONFIG_ARCH_HAS_PTE_SPECIAL. There is no functional change introduced by this patch Signed-off-by: Laurent Dufour --- mm/memory.c | 19 --- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/mm/memory.c b/mm/memory.c index 96910c625daa..7f7dc7b2a341 100644 --- a/mm/memory.c +++ b/mm/memory.c @@ -817,17 +817,12 @@ static void print_bad_pte(struct vm_area_struct *vma, unsigned long addr, * PFNMAP mappings in order to support COWable mappings. * */ -#ifdef CONFIG_ARCH_HAS_PTE_SPECIAL -# define HAVE_PTE_SPECIAL 1 -#else -# define HAVE_PTE_SPECIAL 0 -#endif struct page *_vm_normal_page(struct vm_area_struct *vma, unsigned long addr, pte_t pte, bool with_public_device) { unsigned long pfn = pte_pfn(pte); - if (HAVE_PTE_SPECIAL) { + if (IS_ENABLED(CONFIG_ARCH_HAS_PTE_SPECIAL)) { if (likely(!pte_special(pte))) goto check_pfn; if (vma->vm_ops && vma->vm_ops->find_special_page) @@ -862,7 +857,7 @@ struct page *_vm_normal_page(struct vm_area_struct *vma, unsigned long addr, return NULL; } - /* !HAVE_PTE_SPECIAL case follows: */ + /* !CONFIG_ARCH_HAS_PTE_SPECIAL case follows: */ if (unlikely(vma->vm_flags & (VM_PFNMAP|VM_MIXEDMAP))) { if (vma->vm_flags & VM_MIXEDMAP) { @@ -881,7 +876,8 @@ struct page *_vm_normal_page(struct vm_area_struct *vma, unsigned long addr, if (is_zero_pfn(pfn)) return NULL; -check_pfn: + +check_pfn: __maybe_unused >>> >>> See below >>> if (unlikely(pfn > highest_memmap_pfn)) { print_bad_pte(vma, addr, pte, NULL); return NULL; @@ -891,7 +887,7 @@ struct page *_vm_normal_page(struct vm_area_struct *vma, unsigned long addr, * NOTE! We still have PageReserved() pages in the page tables. * eg. VDSO mappings can cause them to exist. */ -out: +out: __maybe_unused >>> >>> Why do you need that change ? >>> >>> There is no reason for the compiler to complain. It would complain if the >>> goto >>> was within a #ifdef, but all the purpose of using IS_ENABLED() is to allow >>> the >>> compiler to properly handle all possible cases. That's all the force of >>> IS_ENABLED() compared to ifdefs, and that the reason why they are >>> plebicited, >>> ref Linux Codying style for a detailed explanation. >> >> Fair enough. >> >> Should I submit a v4 just to remove these so ugly __maybe_unused ? >> > > Most likely, unless the mm maintainer agrees to remove them by himself when > applying your patch ? That was my point. Andrew, should I send a v4 or could you wipe the 2 __maybe_unsued when applying the patch ? Thanks, Laurent. -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/2] mm: remove odd HAVE_PTE_SPECIAL
Le 11/04/2018 à 11:03, Laurent Dufour a écrit : On 11/04/2018 10:58, Christophe LEROY wrote: Le 11/04/2018 à 10:03, Laurent Dufour a écrit : Remove the additional define HAVE_PTE_SPECIAL and rely directly on CONFIG_ARCH_HAS_PTE_SPECIAL. There is no functional change introduced by this patch Signed-off-by: Laurent Dufour --- mm/memory.c | 19 --- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/mm/memory.c b/mm/memory.c index 96910c625daa..7f7dc7b2a341 100644 --- a/mm/memory.c +++ b/mm/memory.c @@ -817,17 +817,12 @@ static void print_bad_pte(struct vm_area_struct *vma, unsigned long addr, * PFNMAP mappings in order to support COWable mappings. * */ -#ifdef CONFIG_ARCH_HAS_PTE_SPECIAL -# define HAVE_PTE_SPECIAL 1 -#else -# define HAVE_PTE_SPECIAL 0 -#endif struct page *_vm_normal_page(struct vm_area_struct *vma, unsigned long addr, pte_t pte, bool with_public_device) { unsigned long pfn = pte_pfn(pte); - if (HAVE_PTE_SPECIAL) { + if (IS_ENABLED(CONFIG_ARCH_HAS_PTE_SPECIAL)) { if (likely(!pte_special(pte))) goto check_pfn; if (vma->vm_ops && vma->vm_ops->find_special_page) @@ -862,7 +857,7 @@ struct page *_vm_normal_page(struct vm_area_struct *vma, unsigned long addr, return NULL; } - /* !HAVE_PTE_SPECIAL case follows: */ + /* !CONFIG_ARCH_HAS_PTE_SPECIAL case follows: */ if (unlikely(vma->vm_flags & (VM_PFNMAP|VM_MIXEDMAP))) { if (vma->vm_flags & VM_MIXEDMAP) { @@ -881,7 +876,8 @@ struct page *_vm_normal_page(struct vm_area_struct *vma, unsigned long addr, if (is_zero_pfn(pfn)) return NULL; -check_pfn: + +check_pfn: __maybe_unused See below if (unlikely(pfn > highest_memmap_pfn)) { print_bad_pte(vma, addr, pte, NULL); return NULL; @@ -891,7 +887,7 @@ struct page *_vm_normal_page(struct vm_area_struct *vma, unsigned long addr, * NOTE! We still have PageReserved() pages in the page tables. * eg. VDSO mappings can cause them to exist. */ -out: +out: __maybe_unused Why do you need that change ? There is no reason for the compiler to complain. It would complain if the goto was within a #ifdef, but all the purpose of using IS_ENABLED() is to allow the compiler to properly handle all possible cases. That's all the force of IS_ENABLED() compared to ifdefs, and that the reason why they are plebicited, ref Linux Codying style for a detailed explanation. Fair enough. Should I submit a v4 just to remove these so ugly __maybe_unused ? Most likely, unless the mm maintainer agrees to remove them by himself when applying your patch ? Christophe -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/2] mm: remove odd HAVE_PTE_SPECIAL
On 11/04/2018 10:58, Christophe LEROY wrote: > > > Le 11/04/2018 à 10:03, Laurent Dufour a écrit : >> Remove the additional define HAVE_PTE_SPECIAL and rely directly on >> CONFIG_ARCH_HAS_PTE_SPECIAL. >> >> There is no functional change introduced by this patch >> >> Signed-off-by: Laurent Dufour >> --- >> mm/memory.c | 19 --- >> 1 file changed, 8 insertions(+), 11 deletions(-) >> >> diff --git a/mm/memory.c b/mm/memory.c >> index 96910c625daa..7f7dc7b2a341 100644 >> --- a/mm/memory.c >> +++ b/mm/memory.c >> @@ -817,17 +817,12 @@ static void print_bad_pte(struct vm_area_struct *vma, >> unsigned long addr, >> * PFNMAP mappings in order to support COWable mappings. >> * >> */ >> -#ifdef CONFIG_ARCH_HAS_PTE_SPECIAL >> -# define HAVE_PTE_SPECIAL 1 >> -#else >> -# define HAVE_PTE_SPECIAL 0 >> -#endif >> struct page *_vm_normal_page(struct vm_area_struct *vma, unsigned long >> addr, >> pte_t pte, bool with_public_device) >> { >> unsigned long pfn = pte_pfn(pte); >> - if (HAVE_PTE_SPECIAL) { >> + if (IS_ENABLED(CONFIG_ARCH_HAS_PTE_SPECIAL)) { >> if (likely(!pte_special(pte))) >> goto check_pfn; >> if (vma->vm_ops && vma->vm_ops->find_special_page) >> @@ -862,7 +857,7 @@ struct page *_vm_normal_page(struct vm_area_struct *vma, >> unsigned long addr, >> return NULL; >> } >> - /* !HAVE_PTE_SPECIAL case follows: */ >> + /* !CONFIG_ARCH_HAS_PTE_SPECIAL case follows: */ >> if (unlikely(vma->vm_flags & (VM_PFNMAP|VM_MIXEDMAP))) { >> if (vma->vm_flags & VM_MIXEDMAP) { >> @@ -881,7 +876,8 @@ struct page *_vm_normal_page(struct vm_area_struct *vma, >> unsigned long addr, >> if (is_zero_pfn(pfn)) >> return NULL; >> -check_pfn: >> + >> +check_pfn: __maybe_unused > > See below > >> if (unlikely(pfn > highest_memmap_pfn)) { >> print_bad_pte(vma, addr, pte, NULL); >> return NULL; >> @@ -891,7 +887,7 @@ struct page *_vm_normal_page(struct vm_area_struct *vma, >> unsigned long addr, >> * NOTE! We still have PageReserved() pages in the page tables. >> * eg. VDSO mappings can cause them to exist. >> */ >> -out: >> +out: __maybe_unused > > Why do you need that change ? > > There is no reason for the compiler to complain. It would complain if the goto > was within a #ifdef, but all the purpose of using IS_ENABLED() is to allow the > compiler to properly handle all possible cases. That's all the force of > IS_ENABLED() compared to ifdefs, and that the reason why they are plebicited, > ref Linux Codying style for a detailed explanation. Fair enough. Should I submit a v4 just to remove these so ugly __maybe_unused ? -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/2] mm: remove odd HAVE_PTE_SPECIAL
Le 11/04/2018 à 10:41, Laurent Dufour a écrit : On 11/04/2018 10:33, Michal Hocko wrote: On Wed 11-04-18 10:03:36, Laurent Dufour wrote: @@ -881,7 +876,8 @@ struct page *_vm_normal_page(struct vm_area_struct *vma, unsigned long addr, if (is_zero_pfn(pfn)) return NULL; -check_pfn: + +check_pfn: __maybe_unused if (unlikely(pfn > highest_memmap_pfn)) { print_bad_pte(vma, addr, pte, NULL); return NULL; @@ -891,7 +887,7 @@ struct page *_vm_normal_page(struct vm_area_struct *vma, unsigned long addr, * NOTE! We still have PageReserved() pages in the page tables. * eg. VDSO mappings can cause them to exist. */ -out: +out: __maybe_unused return pfn_to_page(pfn); Why do we need this ugliness all of the sudden? Indeed the compiler doesn't complaint but in theory it should since these labels are not used depending on CONFIG_ARCH_HAS_PTE_SPECIAL. Why should it complain ? Regards Christophe -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/2] mm: remove odd HAVE_PTE_SPECIAL
Le 11/04/2018 à 10:03, Laurent Dufour a écrit : Remove the additional define HAVE_PTE_SPECIAL and rely directly on CONFIG_ARCH_HAS_PTE_SPECIAL. There is no functional change introduced by this patch Signed-off-by: Laurent Dufour --- mm/memory.c | 19 --- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/mm/memory.c b/mm/memory.c index 96910c625daa..7f7dc7b2a341 100644 --- a/mm/memory.c +++ b/mm/memory.c @@ -817,17 +817,12 @@ static void print_bad_pte(struct vm_area_struct *vma, unsigned long addr, * PFNMAP mappings in order to support COWable mappings. * */ -#ifdef CONFIG_ARCH_HAS_PTE_SPECIAL -# define HAVE_PTE_SPECIAL 1 -#else -# define HAVE_PTE_SPECIAL 0 -#endif struct page *_vm_normal_page(struct vm_area_struct *vma, unsigned long addr, pte_t pte, bool with_public_device) { unsigned long pfn = pte_pfn(pte); - if (HAVE_PTE_SPECIAL) { + if (IS_ENABLED(CONFIG_ARCH_HAS_PTE_SPECIAL)) { if (likely(!pte_special(pte))) goto check_pfn; if (vma->vm_ops && vma->vm_ops->find_special_page) @@ -862,7 +857,7 @@ struct page *_vm_normal_page(struct vm_area_struct *vma, unsigned long addr, return NULL; } - /* !HAVE_PTE_SPECIAL case follows: */ + /* !CONFIG_ARCH_HAS_PTE_SPECIAL case follows: */ if (unlikely(vma->vm_flags & (VM_PFNMAP|VM_MIXEDMAP))) { if (vma->vm_flags & VM_MIXEDMAP) { @@ -881,7 +876,8 @@ struct page *_vm_normal_page(struct vm_area_struct *vma, unsigned long addr, if (is_zero_pfn(pfn)) return NULL; -check_pfn: + +check_pfn: __maybe_unused See below if (unlikely(pfn > highest_memmap_pfn)) { print_bad_pte(vma, addr, pte, NULL); return NULL; @@ -891,7 +887,7 @@ struct page *_vm_normal_page(struct vm_area_struct *vma, unsigned long addr, * NOTE! We still have PageReserved() pages in the page tables. * eg. VDSO mappings can cause them to exist. */ -out: +out: __maybe_unused Why do you need that change ? There is no reason for the compiler to complain. It would complain if the goto was within a #ifdef, but all the purpose of using IS_ENABLED() is to allow the compiler to properly handle all possible cases. That's all the force of IS_ENABLED() compared to ifdefs, and that the reason why they are plebicited, ref Linux Codying style for a detailed explanation. Christophe return pfn_to_page(pfn); } @@ -904,7 +900,7 @@ struct page *vm_normal_page_pmd(struct vm_area_struct *vma, unsigned long addr, /* * There is no pmd_special() but there may be special pmds, e.g. * in a direct-access (dax) mapping, so let's just replicate the -* !HAVE_PTE_SPECIAL case from vm_normal_page() here. +* !CONFIG_ARCH_HAS_PTE_SPECIAL case from vm_normal_page() here. */ if (unlikely(vma->vm_flags & (VM_PFNMAP|VM_MIXEDMAP))) { if (vma->vm_flags & VM_MIXEDMAP) { @@ -1933,7 +1929,8 @@ static int __vm_insert_mixed(struct vm_area_struct *vma, unsigned long addr, * than insert_pfn). If a zero_pfn were inserted into a VM_MIXEDMAP * without pte special, it would there be refcounted as a normal page. */ - if (!HAVE_PTE_SPECIAL && !pfn_t_devmap(pfn) && pfn_t_valid(pfn)) { + if (!IS_ENABLED(CONFIG_ARCH_HAS_PTE_SPECIAL) && + !pfn_t_devmap(pfn) && pfn_t_valid(pfn)) { struct page *page; /* -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/2] mm: remove odd HAVE_PTE_SPECIAL
On Wed 11-04-18 10:41:23, Laurent Dufour wrote: > On 11/04/2018 10:33, Michal Hocko wrote: > > On Wed 11-04-18 10:03:36, Laurent Dufour wrote: > >> @@ -881,7 +876,8 @@ struct page *_vm_normal_page(struct vm_area_struct > >> *vma, unsigned long addr, > >> > >>if (is_zero_pfn(pfn)) > >>return NULL; > >> -check_pfn: > >> + > >> +check_pfn: __maybe_unused > >>if (unlikely(pfn > highest_memmap_pfn)) { > >>print_bad_pte(vma, addr, pte, NULL); > >>return NULL; > >> @@ -891,7 +887,7 @@ struct page *_vm_normal_page(struct vm_area_struct > >> *vma, unsigned long addr, > >> * NOTE! We still have PageReserved() pages in the page tables. > >> * eg. VDSO mappings can cause them to exist. > >> */ > >> -out: > >> +out: __maybe_unused > >>return pfn_to_page(pfn); > > > > Why do we need this ugliness all of the sudden? > Indeed the compiler doesn't complaint but in theory it should since these > labels are not used depending on CONFIG_ARCH_HAS_PTE_SPECIAL. Well, such a warning would be quite pointless so I would rather not make the code ugly. The value of unused label is quite questionable to start with... -- Michal Hocko SUSE Labs -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/2] mm: remove odd HAVE_PTE_SPECIAL
On 11/04/2018 10:33, Michal Hocko wrote: > On Wed 11-04-18 10:03:36, Laurent Dufour wrote: >> @@ -881,7 +876,8 @@ struct page *_vm_normal_page(struct vm_area_struct *vma, >> unsigned long addr, >> >> if (is_zero_pfn(pfn)) >> return NULL; >> -check_pfn: >> + >> +check_pfn: __maybe_unused >> if (unlikely(pfn > highest_memmap_pfn)) { >> print_bad_pte(vma, addr, pte, NULL); >> return NULL; >> @@ -891,7 +887,7 @@ struct page *_vm_normal_page(struct vm_area_struct *vma, >> unsigned long addr, >> * NOTE! We still have PageReserved() pages in the page tables. >> * eg. VDSO mappings can cause them to exist. >> */ >> -out: >> +out: __maybe_unused >> return pfn_to_page(pfn); > > Why do we need this ugliness all of the sudden? Indeed the compiler doesn't complaint but in theory it should since these labels are not used depending on CONFIG_ARCH_HAS_PTE_SPECIAL. -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/2] mm: remove odd HAVE_PTE_SPECIAL
On Wed 11-04-18 10:03:36, Laurent Dufour wrote: > @@ -881,7 +876,8 @@ struct page *_vm_normal_page(struct vm_area_struct *vma, > unsigned long addr, > > if (is_zero_pfn(pfn)) > return NULL; > -check_pfn: > + > +check_pfn: __maybe_unused > if (unlikely(pfn > highest_memmap_pfn)) { > print_bad_pte(vma, addr, pte, NULL); > return NULL; > @@ -891,7 +887,7 @@ struct page *_vm_normal_page(struct vm_area_struct *vma, > unsigned long addr, >* NOTE! We still have PageReserved() pages in the page tables. >* eg. VDSO mappings can cause them to exist. >*/ > -out: > +out: __maybe_unused > return pfn_to_page(pfn); Why do we need this ugliness all of the sudden? -- Michal Hocko SUSE Labs -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html