[PATCH] mm: add prototype for __add_to_page_cache_locked()

[PATCH] mm: add prototype for __add_to_page_cache_locked()

[Souptick Joarder]

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,
              ^~~~~~~~~~~~~~~~~~~~~~~~~~

A previous attempt to make this function static leads to
compile error for few architectures.

Adding a prototype will silence the warning.

Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
Cc: Alex Shi <alex.shi@linux.alibaba.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
---
 include/linux/mm.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index 5299b90a..ac07f65 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -216,6 +216,12 @@ int overcommit_kbytes_handler(struct ctl_table *, int, void *, size_t *,
 		loff_t *);
 int overcommit_policy_handler(struct ctl_table *, int, void *, size_t *,
 		loff_t *);
+/*
+ * Any attempt to mark this function as static leads to build failure
+ * for few architectures. Adding a prototype to silence gcc warning.
+ */
+int __add_to_page_cache_locked(struct page *page, struct address_space *mapping,
+		pgoff_t offset, gfp_t gfp, void **shadowp);
 
 #define nth_page(page,n) pfn_to_page(page_to_pfn((page)) + (n))
 
-- 
1.9.1

Re: [PATCH] mm: add prototype for __add_to_page_cache_locked()

[Matthew Wilcox]

On Tue, Dec 22, 2020 at 07:49:52PM +0530, Souptick Joarder wrote:
> Otherwise it cause gcc warning:
>           ^~~~~~~~~~~~~~~

That line is just confusing.

> ../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,
>               ^~~~~~~~~~~~~~~~~~~~~~~~~~

And I don't think those two lines add much value, do you?

> A previous attempt to make this function static leads to
> compile error for few architectures.

It might be better to say why it has to be non-static here (because it's
an error injection point).  And it's not architecture dependent (afaik),
it's whether error injection is enabled in the config.

> Adding a prototype will silence the warning.
> 
> Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
> Cc: Alex Shi <alex.shi@linux.alibaba.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> ---
>  include/linux/mm.h | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 5299b90a..ac07f65 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -216,6 +216,12 @@ int overcommit_kbytes_handler(struct ctl_table *, int, void *, size_t *,
>  		loff_t *);
>  int overcommit_policy_handler(struct ctl_table *, int, void *, size_t *,
>  		loff_t *);
> +/*
> + * Any attempt to mark this function as static leads to build failure
> + * for few architectures. Adding a prototype to silence gcc warning.
> + */

We don't need a comment here for this.  The commit log is enough.

> +int __add_to_page_cache_locked(struct page *page, struct address_space *mapping,
> +		pgoff_t offset, gfp_t gfp, void **shadowp);

Please name that 'index', not 'offset'.

Re: [PATCH] mm: add prototype for __add_to_page_cache_locked()

[Andrew Morton]

On Tue, 22 Dec 2020 20:40:00 +0000 Matthew Wilcox <willy@infradead.org> wrote:

> On Tue, Dec 22, 2020 at 07:49:52PM +0530, Souptick Joarder wrote:
> > Otherwise it cause gcc warning:
> >           ^~~~~~~~~~~~~~~
> 
> That line is just confusing.

I cleaned up the changelog.  It is presently

