| Message ID | 20181211200526.3868586-1-arnd@arndb.de |
|---|---|
| State | New |
| Headers | show |
| Series | mm/mmu_notifier: fix mmu_notifier_range_init warning | expand |
On Tue, Dec 11, 2018 at 09:04:43PM +0100, Arnd Bergmann wrote: > The macro version of mmu_notifier_range_init() for CONFIG_MMU_NOTIFIER=n > does not evaluate all its arguments, leading to a warning in one case: > > mm/migrate.c: In function 'migrate_vma_pages': > mm/migrate.c:2711:20: error: unused variable 'mm' [-Werror=unused-variable] > struct mm_struct *mm = vma->vm_mm; > > Pass down the 'mm' as into the inline function as well so gcc can > see why the variable exists. > > Fixes: 137d92bd73b1 ("mm/mmu_notifier: use structure for invalidate_range_start/end calls v2") What about changing migrate.c (i actualy tried to do that everywhere in the patchset but i missed that spot) So we avoid one useless variable on such configuration: diff --git a/mm/migrate.c b/mm/migrate.c index f02bb4b22c1a..883fce631f47 100644 --- a/mm/migrate.c +++ b/mm/migrate.c @@ -2701,7 +2701,6 @@ static void migrate_vma_pages(struct migrate_vma *migrate) const unsigned long npages = migrate->npages; const unsigned long start = migrate->start; struct vm_area_struct *vma = migrate->vma; - struct mm_struct *mm = vma->vm_mm; struct mmu_notifier_range range; unsigned long addr, i; bool notified = false; @@ -2724,8 +2723,8 @@ static void migrate_vma_pages(struct migrate_vma *migrate) if (!notified) { notified = true; - mmu_notifier_range_init(&range, mm, addr, - migrate->end, + mmu_notifier_range_init(&range, vma->vm_mm, + addr, migrate->end, MMU_NOTIFY_CLEAR); mmu_notifier_invalidate_range_start(&range); } > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > include/linux/mmu_notifier.h | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/include/linux/mmu_notifier.h b/include/linux/mmu_notifier.h > index 29f7b9670ba3..b13ea00ded5d 100644 > --- a/include/linux/mmu_notifier.h > +++ b/include/linux/mmu_notifier.h > @@ -476,6 +476,7 @@ struct mmu_notifier_range { > }; > > static inline void _mmu_notifier_range_init(struct mmu_notifier_range *range, > + struct mm_struct *mm, > unsigned long start, > unsigned long end) > { > @@ -484,7 +485,7 @@ static inline void _mmu_notifier_range_init(struct mmu_notifier_range *range, > } > > #define mmu_notifier_range_init(range, mm, start, end, event) \ > - _mmu_notifier_range_init(range, start, end) > + _mmu_notifier_range_init(range, mm, start, end) > > > static inline int mm_has_notifiers(struct mm_struct *mm) > -- > 2.20.0 >
On Tue, 11 Dec 2018, Jerome Glisse wrote: > On Tue, Dec 11, 2018 at 09:04:43PM +0100, Arnd Bergmann wrote: > > The macro version of mmu_notifier_range_init() for CONFIG_MMU_NOTIFIER=n > > does not evaluate all its arguments, leading to a warning in one case: > > > > mm/migrate.c: In function 'migrate_vma_pages': > > mm/migrate.c:2711:20: error: unused variable 'mm' [-Werror=unused-variable] > > struct mm_struct *mm = vma->vm_mm; > > > > Pass down the 'mm' as into the inline function as well so gcc can > > see why the variable exists. > > > > Fixes: 137d92bd73b1 ("mm/mmu_notifier: use structure for invalidate_range_start/end calls v2") > > What about changing migrate.c (i actualy tried to do that everywhere in > the patchset but i missed that spot) So we avoid one useless variable on > such configuration: > > diff --git a/mm/migrate.c b/mm/migrate.c > index f02bb4b22c1a..883fce631f47 100644 > --- a/mm/migrate.c > +++ b/mm/migrate.c > @@ -2701,7 +2701,6 @@ static void migrate_vma_pages(struct migrate_vma *migrate) > const unsigned long npages = migrate->npages; > const unsigned long start = migrate->start; > struct vm_area_struct *vma = migrate->vma; > - struct mm_struct *mm = vma->vm_mm; > struct mmu_notifier_range range; > unsigned long addr, i; > bool notified = false; > @@ -2724,8 +2723,8 @@ static void migrate_vma_pages(struct migrate_vma *migrate) > if (!notified) { > notified = true; > > - mmu_notifier_range_init(&range, mm, addr, > - migrate->end, > + mmu_notifier_range_init(&range, vma->vm_mm, > + addr, migrate->end, > MMU_NOTIFY_CLEAR); > mmu_notifier_invalidate_range_start(&range); > } Wouldn't it be better to just declare mmu_notifier_range_init() as a static inline function rather than a #define to avoid this warning?
On Tue, Dec 11, 2018 at 01:12:54PM -0800, David Rientjes wrote: > On Tue, 11 Dec 2018, Jerome Glisse wrote: > > > On Tue, Dec 11, 2018 at 09:04:43PM +0100, Arnd Bergmann wrote: > > > The macro version of mmu_notifier_range_init() for CONFIG_MMU_NOTIFIER=n > > > does not evaluate all its arguments, leading to a warning in one case: > > > > > > mm/migrate.c: In function 'migrate_vma_pages': > > > mm/migrate.c:2711:20: error: unused variable 'mm' [-Werror=unused-variable] > > > struct mm_struct *mm = vma->vm_mm; > > > > > > Pass down the 'mm' as into the inline function as well so gcc can > > > see why the variable exists. > > > > > > Fixes: 137d92bd73b1 ("mm/mmu_notifier: use structure for invalidate_range_start/end calls v2") > > > > What about changing migrate.c (i actualy tried to do that everywhere in > > the patchset but i missed that spot) So we avoid one useless variable on > > such configuration: > > > > diff --git a/mm/migrate.c b/mm/migrate.c > > index f02bb4b22c1a..883fce631f47 100644 > > --- a/mm/migrate.c > > +++ b/mm/migrate.c > > @@ -2701,7 +2701,6 @@ static void migrate_vma_pages(struct migrate_vma *migrate) > > const unsigned long npages = migrate->npages; > > const unsigned long start = migrate->start; > > struct vm_area_struct *vma = migrate->vma; > > - struct mm_struct *mm = vma->vm_mm; > > struct mmu_notifier_range range; > > unsigned long addr, i; > > bool notified = false; > > @@ -2724,8 +2723,8 @@ static void migrate_vma_pages(struct migrate_vma *migrate) > > if (!notified) { > > notified = true; > > > > - mmu_notifier_range_init(&range, mm, addr, > > - migrate->end, > > + mmu_notifier_range_init(&range, vma->vm_mm, > > + addr, migrate->end, > > MMU_NOTIFY_CLEAR); > > mmu_notifier_invalidate_range_start(&range); > > } > > Wouldn't it be better to just declare mmu_notifier_range_init() as a > static inline function rather than a #define to avoid this warning? The define trick is use to drop the event parameter so that we do not need to define the event value for CONFIG_MMU_NOTIFIER=n case. Cheers, Jérôme
On Tue, 11 Dec 2018, Jerome Glisse wrote: > > > > The macro version of mmu_notifier_range_init() for CONFIG_MMU_NOTIFIER=n > > > > does not evaluate all its arguments, leading to a warning in one case: > > > > > > > > mm/migrate.c: In function 'migrate_vma_pages': > > > > mm/migrate.c:2711:20: error: unused variable 'mm' [-Werror=unused-variable] > > > > struct mm_struct *mm = vma->vm_mm; > > > > > > > > Pass down the 'mm' as into the inline function as well so gcc can > > > > see why the variable exists. > > > > > > > > Fixes: 137d92bd73b1 ("mm/mmu_notifier: use structure for invalidate_range_start/end calls v2") > > > > > > What about changing migrate.c (i actualy tried to do that everywhere in > > > the patchset but i missed that spot) So we avoid one useless variable on > > > such configuration: > > > > > > diff --git a/mm/migrate.c b/mm/migrate.c > > > index f02bb4b22c1a..883fce631f47 100644 > > > --- a/mm/migrate.c > > > +++ b/mm/migrate.c > > > @@ -2701,7 +2701,6 @@ static void migrate_vma_pages(struct migrate_vma *migrate) > > > const unsigned long npages = migrate->npages; > > > const unsigned long start = migrate->start; > > > struct vm_area_struct *vma = migrate->vma; > > > - struct mm_struct *mm = vma->vm_mm; > > > struct mmu_notifier_range range; > > > unsigned long addr, i; > > > bool notified = false; > > > @@ -2724,8 +2723,8 @@ static void migrate_vma_pages(struct migrate_vma *migrate) > > > if (!notified) { > > > notified = true; > > > > > > - mmu_notifier_range_init(&range, mm, addr, > > > - migrate->end, > > > + mmu_notifier_range_init(&range, vma->vm_mm, > > > + addr, migrate->end, > > > MMU_NOTIFY_CLEAR); > > > mmu_notifier_invalidate_range_start(&range); > > > } > > > > Wouldn't it be better to just declare mmu_notifier_range_init() as a > > static inline function rather than a #define to avoid this warning? > > The define trick is use to drop the event parameter so that we do not > need to define the event value for CONFIG_MMU_NOTIFIER=n case. > Hmm, strange that Arnd's build failure is only reporting about an unused variable instead of MMU_NOTIFY_CLEAR being undefined :/ I think this should be done so that anybody using mmu_notifier_range_init() doesn't need to worry about the implications of *any* unused formal parameter as a result of how the #define is formed: diff --git a/include/linux/mmu_notifier.h b/include/linux/mmu_notifier.h --- a/include/linux/mmu_notifier.h +++ b/include/linux/mmu_notifier.h @@ -10,21 +10,6 @@ struct mmu_notifier; struct mmu_notifier_ops; -#ifdef CONFIG_MMU_NOTIFIER - -/* - * The mmu notifier_mm structure is allocated and installed in - * mm->mmu_notifier_mm inside the mm_take_all_locks() protected - * critical section and it's released only when mm_count reaches zero - * in mmdrop(). - */ -struct mmu_notifier_mm { - /* all mmu notifiers registerd in this mm are queued in this list */ - struct hlist_head list; - /* to serialize the list modifications and hlist_unhashed */ - spinlock_t lock; -}; - /** * enum mmu_notifier_event - reason for the mmu notifier callback * @MMU_NOTIFY_UNMAP: either munmap() that unmap the range or a mremap() that @@ -53,6 +38,21 @@ enum mmu_notifier_event { MMU_NOTIFY_SOFT_DIRTY, }; +#ifdef CONFIG_MMU_NOTIFIER + +/* + * The mmu notifier_mm structure is allocated and installed in + * mm->mmu_notifier_mm inside the mm_take_all_locks() protected + * critical section and it's released only when mm_count reaches zero + * in mmdrop(). + */ +struct mmu_notifier_mm { + /* all mmu notifiers registerd in this mm are queued in this list */ + struct hlist_head list; + /* to serialize the list modifications and hlist_unhashed */ + spinlock_t lock; +}; + struct mmu_notifier_range { struct mm_struct *mm; unsigned long start; @@ -483,9 +483,14 @@ static inline void _mmu_notifier_range_init(struct mmu_notifier_range *range, range->end = end; } -#define mmu_notifier_range_init(range, mm, start, end, event) \ - _mmu_notifier_range_init(range, start, end) - +static inline void mmu_notifier_range_init(struct mmu_notifier_range *range, + struct mm_struct *mm, + unsigned long start, + unsigned long end, + enum mmu_notifier_event event) +{ + _mmu_notifier_range_init(range, start, end); +} static inline int mm_has_notifiers(struct mm_struct *mm) {
On Tue, Dec 11, 2018 at 10:43 PM David Rientjes <rientjes@google.com> wrote: > > On Tue, 11 Dec 2018, Jerome Glisse wrote: > > Hmm, strange that Arnd's build failure is only reporting about an unused > variable instead of MMU_NOTIFY_CLEAR being undefined :/ > > I think this should be done so that anybody using > mmu_notifier_range_init() doesn't need to worry about the implications of > *any* unused formal parameter as a result of how the #define is formed: Your patch below is more or less what I tried at first, and that resulted in another build failure for mm/hugetlb.c: mmu_notifier_range_init(&range, mm, start, end, MMU_NOTIFY_CLEAR); mm/hugetlb.c- adjust_range_if_pmd_sharing_possible(vma, &range.start, &range.end); where range.end refers to a nonexisting member of range. Arnd
On Tue, Dec 11, 2018 at 10:53:03PM +0100, Arnd Bergmann wrote: > On Tue, Dec 11, 2018 at 10:43 PM David Rientjes <rientjes@google.com> wrote: > > > > On Tue, 11 Dec 2018, Jerome Glisse wrote: > > > > > Hmm, strange that Arnd's build failure is only reporting about an unused > > variable instead of MMU_NOTIFY_CLEAR being undefined :/ > > > > I think this should be done so that anybody using > > mmu_notifier_range_init() doesn't need to worry about the implications of > > *any* unused formal parameter as a result of how the #define is formed: > > Your patch below is more or less what I tried at first, and that resulted > in another build failure for > > mm/hugetlb.c: mmu_notifier_range_init(&range, mm, start, end, > MMU_NOTIFY_CLEAR); > mm/hugetlb.c- adjust_range_if_pmd_sharing_possible(vma, > &range.start, &range.end); > > where range.end refers to a nonexisting member of range. > > Arnd I will post a v3 with htmldoc fix and build warning fix incorporated. Cheers, Jérôme
On Tue, 11 Dec 2018, Arnd Bergmann wrote: > > Hmm, strange that Arnd's build failure is only reporting about an unused > > variable instead of MMU_NOTIFY_CLEAR being undefined :/ > > > > I think this should be done so that anybody using > > mmu_notifier_range_init() doesn't need to worry about the implications of > > *any* unused formal parameter as a result of how the #define is formed: > > Your patch below is more or less what I tried at first, and that resulted > in another build failure for > > mm/hugetlb.c: mmu_notifier_range_init(&range, mm, start, end, > MMU_NOTIFY_CLEAR); > mm/hugetlb.c- adjust_range_if_pmd_sharing_possible(vma, > &range.start, &range.end); > > where range.end refers to a nonexisting member of range. > Isn't that separate? It seems like there is a strict dependency that CONFIG_HUGETLBFS has on CONFIG_MMU_NOTIFIER.
diff --git a/include/linux/mmu_notifier.h b/include/linux/mmu_notifier.h index 29f7b9670ba3..b13ea00ded5d 100644 --- a/include/linux/mmu_notifier.h +++ b/include/linux/mmu_notifier.h @@ -476,6 +476,7 @@ struct mmu_notifier_range { }; static inline void _mmu_notifier_range_init(struct mmu_notifier_range *range, + struct mm_struct *mm, unsigned long start, unsigned long end) { @@ -484,7 +485,7 @@ static inline void _mmu_notifier_range_init(struct mmu_notifier_range *range, } #define mmu_notifier_range_init(range, mm, start, end, event) \ - _mmu_notifier_range_init(range, start, end) + _mmu_notifier_range_init(range, mm, start, end) static inline int mm_has_notifiers(struct mm_struct *mm)
The macro version of mmu_notifier_range_init() for CONFIG_MMU_NOTIFIER=n does not evaluate all its arguments, leading to a warning in one case: mm/migrate.c: In function 'migrate_vma_pages': mm/migrate.c:2711:20: error: unused variable 'mm' [-Werror=unused-variable] struct mm_struct *mm = vma->vm_mm; Pass down the 'mm' as into the inline function as well so gcc can see why the variable exists. Fixes: 137d92bd73b1 ("mm/mmu_notifier: use structure for invalidate_range_start/end calls v2") Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- include/linux/mmu_notifier.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) -- 2.20.0