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 <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
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
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 <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> ---
> 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
>
On Fri, Nov 6, 2020 at 4:55 PM Alex Shi <[email protected]> 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 <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> ---
> 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
>
>
在 2020/11/10 上午11:09, Souptick Joarder 写道:
> On Fri, Nov 6, 2020 at 4:55 PM Alex Shi <[email protected]> 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 <[email protected]>
>> Cc: Andrew Morton <[email protected]>
>> Cc: [email protected]
>> Cc: [email protected]
>> ---
>> 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
>>
>>
On Tue, 10 Nov 2020 08:39:24 +0530 Souptick Joarder <[email protected]> wrote:
> On Fri, Nov 6, 2020 at 4:55 PM Alex Shi <[email protected]> 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 <[email protected]>
> > Cc: Andrew Morton <[email protected]>
> > Cc: [email protected]
> > Cc: [email protected]
> > ---
> > 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?
在 2020/11/11 上午3:50, Andrew Morton 写道:
> On Tue, 10 Nov 2020 08:39:24 +0530 Souptick Joarder <[email protected]> wrote:
>
>> On Fri, Nov 6, 2020 at 4:55 PM Alex Shi <[email protected]> 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 <[email protected]>
>>> Cc: Andrew Morton <[email protected]>
>>> Cc: [email protected]
>>> Cc: [email protected]
>>> ---
>>> 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?
>
Alex Shi <[email protected]> wrote:
> 在 2020/11/11 上午3:50, Andrew Morton 写道:
>> On Tue, 10 Nov 2020 08:39:24 +0530 Souptick Joarder <[email protected]> wrote:
>>
>>> On Fri, Nov 6, 2020 at 4:55 PM Alex Shi <[email protected]> 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 <[email protected]>
>>>> Cc: Andrew Morton <[email protected]>
>>>> Cc: [email protected]
>>>> Cc: [email protected]
>>>> ---
>>>> 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?
>>
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 <[email protected]> wrote:
> >
> >> On Fri, Nov 6, 2020 at 4:55 PM Alex Shi <[email protected]> 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 <[email protected]>
> >>> Cc: Andrew Morton <[email protected]>
> >>> Cc: [email protected]
> >>> Cc: [email protected]
> >>> ---
> >>> 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
On Mon, Dec 7, 2020 at 2:16 AM Michal Kubecek <[email protected]> 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 <[email protected]> wrote:
> > >
> > >> On Fri, Nov 6, 2020 at 4:55 PM Alex Shi <[email protected]> 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 <[email protected]>
> > >>> Cc: Andrew Morton <[email protected]>
> > >>> Cc: [email protected]
> > >>> Cc: [email protected]
> > >>> ---
> > >>> 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
On Mon, Dec 7, 2020 at 10:36 AM Justin Forbes <[email protected]> wrote:
>
> On Mon, Dec 7, 2020 at 2:16 AM Michal Kubecek <[email protected]> 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 <[email protected]> wrote:
> > > >
> > > >> On Fri, Nov 6, 2020 at 4:55 PM Alex Shi <[email protected]> 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 <[email protected]>
> > > >>> Cc: Andrew Morton <[email protected]>
> > > >>> Cc: [email protected]
> > > >>> Cc: [email protected]
> > > >>> ---
> > > >>> 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.
On Mon, Dec 07, 2020 at 02:44:22PM -0800, Alexei Starovoitov wrote:
> On Mon, Dec 7, 2020 at 10:36 AM Justin Forbes <[email protected]> wrote:
> >
> > On Mon, Dec 7, 2020 at 2:16 AM Michal Kubecek <[email protected]> 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 <[email protected]> wrote:
> > > > >
> > > > >> On Fri, Nov 6, 2020 at 4:55 PM Alex Shi <[email protected]> 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 <[email protected]>
> > > > >>> Cc: Andrew Morton <[email protected]>
> > > > >>> Cc: [email protected]
> > > > >>> Cc: [email protected]
> > > > >>> ---
> > > > >>> 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/[email protected]
Michal
On Mon, Dec 7, 2020 at 2:53 PM Michal Kubecek <[email protected]> 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 <[email protected]> wrote:
> > >
> > > On Mon, Dec 7, 2020 at 2:16 AM Michal Kubecek <[email protected]> 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 <[email protected]> wrote:
> > > > > >
> > > > > >> On Fri, Nov 6, 2020 at 4:55 PM Alex Shi <[email protected]> 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 <[email protected]>
> > > > > >>> Cc: Andrew Morton <[email protected]>
> > > > > >>> Cc: [email protected]
> > > > > >>> Cc: [email protected]
> > > > > >>> ---
> > > > > >>> 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/[email protected]
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.
On Mon, Dec 7, 2020 at 2:59 PM Alexei Starovoitov
<[email protected]> wrote:
>
> On Mon, Dec 7, 2020 at 2:53 PM Michal Kubecek <[email protected]> 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 <[email protected]> wrote:
> > > >
> > > > On Mon, Dec 7, 2020 at 2:16 AM Michal Kubecek <[email protected]> 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 <[email protected]> wrote:
> > > > > > >
> > > > > > >> On Fri, Nov 6, 2020 at 4:55 PM Alex Shi <[email protected]> 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 <[email protected]>
> > > > > > >>> Cc: Andrew Morton <[email protected]>
> > > > > > >>> Cc: [email protected]
> > > > > > >>> Cc: [email protected]
> > > > > > >>> ---
> > > > > > >>> 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/[email protected]
>
> 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")
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.
On Mon, Dec 7, 2020 at 4:36 PM Michal Kubecek <[email protected]> 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
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
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/[email protected]
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)
On Wed, Dec 9, 2020 at 2:32 PM Steven Rostedt <[email protected]> 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?
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?
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
On Wed, 9 Dec 2020 17:12:43 -0800
Alexei Starovoitov <[email protected]> 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
On Wed, Dec 9, 2020 at 6:31 PM Steven Rostedt <[email protected]> wrote:
>
> On Wed, 9 Dec 2020 17:12:43 -0800
> Alexei Starovoitov <[email protected]> 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.