: Subject: include/linux/mm.h: add prototype for __add_to_page_cache_locked()
: 
: Otherwise it causes a gcc warning:
: 
: ../mm/filemap.c:830:14: warning: no previous prototype for
: `__add_to_page_cache_locked' [-Wmissing-prototypes]
: 
: A previous attempt to make this function static led to compilation
: errors for a few architectures, because __add_to_page_cache_locked() is
: referred to by BPF code.
: 
: Adding a prototype will silence the warning.

> > --- a/include/linux/mm.h
> > +++ b/include/linux/mm.h
> > @@ -216,6 +216,12 @@ int overcommit_kbytes_handler(struct ctl_table *, int, void *, size_t *,
> >  		loff_t *);
> >  int overcommit_policy_handler(struct ctl_table *, int, void *, size_t *,
> >  		loff_t *);
> > +/*
> > + * Any attempt to mark this function as static leads to build failure
> > + * for few architectures. Adding a prototype to silence gcc warning.
> > + */
> 
> We don't need a comment here for this.  The commit log is enough.

I think it's OK - people do send patches which remove a prototype and
also make the function static.  A tree-wide grep would catch the bpf
reference but I suspect people tend to grep for "foo(" rather then
"foo".

> > +int __add_to_page_cache_locked(struct page *page, struct address_space *mapping,
> > +		pgoff_t offset, gfp_t gfp, void **shadowp);
> 
> Please name that 'index', not 'offset'.

I too prefer index over offset.  

X1:/usr/src/linux-5.10> grep -r "pgoff_t offset" . | wc -l
52
X1:/usr/src/linux-5.10> grep -r "pgoff_t index" . | wc -l 
250

But renaming this arg should be a separate patch.

And I don't think we should be preparing large "rename offset to index"
patches, please.  The value/noise ratio is too low.

Re: [PATCH] mm: add prototype for __add_to_page_cache_locked()

[Matthew Wilcox]

On Tue, Dec 22, 2020 at 03:53:45PM -0800, Andrew Morton wrote:
> : A previous attempt to make this function static led to compilation
> : errors for a few architectures, because __add_to_page_cache_locked() is
> : referred to by BPF code.

Yes, but it's wrong, because it's not architecture dependent.  It
depends on CONFIG_DEBUG_INFO_BTF

> > > +/*
> > > + * Any attempt to mark this function as static leads to build failure
> > > + * for few architectures. Adding a prototype to silence gcc warning.
> > > + */
> > 
> > We don't need a comment here for this.  The commit log is enough.
> 
> I think it's OK - people do send patches which remove a prototype and
> also make the function static.  A tree-wide grep would catch the bpf
> reference but I suspect people tend to grep for "foo(" rather then
> "foo".

... and the same wrong information is present here.  If there's going to
be a comment here at least make it something informative like

/* Must be visible for error injection */

> > > +int __add_to_page_cache_locked(struct page *page, struct address_space *mapping,
> > > +		pgoff_t offset, gfp_t gfp, void **shadowp);
> > 
> > Please name that 'index', not 'offset'.
> 
> I too prefer index over offset.  
> 
> X1:/usr/src/linux-5.10> grep -r "pgoff_t offset" . | wc -l
> 52
> X1:/usr/src/linux-5.10> grep -r "pgoff_t index" . | wc -l 
> 250
> 
> But renaming this arg should be a separate patch.

... but this is a new prototype.  Prototype names don't have to match
the function name (and often don't ...)

> And I don't think we should be preparing large "rename offset to index"
> patches, please.  The value/noise ratio is too low.

I'm only fixing them as I change those functions.  I just object to
introducing new wrong ones.

Re: [PATCH] mm: add prototype for __add_to_page_cache_locked()

[Christoph Hellwig]

Can we please make the eBPF code stop referencing this function instead
of papering over this crap?  It has no business poking into page cache
internals.

Re: [PATCH] mm: add prototype for __add_to_page_cache_locked()

[Matthew Wilcox]

On Wed, Dec 23, 2020 at 08:31:26AM +0000, Christoph Hellwig wrote:
> Can we please make the eBPF code stop referencing this function instead
> of papering over this crap?  It has no business poking into page cache
> internals.

The reference from the BPF code is simply "you can inject errors here".
And I think we want to be able to inject errors to test the error paths,
no?

Re: [PATCH] mm: add prototype for __add_to_page_cache_locked()

[Christoph Hellwig]

On Wed, Dec 23, 2020 at 12:11:36PM +0000, Matthew Wilcox wrote:
> On Wed, Dec 23, 2020 at 08:31:26AM +0000, Christoph Hellwig wrote:
> > Can we please make the eBPF code stop referencing this function instead
> > of papering over this crap?  It has no business poking into page cache
> > internals.
> 
> The reference from the BPF code is simply "you can inject errors here".
> And I think we want to be able to inject errors to test the error paths,
> no?

Something that expects a symbol to be global is just pretty broken.
I think it need to change so that whatever instrumentation is done can
coexist with a static function.

Re: [PATCH] mm: add prototype for __add_to_page_cache_locked()

[Alexei Starovoitov]

On Wed, Dec 23, 2020 at 7:54 AM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Wed, Dec 23, 2020 at 12:11:36PM +0000, Matthew Wilcox wrote:
> > On Wed, Dec 23, 2020 at 08:31:26AM +0000, Christoph Hellwig wrote:
> > > Can we please make the eBPF code stop referencing this function instead
> > > of papering over this crap?  It has no business poking into page cache
> > > internals.
> >
> > The reference from the BPF code is simply "you can inject errors here".
> > And I think we want to be able to inject errors to test the error paths,
> > no?
>
> Something that expects a symbol to be global is just pretty broken.
> I think it need to change so that whatever instrumentation is done can
> coexist with a static function.

Pls read commit description that made it global.
It has nothing to do with bpf.

[PATCH] mm: add prototype for __add_to_page_cache_locked()

[Souptick Joarder]

Otherwise it causes a gcc warning:

../mm/filemap.c:830:14: warning: no previous prototype for
`__add_to_page_cache_locked' [-Wmissing-prototypes]

