Messages in this thread | | | Date | Wed, 23 Dec 2020 01:19:45 +0000 | From | Matthew Wilcox <> | Subject | Re: [PATCH] mm: add prototype for __add_to_page_cache_locked() |
| |
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.
| |