[PATCH] mm/filemap: add static for function __add_to_page_cache_locked

[PATCH] mm/filemap: add static for function __add_to_page_cache_locked

[Alex Shi]

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

Re: [PATCH] mm/filemap: add static for function __add_to_page_cache_locked

[Ira Weiny]

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
> 

Re: [PATCH] mm/filemap: add static for function __add_to_page_cache_locked

[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 ?

>
> 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
>
>

Re: [PATCH] mm/filemap: add static for function __add_to_page_cache_locked

[Alex Shi]

在 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
>>
>>

Re: [PATCH] mm/filemap: add static for function __add_to_page_cache_locked

[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.

> >
> > 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?

Re: [PATCH] mm/filemap: add static for function __add_to_page_cache_locked

[Alex Shi]

在 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?
> 

Re: [PATCH] mm/filemap: add static for function __add_to_page_cache_locked

[Greg Thelen]

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?
>> 

Re: [PATCH] mm/filemap: add static for function __add_to_page_cache_locked

[Michal Kubecek]

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

Re: [PATCH] mm/filemap: add static for function __add_to_page_cache_locked

[Justin Forbes]

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

Re: [PATCH] mm/filemap: add static for function __add_to_page_cache_locked

[Alexei Starovoitov]

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.

Re: [PATCH] mm/filemap: add static for function __add_to_page_cache_locked

[Michal Kubecek]

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

Re: [PATCH] mm/filemap: add static for function __add_to_page_cache_locked

[Alexei Starovoitov]

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.

Re: [PATCH] mm/filemap: add static for function __add_to_page_cache_locked

[Alexei Starovoitov]

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")

Re: [PATCH] mm/filemap: add static for function __add_to_page_cache_locked

[Stanislaw Gruszka]

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)

Re: [PATCH] mm/filemap: add static for function __add_to_page_cache_locked

[Matthew Wilcox]

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?

Re: [PATCH] mm/filemap: add static for function __add_to_page_cache_locked

[Stanislaw Gruszka]

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

Re: [PATCH] mm/filemap: add static for function __add_to_page_cache_locked

[Christoph Hellwig]

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.

Re: [PATCH] mm/filemap: add static for function __add_to_page_cache_locked

[Tony Luck]

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

Re: [PATCH] mm/filemap: add static for function __add_to_page_cache_locked

[Steven Rostedt]

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

Re: [PATCH] mm/filemap: add static for function __add_to_page_cache_locked

[Alexei Starovoitov]

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?

Re: [PATCH] mm/filemap: add static for function __add_to_page_cache_locked

[Steven Rostedt]

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

Re: [PATCH] mm/filemap: add static for function __add_to_page_cache_locked

[Alexei Starovoitov]

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.