A previous attempt to make this function static led to compilation
errors when CONFIG_DEBUG_INFO_BTF is enabled because
__add_to_page_cache_locked() is referred to by BPF code.

Adding a prototype will silence the warning.

Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
Cc: Alex Shi <alex.shi@linux.alibaba.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Matthew Wilcox <willy@infradead.org>
---
 include/linux/mm.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index 5299b90a..c1e9081 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -216,6 +216,13 @@ int overcommit_kbytes_handler(struct ctl_table *, int, void *, size_t *,
 		loff_t *);
 int overcommit_policy_handler(struct ctl_table *, int, void *, size_t *,
 		loff_t *);
+/*
+ * Any attempt to mark this function as static leads to build failure
+ * when CONFIG_DEBUG_INFO_BTF is enabled because __add_to_page_cache_locked()
+ * is referred to by BPF code. This must be visible for error injection.
+ */
+int __add_to_page_cache_locked(struct page *page, struct address_space *mapping,
+		pgoff_t index, gfp_t gfp, void **shadowp);
 
 #define nth_page(page,n) pfn_to_page(page_to_pfn((page)) + (n))
 
-- 
1.9.1

Re: [PATCH] mm: add prototype for __add_to_page_cache_locked()

[Souptick Joarder]

On Wed, Dec 23, 2020 at 8:46 AM Souptick Joarder <jrdr.linux@gmail.com> wrote:
>
> Otherwise it causes a gcc warning:
>
> ../mm/filemap.c:830:14: warning: no previous prototype for
> `__add_to_page_cache_locked' [-Wmissing-prototypes]
>
> A previous attempt to make this function static led to compilation
> errors when CONFIG_DEBUG_INFO_BTF is enabled because
> __add_to_page_cache_locked() is referred to by BPF code.
>
> Adding a prototype will silence the warning.
>

Please ignore this patch. I forget to update version.

> Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
> Cc: Alex Shi <alex.shi@linux.alibaba.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Matthew Wilcox <willy@infradead.org>
> ---
>  include/linux/mm.h | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 5299b90a..c1e9081 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -216,6 +216,13 @@ int overcommit_kbytes_handler(struct ctl_table *, int, void *, size_t *,
>                 loff_t *);
>  int overcommit_policy_handler(struct ctl_table *, int, void *, size_t *,
>                 loff_t *);
> +/*
> + * Any attempt to mark this function as static leads to build failure
> + * when CONFIG_DEBUG_INFO_BTF is enabled because __add_to_page_cache_locked()
> + * is referred to by BPF code. This must be visible for error injection.
> + */
> +int __add_to_page_cache_locked(struct page *page, struct address_space *mapping,
> +               pgoff_t index, gfp_t gfp, void **shadowp);
>
>  #define nth_page(page,n) pfn_to_page(page_to_pfn((page)) + (n))
>
> --
> 1.9.1
>