* [PATCH] mm/filemap: add static for function __add_to_page_cache_locked @ 2020-11-06 11:24 Alex Shi 2020-11-06 18:24 ` Ira Weiny 2020-11-10 3:09 ` Souptick Joarder 0 siblings, 2 replies; 22+ messages in thread From: Alex Shi @ 2020-11-06 11:24 UTC (permalink / raw) To: akpm; +Cc: linux-mm, linux-kernel Otherwise it cause gcc warning: ^~~~~~~~~~~~~~~ ../mm/filemap.c:830:14: warning: no previous prototype for ‘__add_to_page_cache_locked’ [-Wmissing-prototypes] noinline int __add_to_page_cache_locked(struct page *page, ^~~~~~~~~~~~~~~~~~~~~~~~~~ Signed-off-by: Alex Shi <alex.shi@linux.alibaba.com> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: linux-mm@kvack.org Cc: linux-kernel@vger.kernel.org --- mm/filemap.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mm/filemap.c b/mm/filemap.c index d90614f501da..249cf489f5df 100644 --- a/mm/filemap.c +++ b/mm/filemap.c @@ -827,7 +827,7 @@ int replace_page_cache_page(struct page *old, struct page *new, gfp_t gfp_mask) } EXPORT_SYMBOL_GPL(replace_page_cache_page); -noinline int __add_to_page_cache_locked(struct page *page, +static noinline int __add_to_page_cache_locked(struct page *page, struct address_space *mapping, pgoff_t offset, gfp_t gfp, void **shadowp) -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH] mm/filemap: add static for function __add_to_page_cache_locked 2020-11-06 11:24 [PATCH] mm/filemap: add static for function __add_to_page_cache_locked Alex Shi @ 2020-11-06 18:24 ` Ira Weiny 2020-11-10 3:09 ` Souptick Joarder 1 sibling, 0 replies; 22+ messages in thread From: Ira Weiny @ 2020-11-06 18:24 UTC (permalink / raw) To: Alex Shi; +Cc: akpm, linux-mm, linux-kernel On Fri, Nov 06, 2020 at 07:24:55PM +0800, Alex Shi wrote: > Otherwise it cause gcc warning: > ^~~~~~~~~~~~~~~ > ../mm/filemap.c:830:14: warning: no previous prototype for > ‘__add_to_page_cache_locked’ [-Wmissing-prototypes] > noinline int __add_to_page_cache_locked(struct page *page, > ^~~~~~~~~~~~~~~~~~~~~~~~~~ > Does this affect the use in: kernel/bpf/verifier.c|11478| BTF_ID(func, __add_to_page_cache_locked) ? It does not look like that calls the function but I'm not sure what BTF_ID does? Ira > > Signed-off-by: Alex Shi <alex.shi@linux.alibaba.com> > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: linux-mm@kvack.org > Cc: linux-kernel@vger.kernel.org > --- > mm/filemap.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/mm/filemap.c b/mm/filemap.c > index d90614f501da..249cf489f5df 100644 > --- a/mm/filemap.c > +++ b/mm/filemap.c > @@ -827,7 +827,7 @@ int replace_page_cache_page(struct page *old, struct page *new, gfp_t gfp_mask) > } > EXPORT_SYMBOL_GPL(replace_page_cache_page); > > -noinline int __add_to_page_cache_locked(struct page *page, > +static noinline int __add_to_page_cache_locked(struct page *page, > struct address_space *mapping, > pgoff_t offset, gfp_t gfp, > void **shadowp) > -- > 1.8.3.1 > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] mm/filemap: add static for function __add_to_page_cache_locked 2020-11-06 11:24 [PATCH] mm/filemap: add static for function __add_to_page_cache_locked Alex Shi 2020-11-06 18:24 ` Ira Weiny @ 2020-11-10 3:09 ` Souptick Joarder 2020-11-10 11:58 ` Alex Shi 2020-11-10 19:50 ` Andrew Morton 1 sibling, 2 replies; 22+ messages in thread From: Souptick Joarder @ 2020-11-10 3:09 UTC (permalink / raw) To: Alex Shi; +Cc: Andrew Morton, Linux-MM, linux-kernel On Fri, Nov 6, 2020 at 4:55 PM Alex Shi <alex.shi@linux.alibaba.com> wrote: > > Otherwise it cause gcc warning: > ^~~~~~~~~~~~~~~ > ../mm/filemap.c:830:14: warning: no previous prototype for > ‘__add_to_page_cache_locked’ [-Wmissing-prototypes] > noinline int __add_to_page_cache_locked(struct page *page, > ^~~~~~~~~~~~~~~~~~~~~~~~~~ Is CONFIG_DEBUG_INFO_BTF enabled in your .config ? > > Signed-off-by: Alex Shi <alex.shi@linux.alibaba.com> > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: linux-mm@kvack.org > Cc: linux-kernel@vger.kernel.org > --- > mm/filemap.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/mm/filemap.c b/mm/filemap.c > index d90614f501da..249cf489f5df 100644 > --- a/mm/filemap.c > +++ b/mm/filemap.c > @@ -827,7 +827,7 @@ int replace_page_cache_page(struct page *old, struct page *new, gfp_t gfp_mask) > } > EXPORT_SYMBOL_GPL(replace_page_cache_page); > > -noinline int __add_to_page_cache_locked(struct page *page, > +static noinline int __add_to_page_cache_locked(struct page *page, > struct address_space *mapping, > pgoff_t offset, gfp_t gfp, > void **shadowp) > -- > 1.8.3.1 > > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] mm/filemap: add static for function __add_to_page_cache_locked 2020-11-10 3:09 ` Souptick Joarder @ 2020-11-10 11:58 ` Alex Shi 2020-11-10 19:50 ` Andrew Morton 1 sibling, 0 replies; 22+ messages in thread From: Alex Shi @ 2020-11-10 11:58 UTC (permalink / raw) To: Souptick Joarder; +Cc: Andrew Morton, Linux-MM, linux-kernel 在 2020/11/10 上午11:09, Souptick Joarder 写道: > On Fri, Nov 6, 2020 at 4:55 PM Alex Shi <alex.shi@linux.alibaba.com> wrote: >> >> Otherwise it cause gcc warning: >> ^~~~~~~~~~~~~~~ >> ../mm/filemap.c:830:14: warning: no previous prototype for >> ‘__add_to_page_cache_locked’ [-Wmissing-prototypes] >> noinline int __add_to_page_cache_locked(struct page *page, >> ^~~~~~~~~~~~~~~~~~~~~~~~~~ > > Is CONFIG_DEBUG_INFO_BTF enabled in your .config ? Sorry, I tried to buld the configure with bzImage, but failed on pahole version too low, and compiled pahole still can not run in git://git.kernel.org/pub/scm/devel/pahole/pahole.git #pahole pahole: symbol lookup error: pahole: undefined symbol: tabs > >> >> Signed-off-by: Alex Shi <alex.shi@linux.alibaba.com> >> Cc: Andrew Morton <akpm@linux-foundation.org> >> Cc: linux-mm@kvack.org >> Cc: linux-kernel@vger.kernel.org >> --- >> mm/filemap.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/mm/filemap.c b/mm/filemap.c >> index d90614f501da..249cf489f5df 100644 >> --- a/mm/filemap.c >> +++ b/mm/filemap.c >> @@ -827,7 +827,7 @@ int replace_page_cache_page(struct page *old, struct page *new, gfp_t gfp_mask) >> } >> EXPORT_SYMBOL_GPL(replace_page_cache_page); >> >> -noinline int __add_to_page_cache_locked(struct page *page, >> +static noinline int __add_to_page_cache_locked(struct page *page, >> struct address_space *mapping, >> pgoff_t offset, gfp_t gfp, >> void **shadowp) >> -- >> 1.8.3.1 >> >> ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] mm/filemap: add static for function __add_to_page_cache_locked 2020-11-10 3:09 ` Souptick Joarder 2020-11-10 11:58 ` Alex Shi @ 2020-11-10 19:50 ` Andrew Morton 2020-11-12 0:18 ` Alex Shi 1 sibling, 1 reply; 22+ messages in thread From: Andrew Morton @ 2020-11-10 19:50 UTC (permalink / raw) To: Souptick Joarder Cc: Alex Shi, Linux-MM, linux-kernel, Alexei Starovoitov, Daniel Borkmann, Josef Bacik On Tue, 10 Nov 2020 08:39:24 +0530 Souptick Joarder <jrdr.linux@gmail.com> wrote: > On Fri, Nov 6, 2020 at 4:55 PM Alex Shi <alex.shi@linux.alibaba.com> wrote: > > > > Otherwise it cause gcc warning: > > ^~~~~~~~~~~~~~~ > > ../mm/filemap.c:830:14: warning: no previous prototype for > > ‘__add_to_page_cache_locked’ [-Wmissing-prototypes] > > noinline int __add_to_page_cache_locked(struct page *page, > > ^~~~~~~~~~~~~~~~~~~~~~~~~~ > > Is CONFIG_DEBUG_INFO_BTF enabled in your .config ? hm, yes. > > > > Signed-off-by: Alex Shi <alex.shi@linux.alibaba.com> > > Cc: Andrew Morton <akpm@linux-foundation.org> > > Cc: linux-mm@kvack.org > > Cc: linux-kernel@vger.kernel.org > > --- > > mm/filemap.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/mm/filemap.c b/mm/filemap.c > > index d90614f501da..249cf489f5df 100644 > > --- a/mm/filemap.c > > +++ b/mm/filemap.c > > @@ -827,7 +827,7 @@ int replace_page_cache_page(struct page *old, struct page *new, gfp_t gfp_mask) > > } > > EXPORT_SYMBOL_GPL(replace_page_cache_page); > > > > -noinline int __add_to_page_cache_locked(struct page *page, > > +static noinline int __add_to_page_cache_locked(struct page *page, > > struct address_space *mapping, > > pgoff_t offset, gfp_t gfp, > > void **shadowp) It's unclear to me whether BTF_ID() requires that the target symbol be non-static. It doesn't actually reference the symbol: #define BTF_ID(prefix, name) \ __BTF_ID(__ID(__BTF_ID__##prefix##__##name##__)) Alexei, can you please comment? ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] mm/filemap: add static for function __add_to_page_cache_locked 2020-11-10 19:50 ` Andrew Morton @ 2020-11-12 0:18 ` Alex Shi 2020-12-07 8:11 ` Greg Thelen 2020-12-07 8:15 ` Michal Kubecek 0 siblings, 2 replies; 22+ messages in thread From: Alex Shi @ 2020-11-12 0:18 UTC (permalink / raw) To: Andrew Morton, Souptick Joarder Cc: Linux-MM, linux-kernel, Alexei Starovoitov, Daniel Borkmann, Josef Bacik 在 2020/11/11 上午3:50, Andrew Morton 写道: > On Tue, 10 Nov 2020 08:39:24 +0530 Souptick Joarder <jrdr.linux@gmail.com> wrote: > >> On Fri, Nov 6, 2020 at 4:55 PM Alex Shi <alex.shi@linux.alibaba.com> wrote: >>> >>> Otherwise it cause gcc warning: >>> ^~~~~~~~~~~~~~~ >>> ../mm/filemap.c:830:14: warning: no previous prototype for >>> ‘__add_to_page_cache_locked’ [-Wmissing-prototypes] >>> noinline int __add_to_page_cache_locked(struct page *page, >>> ^~~~~~~~~~~~~~~~~~~~~~~~~~ >> >> Is CONFIG_DEBUG_INFO_BTF enabled in your .config ? > > hm, yes. When the config enabled, compiling looks good untill pahole tool used to get BTF info, but I still failed on a right version pahole > 1.16. Sorry. > >>> >>> Signed-off-by: Alex Shi <alex.shi@linux.alibaba.com> >>> Cc: Andrew Morton <akpm@linux-foundation.org> >>> Cc: linux-mm@kvack.org >>> Cc: linux-kernel@vger.kernel.org >>> --- >>> mm/filemap.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/mm/filemap.c b/mm/filemap.c >>> index d90614f501da..249cf489f5df 100644 >>> --- a/mm/filemap.c >>> +++ b/mm/filemap.c >>> @@ -827,7 +827,7 @@ int replace_page_cache_page(struct page *old, struct page *new, gfp_t gfp_mask) >>> } >>> EXPORT_SYMBOL_GPL(replace_page_cache_page); >>> >>> -noinline int __add_to_page_cache_locked(struct page *page, >>> +static noinline int __add_to_page_cache_locked(struct page *page, >>> struct address_space *mapping, >>> pgoff_t offset, gfp_t gfp, >>> void **shadowp) > > It's unclear to me whether BTF_ID() requires that the target symbol be > non-static. It doesn't actually reference the symbol: > > #define BTF_ID(prefix, name) \ > __BTF_ID(__ID(__BTF_ID__##prefix##__##name##__)) > The above usage make me thought BTF don't require the symbol, though the symbol still exist in vmlinux with 'static'. So any comments of this, Alexei? > Alexei, can you please comment? > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] mm/filemap: add static for function __add_to_page_cache_locked 2020-11-12 0:18 ` Alex Shi @ 2020-12-07 8:11 ` Greg Thelen 2020-12-07 8:15 ` Michal Kubecek 1 sibling, 0 replies; 22+ messages in thread From: Greg Thelen @ 2020-12-07 8:11 UTC (permalink / raw) To: Alex Shi, Andrew Morton, Souptick Joarder Cc: Linux-MM, linux-kernel, Alexei Starovoitov, Daniel Borkmann, Josef Bacik Alex Shi <alex.shi@linux.alibaba.com> wrote: > 在 2020/11/11 上午3:50, Andrew Morton 写道: >> On Tue, 10 Nov 2020 08:39:24 +0530 Souptick Joarder <jrdr.linux@gmail.com> wrote: >> >>> On Fri, Nov 6, 2020 at 4:55 PM Alex Shi <alex.shi@linux.alibaba.com> wrote: >>>> >>>> Otherwise it cause gcc warning: >>>> ^~~~~~~~~~~~~~~ >>>> ../mm/filemap.c:830:14: warning: no previous prototype for >>>> ‘__add_to_page_cache_locked’ [-Wmissing-prototypes] >>>> noinline int __add_to_page_cache_locked(struct page *page, >>>> ^~~~~~~~~~~~~~~~~~~~~~~~~~ >>> >>> Is CONFIG_DEBUG_INFO_BTF enabled in your .config ? >> >> hm, yes. > > When the config enabled, compiling looks good untill pahole tool > used to get BTF info, but I still failed on a right version pahole >> 1.16. Sorry. I'm seeing an issue with this patch. My build system has pahole v1.17, but I don't think the pahole version is key. $ git checkout 3351b16af494 # recently added to linus/master $ make defconfig $ make menuconfig # set CONFIG_BPF_SYSCALL and CONFIG_DEBUG_INFO_BTF $ make V=1 + ./tools/bpf/resolve_btfids/resolve_btfids vmlinux FAILED unresolved symbol __add_to_page_cache_locked Reverting 3351b16af494 ("mm/filemap: add static for function __add_to_page_cache_locked") fixes the issue. I don't see the warning which motivated this patch, but maybe it requires particular a .config or gcc version. Perhaps adding a __add_to_page_cache_locked() prototype would meet avoid it. But I haven't studied enough on BTF to know if there's a better answer. >>>> Signed-off-by: Alex Shi <alex.shi@linux.alibaba.com> >>>> Cc: Andrew Morton <akpm@linux-foundation.org> >>>> Cc: linux-mm@kvack.org >>>> Cc: linux-kernel@vger.kernel.org >>>> --- >>>> mm/filemap.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/mm/filemap.c b/mm/filemap.c >>>> index d90614f501da..249cf489f5df 100644 >>>> --- a/mm/filemap.c >>>> +++ b/mm/filemap.c >>>> @@ -827,7 +827,7 @@ int replace_page_cache_page(struct page *old, struct page *new, gfp_t gfp_mask) >>>> } >>>> EXPORT_SYMBOL_GPL(replace_page_cache_page); >>>> >>>> -noinline int __add_to_page_cache_locked(struct page *page, >>>> +static noinline int __add_to_page_cache_locked(struct page *page, >>>> struct address_space *mapping, >>>> pgoff_t offset, gfp_t gfp, >>>> void **shadowp) >> >> It's unclear to me whether BTF_ID() requires that the target symbol be >> non-static. It doesn't actually reference the symbol: >> >> #define BTF_ID(prefix, name) \ >> __BTF_ID(__ID(__BTF_ID__##prefix##__##name##__)) >> > > The above usage make me thought BTF don't require the symbol, though > the symbol still exist in vmlinux with 'static'. > > So any comments of this, Alexei? > >> Alexei, can you please comment? >> ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] mm/filemap: add static for function __add_to_page_cache_locked 2020-11-12 0:18 ` Alex Shi 2020-12-07 8:11 ` Greg Thelen @ 2020-12-07 8:15 ` Michal Kubecek 2020-12-07 18:35 ` Justin Forbes 1 sibling, 1 reply; 22+ messages in thread From: Michal Kubecek @ 2020-12-07 8:15 UTC (permalink / raw) To: Alex Shi Cc: Andrew Morton, Souptick Joarder, Linux-MM, linux-kernel, Alexei Starovoitov, Daniel Borkmann, Josef Bacik On Thu, Nov 12, 2020 at 08:18:57AM +0800, Alex Shi wrote: > > > 在 2020/11/11 上午3:50, Andrew Morton 写道: > > On Tue, 10 Nov 2020 08:39:24 +0530 Souptick Joarder <jrdr.linux@gmail.com> wrote: > > > >> On Fri, Nov 6, 2020 at 4:55 PM Alex Shi <alex.shi@linux.alibaba.com> wrote: > >>> > >>> Otherwise it cause gcc warning: > >>> ^~~~~~~~~~~~~~~ > >>> ../mm/filemap.c:830:14: warning: no previous prototype for > >>> ‘__add_to_page_cache_locked’ [-Wmissing-prototypes] > >>> noinline int __add_to_page_cache_locked(struct page *page, > >>> ^~~~~~~~~~~~~~~~~~~~~~~~~~ > >> > >> Is CONFIG_DEBUG_INFO_BTF enabled in your .config ? > > > > hm, yes. > > When the config enabled, compiling looks good untill pahole tool > used to get BTF info, but I still failed on a right version pahole > > 1.16. Sorry. > > > > >>> > >>> Signed-off-by: Alex Shi <alex.shi@linux.alibaba.com> > >>> Cc: Andrew Morton <akpm@linux-foundation.org> > >>> Cc: linux-mm@kvack.org > >>> Cc: linux-kernel@vger.kernel.org > >>> --- > >>> mm/filemap.c | 2 +- > >>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>> > >>> diff --git a/mm/filemap.c b/mm/filemap.c > >>> index d90614f501da..249cf489f5df 100644 > >>> --- a/mm/filemap.c > >>> +++ b/mm/filemap.c > >>> @@ -827,7 +827,7 @@ int replace_page_cache_page(struct page *old, struct page *new, gfp_t gfp_mask) > >>> } > >>> EXPORT_SYMBOL_GPL(replace_page_cache_page); > >>> > >>> -noinline int __add_to_page_cache_locked(struct page *page, > >>> +static noinline int __add_to_page_cache_locked(struct page *page, > >>> struct address_space *mapping, > >>> pgoff_t offset, gfp_t gfp, > >>> void **shadowp) > > > > It's unclear to me whether BTF_ID() requires that the target symbol be > > non-static. It doesn't actually reference the symbol: > > > > #define BTF_ID(prefix, name) \ > > __BTF_ID(__ID(__BTF_ID__##prefix##__##name##__)) > > > > The above usage make me thought BTF don't require the symbol, though > the symbol still exist in vmlinux with 'static'. > > So any comments of this, Alexei? It's probably more complicated: our v5.10-rc7 builds with CONFIG_DEBUG_INFO_BTF=y fail on ppc64 and ppc64le with BTFIDS vmlinux FAILED unresolved symbol __add_to_page_cache_locked but succeed on x86_64, i586, aarch64 and s390x. So far I don't see why this should depend on architecture. Michal ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] mm/filemap: add static for function __add_to_page_cache_locked 2020-12-07 8:15 ` Michal Kubecek @ 2020-12-07 18:35 ` Justin Forbes 2020-12-07 22:44 ` Alexei Starovoitov 0 siblings, 1 reply; 22+ messages in thread From: Justin Forbes @ 2020-12-07 18:35 UTC (permalink / raw) To: Michal Kubecek Cc: Alex Shi, Andrew Morton, Souptick Joarder, Linux-MM, LKML, Alexei Starovoitov, Daniel Borkmann, Josef Bacik On Mon, Dec 7, 2020 at 2:16 AM Michal Kubecek <mkubecek@suse.cz> wrote: > > On Thu, Nov 12, 2020 at 08:18:57AM +0800, Alex Shi wrote: > > > > > > 在 2020/11/11 上午3:50, Andrew Morton 写道: > > > On Tue, 10 Nov 2020 08:39:24 +0530 Souptick Joarder <jrdr.linux@gmail.com> wrote: > > > > > >> On Fri, Nov 6, 2020 at 4:55 PM Alex Shi <alex.shi@linux.alibaba.com> wrote: > > >>> > > >>> Otherwise it cause gcc warning: > > >>> ^~~~~~~~~~~~~~~ > > >>> ../mm/filemap.c:830:14: warning: no previous prototype for > > >>> ‘__add_to_page_cache_locked’ [-Wmissing-prototypes] > > >>> noinline int __add_to_page_cache_locked(struct page *page, > > >>> ^~~~~~~~~~~~~~~~~~~~~~~~~~ > > >> > > >> Is CONFIG_DEBUG_INFO_BTF enabled in your .config ? > > > > > > hm, yes. > > > > When the config enabled, compiling looks good untill pahole tool > > used to get BTF info, but I still failed on a right version pahole > > > 1.16. Sorry. > > > > > > > >>> > > >>> Signed-off-by: Alex Shi <alex.shi@linux.alibaba.com> > > >>> Cc: Andrew Morton <akpm@linux-foundation.org> > > >>> Cc: linux-mm@kvack.org > > >>> Cc: linux-kernel@vger.kernel.org > > >>> --- > > >>> mm/filemap.c | 2 +- > > >>> 1 file changed, 1 insertion(+), 1 deletion(-) > > >>> > > >>> diff --git a/mm/filemap.c b/mm/filemap.c > > >>> index d90614f501da..249cf489f5df 100644 > > >>> --- a/mm/filemap.c > > >>> +++ b/mm/filemap.c > > >>> @@ -827,7 +827,7 @@ int replace_page_cache_page(struct page *old, struct page *new, gfp_t gfp_mask) > > >>> } > > >>> EXPORT_SYMBOL_GPL(replace_page_cache_page); > > >>> > > >>> -noinline int __add_to_page_cache_locked(struct page *page, > > >>> +static noinline int __add_to_page_cache_locked(struct page *page, > > >>> struct address_space *mapping, > > >>> pgoff_t offset, gfp_t gfp, > > >>> void **shadowp) > > > > > > It's unclear to me whether BTF_ID() requires that the target symbol be > > > non-static. It doesn't actually reference the symbol: > > > > > > #define BTF_ID(prefix, name) \ > > > __BTF_ID(__ID(__BTF_ID__##prefix##__##name##__)) > > > > > > > The above usage make me thought BTF don't require the symbol, though > > the symbol still exist in vmlinux with 'static'. > > > > So any comments of this, Alexei? > > It's probably more complicated: our v5.10-rc7 builds with > CONFIG_DEBUG_INFO_BTF=y fail on ppc64 and ppc64le with > > BTFIDS vmlinux > FAILED unresolved symbol __add_to_page_cache_locked > > > but succeed on x86_64, i586, aarch64 and s390x. So far I don't see why > this should depend on architecture. > Fedora is failing with rc7 on the same issue on PPC only. Justin ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] mm/filemap: add static for function __add_to_page_cache_locked 2020-12-07 18:35 ` Justin Forbes @ 2020-12-07 22:44 ` Alexei Starovoitov 2020-12-07 22:53 ` Michal Kubecek 0 siblings, 1 reply; 22+ messages in thread From: Alexei Starovoitov @ 2020-12-07 22:44 UTC (permalink / raw) To: Justin Forbes, bpf Cc: Michal Kubecek, Alex Shi, Andrew Morton, Souptick Joarder, Linux-MM, LKML, Alexei Starovoitov, Daniel Borkmann, Josef Bacik On Mon, Dec 7, 2020 at 10:36 AM Justin Forbes <jmforbes@linuxtx.org> wrote: > > On Mon, Dec 7, 2020 at 2:16 AM Michal Kubecek <mkubecek@suse.cz> wrote: > > > > On Thu, Nov 12, 2020 at 08:18:57AM +0800, Alex Shi wrote: > > > > > > > > > 在 2020/11/11 上午3:50, Andrew Morton 写道: > > > > On Tue, 10 Nov 2020 08:39:24 +0530 Souptick Joarder <jrdr.linux@gmail.com> wrote: > > > > > > > >> On Fri, Nov 6, 2020 at 4:55 PM Alex Shi <alex.shi@linux.alibaba.com> wrote: > > > >>> > > > >>> Otherwise it cause gcc warning: > > > >>> ^~~~~~~~~~~~~~~ > > > >>> ../mm/filemap.c:830:14: warning: no previous prototype for > > > >>> ‘__add_to_page_cache_locked’ [-Wmissing-prototypes] > > > >>> noinline int __add_to_page_cache_locked(struct page *page, > > > >>> ^~~~~~~~~~~~~~~~~~~~~~~~~~ > > > >> > > > >> Is CONFIG_DEBUG_INFO_BTF enabled in your .config ? > > > > > > > > hm, yes. > > > > > > When the config enabled, compiling looks good untill pahole tool > > > used to get BTF info, but I still failed on a right version pahole > > > > 1.16. Sorry. > > > > > > > > > > >>> > > > >>> Signed-off-by: Alex Shi <alex.shi@linux.alibaba.com> > > > >>> Cc: Andrew Morton <akpm@linux-foundation.org> > > > >>> Cc: linux-mm@kvack.org > > > >>> Cc: linux-kernel@vger.kernel.org > > > >>> --- > > > >>> mm/filemap.c | 2 +- > > > >>> 1 file changed, 1 insertion(+), 1 deletion(-) > > > >>> > > > >>> diff --git a/mm/filemap.c b/mm/filemap.c > > > >>> index d90614f501da..249cf489f5df 100644 > > > >>> --- a/mm/filemap.c > > > >>> +++ b/mm/filemap.c > > > >>> @@ -827,7 +827,7 @@ int replace_page_cache_page(struct page *old, struct page *new, gfp_t gfp_mask) > > > >>> } > > > >>> EXPORT_SYMBOL_GPL(replace_page_cache_page); > > > >>> > > > >>> -noinline int __add_to_page_cache_locked(struct page *page, > > > >>> +static noinline int __add_to_page_cache_locked(struct page *page, > > > >>> struct address_space *mapping, > > > >>> pgoff_t offset, gfp_t gfp, > > > >>> void **shadowp) > > > > > > > > It's unclear to me whether BTF_ID() requires that the target symbol be > > > > non-static. It doesn't actually reference the symbol: > > > > > > > > #define BTF_ID(prefix, name) \ > > > > __BTF_ID(__ID(__BTF_ID__##prefix##__##name##__)) > > > > > > > > > > The above usage make me thought BTF don't require the symbol, though > > > the symbol still exist in vmlinux with 'static'. > > > > > > So any comments of this, Alexei? Sorry. I've completely missed this thread. Now I have a hard time finding context in archives. If I understood what's going on the removal of "static" cases issues? Yes. That's expected. noinline alone is not enough to work reliably. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] mm/filemap: add static for function __add_to_page_cache_locked 2020-12-07 22:44 ` Alexei Starovoitov @ 2020-12-07 22:53 ` Michal Kubecek 2020-12-07 22:59 ` Alexei Starovoitov 2020-12-09 20:59 ` Tony Luck 0 siblings, 2 replies; 22+ messages in thread From: Michal Kubecek @ 2020-12-07 22:53 UTC (permalink / raw) To: Alexei Starovoitov Cc: Justin Forbes, bpf, Alex Shi, Andrew Morton, Souptick Joarder, Linux-MM, LKML, Alexei Starovoitov, Daniel Borkmann, Josef Bacik On Mon, Dec 07, 2020 at 02:44:22PM -0800, Alexei Starovoitov wrote: > On Mon, Dec 7, 2020 at 10:36 AM Justin Forbes <jmforbes@linuxtx.org> wrote: > > > > On Mon, Dec 7, 2020 at 2:16 AM Michal Kubecek <mkubecek@suse.cz> wrote: > > > > > > On Thu, Nov 12, 2020 at 08:18:57AM +0800, Alex Shi wrote: > > > > > > > > > > > > 在 2020/11/11 上午3:50, Andrew Morton 写道: > > > > > On Tue, 10 Nov 2020 08:39:24 +0530 Souptick Joarder <jrdr.linux@gmail.com> wrote: > > > > > > > > > >> On Fri, Nov 6, 2020 at 4:55 PM Alex Shi <alex.shi@linux.alibaba.com> wrote: > > > > >>> > > > > >>> Otherwise it cause gcc warning: > > > > >>> ^~~~~~~~~~~~~~~ > > > > >>> ../mm/filemap.c:830:14: warning: no previous prototype for > > > > >>> ‘__add_to_page_cache_locked’ [-Wmissing-prototypes] > > > > >>> noinline int __add_to_page_cache_locked(struct page *page, > > > > >>> ^~~~~~~~~~~~~~~~~~~~~~~~~~ > > > > >> > > > > >> Is CONFIG_DEBUG_INFO_BTF enabled in your .config ? > > > > > > > > > > hm, yes. > > > > > > > > When the config enabled, compiling looks good untill pahole tool > > > > used to get BTF info, but I still failed on a right version pahole > > > > > 1.16. Sorry. > > > > > > > > > > > > > >>> > > > > >>> Signed-off-by: Alex Shi <alex.shi@linux.alibaba.com> > > > > >>> Cc: Andrew Morton <akpm@linux-foundation.org> > > > > >>> Cc: linux-mm@kvack.org > > > > >>> Cc: linux-kernel@vger.kernel.org > > > > >>> --- > > > > >>> mm/filemap.c | 2 +- > > > > >>> 1 file changed, 1 insertion(+), 1 deletion(-) > > > > >>> > > > > >>> diff --git a/mm/filemap.c b/mm/filemap.c > > > > >>> index d90614f501da..249cf489f5df 100644 > > > > >>> --- a/mm/filemap.c > > > > >>> +++ b/mm/filemap.c > > > > >>> @@ -827,7 +827,7 @@ int replace_page_cache_page(struct page *old, struct page *new, gfp_t gfp_mask) > > > > >>> } > > > > >>> EXPORT_SYMBOL_GPL(replace_page_cache_page); > > > > >>> > > > > >>> -noinline int __add_to_page_cache_locked(struct page *page, > > > > >>> +static noinline int __add_to_page_cache_locked(struct page *page, > > > > >>> struct address_space *mapping, > > > > >>> pgoff_t offset, gfp_t gfp, > > > > >>> void **shadowp) > > > > > > > > > > It's unclear to me whether BTF_ID() requires that the target symbol be > > > > > non-static. It doesn't actually reference the symbol: > > > > > > > > > > #define BTF_ID(prefix, name) \ > > > > > __BTF_ID(__ID(__BTF_ID__##prefix##__##name##__)) > > > > > > > > > > > > > The above usage make me thought BTF don't require the symbol, though > > > > the symbol still exist in vmlinux with 'static'. > > > > > > > > So any comments of this, Alexei? > > Sorry. I've completely missed this thread. > Now I have a hard time finding context in archives. > If I understood what's going on the removal of "static" cases issues? > Yes. That's expected. > noinline alone is not enough to work reliably. Not removal, commit 3351b16af494 ("mm/filemap: add static for function __add_to_page_cache_locked") made the function static which breaks the build in btfids phase - but it seems to happen only on some architectures. In our case, ppc64, ppc64le and riscv64 are broken, x86_64, i586 and s390x succeed. (I made a mistake above, aarch64 did not fail - but only because it was not built at all.) The thread starts with http://lkml.kernel.org/r/1604661895-5495-1-git-send-email-alex.shi@linux.alibaba.com Michal ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] mm/filemap: add static for function __add_to_page_cache_locked 2020-12-07 22:53 ` Michal Kubecek @ 2020-12-07 22:59 ` Alexei Starovoitov 2020-12-08 1:12 ` Alexei Starovoitov 2020-12-09 20:59 ` Tony Luck 1 sibling, 1 reply; 22+ messages in thread From: Alexei Starovoitov @ 2020-12-07 22:59 UTC (permalink / raw) To: Michal Kubecek Cc: Justin Forbes, bpf, Alex Shi, Andrew Morton, Souptick Joarder, Linux-MM, LKML, Alexei Starovoitov, Daniel Borkmann, Josef Bacik On Mon, Dec 7, 2020 at 2:53 PM Michal Kubecek <mkubecek@suse.cz> wrote: > > On Mon, Dec 07, 2020 at 02:44:22PM -0800, Alexei Starovoitov wrote: > > On Mon, Dec 7, 2020 at 10:36 AM Justin Forbes <jmforbes@linuxtx.org> wrote: > > > > > > On Mon, Dec 7, 2020 at 2:16 AM Michal Kubecek <mkubecek@suse.cz> wrote: > > > > > > > > On Thu, Nov 12, 2020 at 08:18:57AM +0800, Alex Shi wrote: > > > > > > > > > > > > > > > 在 2020/11/11 上午3:50, Andrew Morton 写道: > > > > > > On Tue, 10 Nov 2020 08:39:24 +0530 Souptick Joarder <jrdr.linux@gmail.com> wrote: > > > > > > > > > > > >> On Fri, Nov 6, 2020 at 4:55 PM Alex Shi <alex.shi@linux.alibaba.com> wrote: > > > > > >>> > > > > > >>> Otherwise it cause gcc warning: > > > > > >>> ^~~~~~~~~~~~~~~ > > > > > >>> ../mm/filemap.c:830:14: warning: no previous prototype for > > > > > >>> ‘__add_to_page_cache_locked’ [-Wmissing-prototypes] > > > > > >>> noinline int __add_to_page_cache_locked(struct page *page, > > > > > >>> ^~~~~~~~~~~~~~~~~~~~~~~~~~ > > > > > >> > > > > > >> Is CONFIG_DEBUG_INFO_BTF enabled in your .config ? > > > > > > > > > > > > hm, yes. > > > > > > > > > > When the config enabled, compiling looks good untill pahole tool > > > > > used to get BTF info, but I still failed on a right version pahole > > > > > > 1.16. Sorry. > > > > > > > > > > > > > > > > >>> > > > > > >>> Signed-off-by: Alex Shi <alex.shi@linux.alibaba.com> > > > > > >>> Cc: Andrew Morton <akpm@linux-foundation.org> > > > > > >>> Cc: linux-mm@kvack.org > > > > > >>> Cc: linux-kernel@vger.kernel.org > > > > > >>> --- > > > > > >>> mm/filemap.c | 2 +- > > > > > >>> 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > >>> > > > > > >>> diff --git a/mm/filemap.c b/mm/filemap.c > > > > > >>> index d90614f501da..249cf489f5df 100644 > > > > > >>> --- a/mm/filemap.c > > > > > >>> +++ b/mm/filemap.c > > > > > >>> @@ -827,7 +827,7 @@ int replace_page_cache_page(struct page *old, struct page *new, gfp_t gfp_mask) > > > > > >>> } > > > > > >>> EXPORT_SYMBOL_GPL(replace_page_cache_page); > > > > > >>> > > > > > >>> -noinline int __add_to_page_cache_locked(struct page *page, > > > > > >>> +static noinline int __add_to_page_cache_locked(struct page *page, > > > > > >>> struct address_space *mapping, > > > > > >>> pgoff_t offset, gfp_t gfp, > > > > > >>> void **shadowp) > > > > > > > > > > > > It's unclear to me whether BTF_ID() requires that the target symbol be > > > > > > non-static. It doesn't actually reference the symbol: > > > > > > > > > > > > #define BTF_ID(prefix, name) \ > > > > > > __BTF_ID(__ID(__BTF_ID__##prefix##__##name##__)) > > > > > > > > > > > > > > > > The above usage make me thought BTF don't require the symbol, though > > > > > the symbol still exist in vmlinux with 'static'. > > > > > > > > > > So any comments of this, Alexei? > > > > Sorry. I've completely missed this thread. > > Now I have a hard time finding context in archives. > > If I understood what's going on the removal of "static" cases issues? > > Yes. That's expected. > > noinline alone is not enough to work reliably. > > Not removal, commit 3351b16af494 ("mm/filemap: add static for function > __add_to_page_cache_locked") made the function static which breaks the > build in btfids phase - but it seems to happen only on some > architectures. In our case, ppc64, ppc64le and riscv64 are broken, > x86_64, i586 and s390x succeed. (I made a mistake above, aarch64 did not > fail - but only because it was not built at all.) > > The thread starts with > http://lkml.kernel.org/r/1604661895-5495-1-git-send-email-alex.shi@linux.alibaba.com Got it. So the above commit is wrong. The addition of "static" is incorrect here. Regardless of btf_id generation. "static noinline" means that the error injection in that spot is unreliable. Even when bpf is completely compiled out of the kernel. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] mm/filemap: add static for function __add_to_page_cache_locked 2020-12-07 22:59 ` Alexei Starovoitov @ 2020-12-08 1:12 ` Alexei Starovoitov 2020-12-09 14:46 ` Stanislaw Gruszka 0 siblings, 1 reply; 22+ messages in thread From: Alexei Starovoitov @ 2020-12-08 1:12 UTC (permalink / raw) To: Michal Kubecek Cc: Justin Forbes, bpf, Alex Shi, Andrew Morton, Souptick Joarder, Linux-MM, LKML, Alexei Starovoitov, Daniel Borkmann, Josef Bacik On Mon, Dec 7, 2020 at 2:59 PM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Mon, Dec 7, 2020 at 2:53 PM Michal Kubecek <mkubecek@suse.cz> wrote: > > > > On Mon, Dec 07, 2020 at 02:44:22PM -0800, Alexei Starovoitov wrote: > > > On Mon, Dec 7, 2020 at 10:36 AM Justin Forbes <jmforbes@linuxtx.org> wrote: > > > > > > > > On Mon, Dec 7, 2020 at 2:16 AM Michal Kubecek <mkubecek@suse.cz> wrote: > > > > > > > > > > On Thu, Nov 12, 2020 at 08:18:57AM +0800, Alex Shi wrote: > > > > > > > > > > > > > > > > > > 在 2020/11/11 上午3:50, Andrew Morton 写道: > > > > > > > On Tue, 10 Nov 2020 08:39:24 +0530 Souptick Joarder <jrdr.linux@gmail.com> wrote: > > > > > > > > > > > > > >> On Fri, Nov 6, 2020 at 4:55 PM Alex Shi <alex.shi@linux.alibaba.com> wrote: > > > > > > >>> > > > > > > >>> Otherwise it cause gcc warning: > > > > > > >>> ^~~~~~~~~~~~~~~ > > > > > > >>> ../mm/filemap.c:830:14: warning: no previous prototype for > > > > > > >>> ‘__add_to_page_cache_locked’ [-Wmissing-prototypes] > > > > > > >>> noinline int __add_to_page_cache_locked(struct page *page, > > > > > > >>> ^~~~~~~~~~~~~~~~~~~~~~~~~~ > > > > > > >> > > > > > > >> Is CONFIG_DEBUG_INFO_BTF enabled in your .config ? > > > > > > > > > > > > > > hm, yes. > > > > > > > > > > > > When the config enabled, compiling looks good untill pahole tool > > > > > > used to get BTF info, but I still failed on a right version pahole > > > > > > > 1.16. Sorry. > > > > > > > > > > > > > > > > > > > >>> > > > > > > >>> Signed-off-by: Alex Shi <alex.shi@linux.alibaba.com> > > > > > > >>> Cc: Andrew Morton <akpm@linux-foundation.org> > > > > > > >>> Cc: linux-mm@kvack.org > > > > > > >>> Cc: linux-kernel@vger.kernel.org > > > > > > >>> --- > > > > > > >>> mm/filemap.c | 2 +- > > > > > > >>> 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > >>> > > > > > > >>> diff --git a/mm/filemap.c b/mm/filemap.c > > > > > > >>> index d90614f501da..249cf489f5df 100644 > > > > > > >>> --- a/mm/filemap.c > > > > > > >>> +++ b/mm/filemap.c > > > > > > >>> @@ -827,7 +827,7 @@ int replace_page_cache_page(struct page *old, struct page *new, gfp_t gfp_mask) > > > > > > >>> } > > > > > > >>> EXPORT_SYMBOL_GPL(replace_page_cache_page); > > > > > > >>> > > > > > > >>> -noinline int __add_to_page_cache_locked(struct page *page, > > > > > > >>> +static noinline int __add_to_page_cache_locked(struct page *page, > > > > > > >>> struct address_space *mapping, > > > > > > >>> pgoff_t offset, gfp_t gfp, > > > > > > >>> void **shadowp) > > > > > > > > > > > > > > It's unclear to me whether BTF_ID() requires that the target symbol be > > > > > > > non-static. It doesn't actually reference the symbol: > > > > > > > > > > > > > > #define BTF_ID(prefix, name) \ > > > > > > > __BTF_ID(__ID(__BTF_ID__##prefix##__##name##__)) > > > > > > > > > > > > > > > > > > > The above usage make me thought BTF don't require the symbol, though > > > > > > the symbol still exist in vmlinux with 'static'. > > > > > > > > > > > > So any comments of this, Alexei? > > > > > > Sorry. I've completely missed this thread. > > > Now I have a hard time finding context in archives. > > > If I understood what's going on the removal of "static" cases issues? > > > Yes. That's expected. > > > noinline alone is not enough to work reliably. > > > > Not removal, commit 3351b16af494 ("mm/filemap: add static for function > > __add_to_page_cache_locked") made the function static which breaks the > > build in btfids phase - but it seems to happen only on some > > architectures. In our case, ppc64, ppc64le and riscv64 are broken, > > x86_64, i586 and s390x succeed. (I made a mistake above, aarch64 did not > > fail - but only because it was not built at all.) > > > > The thread starts with > > http://lkml.kernel.org/r/1604661895-5495-1-git-send-email-alex.shi@linux.alibaba.com > > Got it. So the above commit is wrong. > The addition of "static" is incorrect here. > Regardless of btf_id generation. > "static noinline" means that the error injection in that spot is unreliable. > Even when bpf is completely compiled out of the kernel. I finally realized that the addition of 'static' was pushed into Linus's tree :( Please revert commit 3351b16af494 ("mm/filemap: add static for function __add_to_page_cache_locked") ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] mm/filemap: add static for function __add_to_page_cache_locked 2020-12-08 1:12 ` Alexei Starovoitov @ 2020-12-09 14:46 ` Stanislaw Gruszka 2020-12-09 15:08 ` Matthew Wilcox 0 siblings, 1 reply; 22+ messages in thread From: Stanislaw Gruszka @ 2020-12-09 14:46 UTC (permalink / raw) To: Alexei Starovoitov Cc: Michal Kubecek, Justin Forbes, bpf, Alex Shi, Andrew Morton, Souptick Joarder, Linux-MM, LKML, Alexei Starovoitov, Daniel Borkmann, Josef Bacik On Mon, Dec 07, 2020 at 05:12:52PM -0800, Alexei Starovoitov wrote: > > > > > > > >>> -noinline int __add_to_page_cache_locked(struct page *page, > > > > > > > >>> +static noinline int __add_to_page_cache_locked(struct page *page, > > > > > > > >>> struct address_space *mapping, > > > > > > > >>> pgoff_t offset, gfp_t gfp, > > > > > > > >>> void **shadowp) > > > > > > > > > > > > > > > > It's unclear to me whether BTF_ID() requires that the target symbol be > > > > > > > > non-static. It doesn't actually reference the symbol: > > > > > > > > > > > > > > > > #define BTF_ID(prefix, name) \ > > > > > > > > __BTF_ID(__ID(__BTF_ID__##prefix##__##name##__)) [snip] > > > __add_to_page_cache_locked") made the function static which breaks the > > > build in btfids phase - but it seems to happen only on some > > > architectures. In our case, ppc64, ppc64le and riscv64 are broken, > > > x86_64, i586 and s390x succeed. (I made a mistake above, aarch64 did not > > > fail - but only because it was not built at all.) > > > > > > The thread starts with > > > http://lkml.kernel.org/r/1604661895-5495-1-git-send-email-alex.shi@linux.alibaba.com I have 5.10-rc7 build failure because of this on x86_64: BTFIDS vmlinux FAILED unresolved symbol __add_to_page_cache_locked make: *** [Makefile:1170: vmlinux] Error 255 > > Got it. So the above commit is wrong. > > The addition of "static" is incorrect here. > > Regardless of btf_id generation. > > "static noinline" means that the error injection in that spot is unreliable. > > Even when bpf is completely compiled out of the kernel. > > I finally realized that the addition of 'static' was pushed into Linus's tree :( > Please revert commit 3351b16af494 ("mm/filemap: add static for > function __add_to_page_cache_locked") At this point of release cycle we should probably go with revert, but I think the main problem is that BPF and ERROR_INJECTION use function that is not intended to be used externally. For external users add_to_page_cache_lru() and add_to_page_cache_locked() are exported and I think those should be used (see the patch below). Stanislaw diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 1388bf733071..dd6357802504 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -11487,7 +11487,8 @@ BTF_SET_START(btf_non_sleepable_error_inject) /* Three functions below can be called from sleepable and non-sleepable context. * Assume non-sleepable from bpf safety point of view. */ -BTF_ID(func, __add_to_page_cache_locked) +BTF_ID(func, add_to_page_cache_locked) +BTF_ID(func, add_to_page_cache_lru) BTF_ID(func, should_fail_alloc_page) BTF_ID(func, should_failslab) BTF_SET_END(btf_non_sleepable_error_inject) diff --git a/mm/filemap.c b/mm/filemap.c index 331f4261d723..168deec64a10 100644 --- a/mm/filemap.c +++ b/mm/filemap.c @@ -827,10 +827,10 @@ int replace_page_cache_page(struct page *old, struct page *new, gfp_t gfp_mask) } EXPORT_SYMBOL_GPL(replace_page_cache_page); -static noinline int __add_to_page_cache_locked(struct page *page, - struct address_space *mapping, - pgoff_t offset, gfp_t gfp, - void **shadowp) +static int __add_to_page_cache_locked(struct page *page, + struct address_space *mapping, + pgoff_t offset, gfp_t gfp, + void **shadowp) { XA_STATE(xas, &mapping->i_pages, offset); int huge = PageHuge(page); @@ -907,7 +907,6 @@ static noinline int __add_to_page_cache_locked(struct page *page, put_page(page); return error; } -ALLOW_ERROR_INJECTION(__add_to_page_cache_locked, ERRNO); /** * add_to_page_cache_locked - add a locked page to the pagecache @@ -928,6 +927,7 @@ int add_to_page_cache_locked(struct page *page, struct address_space *mapping, gfp_mask, NULL); } EXPORT_SYMBOL(add_to_page_cache_locked); +ALLOW_ERROR_INJECTION(add_to_page_cache_locked, ERRNO); int add_to_page_cache_lru(struct page *page, struct address_space *mapping, pgoff_t offset, gfp_t gfp_mask) @@ -957,6 +957,7 @@ int add_to_page_cache_lru(struct page *page, struct address_space *mapping, return ret; } EXPORT_SYMBOL_GPL(add_to_page_cache_lru); +ALLOW_ERROR_INJECTION(add_to_page_cache_lru, ERRNO); #ifdef CONFIG_NUMA struct page *__page_cache_alloc(gfp_t gfp) ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH] mm/filemap: add static for function __add_to_page_cache_locked 2020-12-09 14:46 ` Stanislaw Gruszka @ 2020-12-09 15:08 ` Matthew Wilcox 2020-12-09 15:51 ` Stanislaw Gruszka 0 siblings, 1 reply; 22+ messages in thread From: Matthew Wilcox @ 2020-12-09 15:08 UTC (permalink / raw) To: Stanislaw Gruszka Cc: Alexei Starovoitov, Michal Kubecek, Justin Forbes, bpf, Alex Shi, Andrew Morton, Souptick Joarder, Linux-MM, LKML, Alexei Starovoitov, Daniel Borkmann, Josef Bacik On Wed, Dec 09, 2020 at 03:46:28PM +0100, Stanislaw Gruszka wrote: > At this point of release cycle we should probably go with revert, > but I think the main problem is that BPF and ERROR_INJECTION use > function that is not intended to be used externally. For external users > add_to_page_cache_lru() and add_to_page_cache_locked() are exported > and I think those should be used (see the patch below). FWIW, I intend to do some consolidation/renaming in this area. I trust that will not be a problem? ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] mm/filemap: add static for function __add_to_page_cache_locked 2020-12-09 15:08 ` Matthew Wilcox @ 2020-12-09 15:51 ` Stanislaw Gruszka 2020-12-09 18:05 ` Christoph Hellwig 0 siblings, 1 reply; 22+ messages in thread From: Stanislaw Gruszka @ 2020-12-09 15:51 UTC (permalink / raw) To: Matthew Wilcox Cc: Alexei Starovoitov, Michal Kubecek, Justin Forbes, bpf, Alex Shi, Andrew Morton, Souptick Joarder, Linux-MM, LKML, Alexei Starovoitov, Daniel Borkmann, Josef Bacik On Wed, Dec 09, 2020 at 03:08:26PM +0000, Matthew Wilcox wrote: > On Wed, Dec 09, 2020 at 03:46:28PM +0100, Stanislaw Gruszka wrote: > > At this point of release cycle we should probably go with revert, > > but I think the main problem is that BPF and ERROR_INJECTION use > > function that is not intended to be used externally. For external users > > add_to_page_cache_lru() and add_to_page_cache_locked() are exported > > and I think those should be used (see the patch below). > > FWIW, I intend to do some consolidation/renaming in this area. I > trust that will not be a problem? If it does not break anything, it will be not a problem ;-) It's possible that __add_to_page_cache_locked() can be a global symbol with add_to_page_cache_lru() + add_to_page_cache_locked() being just static/inline wrappers around it. Stanislaw ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] mm/filemap: add static for function __add_to_page_cache_locked 2020-12-09 15:51 ` Stanislaw Gruszka @ 2020-12-09 18:05 ` Christoph Hellwig 2020-12-09 22:32 ` Steven Rostedt 0 siblings, 1 reply; 22+ messages in thread From: Christoph Hellwig @ 2020-12-09 18:05 UTC (permalink / raw) To: Stanislaw Gruszka Cc: Matthew Wilcox, Alexei Starovoitov, Michal Kubecek, Justin Forbes, bpf, Alex Shi, Andrew Morton, Souptick Joarder, Linux-MM, LKML, Alexei Starovoitov, Daniel Borkmann, Josef Bacik On Wed, Dec 09, 2020 at 04:51:48PM +0100, Stanislaw Gruszka wrote: > On Wed, Dec 09, 2020 at 03:08:26PM +0000, Matthew Wilcox wrote: > > On Wed, Dec 09, 2020 at 03:46:28PM +0100, Stanislaw Gruszka wrote: > > > At this point of release cycle we should probably go with revert, > > > but I think the main problem is that BPF and ERROR_INJECTION use > > > function that is not intended to be used externally. For external users > > > add_to_page_cache_lru() and add_to_page_cache_locked() are exported > > > and I think those should be used (see the patch below). > > > > FWIW, I intend to do some consolidation/renaming in this area. I > > trust that will not be a problem? > > If it does not break anything, it will be not a problem ;-) > > It's possible that __add_to_page_cache_locked() can be a global symbol > with add_to_page_cache_lru() + add_to_page_cache_locked() being just > static/inline wrappers around it. So what happens to BTF if we change this area entirely? Your IDs sound like some kind of ABI to me, which is extremely scary. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] mm/filemap: add static for function __add_to_page_cache_locked 2020-12-09 18:05 ` Christoph Hellwig @ 2020-12-09 22:32 ` Steven Rostedt 2020-12-10 1:12 ` Alexei Starovoitov 0 siblings, 1 reply; 22+ messages in thread From: Steven Rostedt @ 2020-12-09 22:32 UTC (permalink / raw) To: Christoph Hellwig Cc: Stanislaw Gruszka, Matthew Wilcox, Alexei Starovoitov, Michal Kubecek, Justin Forbes, bpf, Alex Shi, Andrew Morton, Souptick Joarder, Linux-MM, LKML, Alexei Starovoitov, Daniel Borkmann, Josef Bacik On Wed, Dec 09, 2020 at 06:05:52PM +0000, Christoph Hellwig wrote: > On Wed, Dec 09, 2020 at 04:51:48PM +0100, Stanislaw Gruszka wrote: > > On Wed, Dec 09, 2020 at 03:08:26PM +0000, Matthew Wilcox wrote: > > > On Wed, Dec 09, 2020 at 03:46:28PM +0100, Stanislaw Gruszka wrote: > > > > At this point of release cycle we should probably go with revert, > > > > but I think the main problem is that BPF and ERROR_INJECTION use > > > > function that is not intended to be used externally. For external users > > > > add_to_page_cache_lru() and add_to_page_cache_locked() are exported > > > > and I think those should be used (see the patch below). > > > > > > FWIW, I intend to do some consolidation/renaming in this area. I > > > trust that will not be a problem? > > > > If it does not break anything, it will be not a problem ;-) > > > > It's possible that __add_to_page_cache_locked() can be a global symbol > > with add_to_page_cache_lru() + add_to_page_cache_locked() being just > > static/inline wrappers around it. > > So what happens to BTF if we change this area entirely? Your IDs > sound like some kind of ABI to me, which is extremely scary. Is BTF becoming the new tracepoint? That is, random additions of things like: BTF_ID(func,__add_to_page_cache_locked) Like was done in commit 1e6c62a882155 ("bpf: Introduce sleepable BPF programs") without any notification to the maintainers of the __add_to_page_cache_locked code, will suddenly become an API? There's no mention in the change log to why __add_to_page_cache_locked was added. And interesting enough, __add_to_page_cache_locked is not in any header file, which is why it was switched to static. -- Steve ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] mm/filemap: add static for function __add_to_page_cache_locked 2020-12-09 22:32 ` Steven Rostedt @ 2020-12-10 1:12 ` Alexei Starovoitov 2020-12-10 2:31 ` Steven Rostedt 0 siblings, 1 reply; 22+ messages in thread From: Alexei Starovoitov @ 2020-12-10 1:12 UTC (permalink / raw) To: Steven Rostedt Cc: Christoph Hellwig, Stanislaw Gruszka, Matthew Wilcox, Michal Kubecek, Justin Forbes, bpf, Alex Shi, Andrew Morton, Souptick Joarder, Linux-MM, LKML, Alexei Starovoitov, Daniel Borkmann, Josef Bacik On Wed, Dec 9, 2020 at 2:32 PM Steven Rostedt <rostedt@goodmis.org> wrote: > > On Wed, Dec 09, 2020 at 06:05:52PM +0000, Christoph Hellwig wrote: > > On Wed, Dec 09, 2020 at 04:51:48PM +0100, Stanislaw Gruszka wrote: > > > On Wed, Dec 09, 2020 at 03:08:26PM +0000, Matthew Wilcox wrote: > > > > On Wed, Dec 09, 2020 at 03:46:28PM +0100, Stanislaw Gruszka wrote: > > > > > At this point of release cycle we should probably go with revert, > > > > > but I think the main problem is that BPF and ERROR_INJECTION use > > > > > function that is not intended to be used externally. For external users > > > > > add_to_page_cache_lru() and add_to_page_cache_locked() are exported > > > > > and I think those should be used (see the patch below). > > > > > > > > FWIW, I intend to do some consolidation/renaming in this area. I > > > > trust that will not be a problem? > > > > > > If it does not break anything, it will be not a problem ;-) > > > > > > It's possible that __add_to_page_cache_locked() can be a global symbol > > > with add_to_page_cache_lru() + add_to_page_cache_locked() being just > > > static/inline wrappers around it. > > > > So what happens to BTF if we change this area entirely? Your IDs > > sound like some kind of ABI to me, which is extremely scary. > > Is BTF becoming the new tracepoint? That is, random additions of things like: > > BTF_ID(func,__add_to_page_cache_locked) > > Like was done in commit 1e6c62a882155 ("bpf: Introduce sleepable BPF > programs") without any notification to the maintainers of the > __add_to_page_cache_locked code, will suddenly become an API? huh? what api/abi you're talking about? ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] mm/filemap: add static for function __add_to_page_cache_locked 2020-12-10 1:12 ` Alexei Starovoitov @ 2020-12-10 2:31 ` Steven Rostedt 2020-12-10 3:02 ` Alexei Starovoitov 0 siblings, 1 reply; 22+ messages in thread From: Steven Rostedt @ 2020-12-10 2:31 UTC (permalink / raw) To: Alexei Starovoitov Cc: Christoph Hellwig, Stanislaw Gruszka, Matthew Wilcox, Michal Kubecek, Justin Forbes, bpf, Alex Shi, Andrew Morton, Souptick Joarder, Linux-MM, LKML, Alexei Starovoitov, Daniel Borkmann, Josef Bacik On Wed, 9 Dec 2020 17:12:43 -0800 Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > > > > FWIW, I intend to do some consolidation/renaming in this area. I > > > > > trust that will not be a problem? > > > > > > > > If it does not break anything, it will be not a problem ;-) > > > > > > > > It's possible that __add_to_page_cache_locked() can be a global symbol > > > > with add_to_page_cache_lru() + add_to_page_cache_locked() being just > > > > static/inline wrappers around it. > > > > > > So what happens to BTF if we change this area entirely? Your IDs > > > sound like some kind of ABI to me, which is extremely scary. > > > > Is BTF becoming the new tracepoint? That is, random additions of things like: > > > > BTF_ID(func,__add_to_page_cache_locked) > > > > Like was done in commit 1e6c62a882155 ("bpf: Introduce sleepable BPF > > programs") without any notification to the maintainers of the > > __add_to_page_cache_locked code, will suddenly become an API? > > huh? what api/abi you're talking about? If the function __add_to_page_cache_locked were to be removed due to the code being rewritten, would it break any user space? If not, then there's nothing to worry about. ;-) -- Steve ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] mm/filemap: add static for function __add_to_page_cache_locked 2020-12-10 2:31 ` Steven Rostedt @ 2020-12-10 3:02 ` Alexei Starovoitov 0 siblings, 0 replies; 22+ messages in thread From: Alexei Starovoitov @ 2020-12-10 3:02 UTC (permalink / raw) To: Steven Rostedt Cc: Christoph Hellwig, Stanislaw Gruszka, Matthew Wilcox, Michal Kubecek, Justin Forbes, bpf, Alex Shi, Andrew Morton, Souptick Joarder, Linux-MM, LKML, Alexei Starovoitov, Daniel Borkmann, Josef Bacik On Wed, Dec 9, 2020 at 6:31 PM Steven Rostedt <rostedt@goodmis.org> wrote: > > On Wed, 9 Dec 2020 17:12:43 -0800 > Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > > > > > > FWIW, I intend to do some consolidation/renaming in this area. I > > > > > > trust that will not be a problem? > > > > > > > > > > If it does not break anything, it will be not a problem ;-) > > > > > > > > > > It's possible that __add_to_page_cache_locked() can be a global symbol > > > > > with add_to_page_cache_lru() + add_to_page_cache_locked() being just > > > > > static/inline wrappers around it. > > > > > > > > So what happens to BTF if we change this area entirely? Your IDs > > > > sound like some kind of ABI to me, which is extremely scary. > > > > > > Is BTF becoming the new tracepoint? That is, random additions of things like: > > > > > > BTF_ID(func,__add_to_page_cache_locked) > > > > > > Like was done in commit 1e6c62a882155 ("bpf: Introduce sleepable BPF > > > programs") without any notification to the maintainers of the > > > __add_to_page_cache_locked code, will suddenly become an API? > > > > huh? what api/abi you're talking about? > > If the function __add_to_page_cache_locked were to be removed due to > the code being rewritten, would it break any user space? If not, then > there's nothing to worry about. ;-) That function is marked with ALLOW_ERROR_INJECTION. So any script that exercises it via debugfs (or via bpf) will not work. That's nothing new. Same "breakage" happens with kprobes, etc. The function was marked with error_inject for a reason though. The refactoring or renaming of this code ideally should provide a way to do similar pattern of injecting errors in this code path. It could be a completely new function, of course. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] mm/filemap: add static for function __add_to_page_cache_locked 2020-12-07 22:53 ` Michal Kubecek 2020-12-07 22:59 ` Alexei Starovoitov @ 2020-12-09 20:59 ` Tony Luck 1 sibling, 0 replies; 22+ messages in thread From: Tony Luck @ 2020-12-09 20:59 UTC (permalink / raw) To: Michal Kubecek Cc: Alexei Starovoitov, Justin Forbes, bpf, Alex Shi, Andrew Morton, Souptick Joarder, Linux-MM, LKML, Alexei Starovoitov, Daniel Borkmann, Josef Bacik On Mon, Dec 7, 2020 at 4:36 PM Michal Kubecek <mkubecek@suse.cz> wrote: > Not removal, commit 3351b16af494 ("mm/filemap: add static for function > __add_to_page_cache_locked") made the function static which breaks the > build in btfids phase - but it seems to happen only on some > architectures. In our case, ppc64, ppc64le and riscv64 are broken, > x86_64, i586 and s390x succeed. (I made a mistake above, aarch64 did not > fail - but only because it was not built at all.) x86_64 fails for me: LD vmlinux BTFIDS vmlinux FAILED unresolved symbol __add_to_page_cache_locked make: *** [Makefile:1170: vmlinux] Error 255 ^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2020-12-10 3:02 UTC | newest] Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-11-06 11:24 [PATCH] mm/filemap: add static for function __add_to_page_cache_locked Alex Shi 2020-11-06 18:24 ` Ira Weiny 2020-11-10 3:09 ` Souptick Joarder 2020-11-10 11:58 ` Alex Shi 2020-11-10 19:50 ` Andrew Morton 2020-11-12 0:18 ` Alex Shi 2020-12-07 8:11 ` Greg Thelen 2020-12-07 8:15 ` Michal Kubecek 2020-12-07 18:35 ` Justin Forbes 2020-12-07 22:44 ` Alexei Starovoitov 2020-12-07 22:53 ` Michal Kubecek 2020-12-07 22:59 ` Alexei Starovoitov 2020-12-08 1:12 ` Alexei Starovoitov 2020-12-09 14:46 ` Stanislaw Gruszka 2020-12-09 15:08 ` Matthew Wilcox 2020-12-09 15:51 ` Stanislaw Gruszka 2020-12-09 18:05 ` Christoph Hellwig 2020-12-09 22:32 ` Steven Rostedt 2020-12-10 1:12 ` Alexei Starovoitov 2020-12-10 2:31 ` Steven Rostedt 2020-12-10 3:02 ` Alexei Starovoitov 2020-12-09 20:59 ` Tony Luck
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).