| Message ID | 1608646792-29073-1-git-send-email-jrdr.linux@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm: add prototype for __add_to_page_cache_locked() | expand |
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'.
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.
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.
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.
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?
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.
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.
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))
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(+)