| Message ID | 20210630040034.1155892-16-willy@infradead.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Folio conversion of memcg | expand |
Hi "Matthew,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on linus/master]
[also build test WARNING on next-20210629]
[cannot apply to hnaz-linux-mm/master tip/perf/core cgroup/for-next v5.13]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Matthew-Wilcox-Oracle/Folio-conversion-of-memcg/20210630-121408
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 007b350a58754a93ca9fe50c498cc27780171153
config: arm64-randconfig-c024-20210630 (attached as .config)
compiler: aarch64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/c7ce93681b922cb2b709109c21e8206ef623e0be
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Matthew-Wilcox-Oracle/Folio-conversion-of-memcg/20210630-121408
git checkout c7ce93681b922cb2b709109c21e8206ef623e0be
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=arm64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
In file included from arch/arm64/kernel/asm-offsets.c:12:
include/linux/mm.h:1382:42: warning: 'struct folio' declared inside parameter list will not be visible outside of this definition or declaration
1382 | static inline int folio_nid(const struct folio *folio)
| ^~~~~
include/linux/mm.h: In function 'folio_nid':
include/linux/mm.h:1384:27: error: dereferencing pointer to incomplete type 'const struct folio'
1384 | return page_to_nid(&folio->page);
| ^~
In file included from include/linux/swap.h:9,
from include/linux/suspend.h:5,
from arch/arm64/kernel/asm-offsets.c:16:
include/linux/memcontrol.h: At top level:
include/linux/memcontrol.h:1138:44: warning: 'struct folio' declared inside parameter list will not be visible outside of this definition or declaration
1138 | static inline bool folio_memcg_kmem(struct folio *folio)
| ^~~~~
include/linux/memcontrol.h:1190:44: warning: 'struct folio' declared inside parameter list will not be visible outside of this definition or declaration
1190 | static inline int mem_cgroup_charge(struct folio *folio,
| ^~~~~
include/linux/memcontrol.h:1206:47: warning: 'struct folio' declared inside parameter list will not be visible outside of this definition or declaration
1206 | static inline void mem_cgroup_uncharge(struct folio *folio)
| ^~~~~
include/linux/memcontrol.h:1214:46: warning: 'struct folio' declared inside parameter list will not be visible outside of this definition or declaration
1214 | static inline void mem_cgroup_migrate(struct folio *old, struct folio *new)
| ^~~~~
include/linux/memcontrol.h:1224:61: warning: 'struct folio' declared inside parameter list will not be visible outside of this definition or declaration
1224 | static inline struct lruvec *mem_cgroup_folio_lruvec(struct folio *folio)
| ^~~~~
include/linux/memcontrol.h: In function 'mem_cgroup_folio_lruvec':
include/linux/memcontrol.h:1226:30: error: implicit declaration of function 'folio_pgdat'; did you mean 'folio_nid'? [-Werror=implicit-function-declaration]
1226 | struct pglist_data *pgdat = folio_pgdat(folio);
| ^~~~~~~~~~~
| folio_nid
>> include/linux/memcontrol.h:1226:30: warning: initialization of 'struct pglist_data *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
include/linux/memcontrol.h: At top level:
include/linux/memcontrol.h:1366:44: warning: 'struct folio' declared inside parameter list will not be visible outside of this definition or declaration
1366 | static inline void folio_memcg_lock(struct folio *folio)
| ^~~~~
include/linux/memcontrol.h:1370:46: warning: 'struct folio' declared inside parameter list will not be visible outside of this definition or declaration
1370 | static inline void folio_memcg_unlock(struct folio *folio)
| ^~~~~
include/linux/memcontrol.h: In function 'mem_cgroup_page_lruvec':
include/linux/memcontrol.h:1488:33: error: implicit declaration of function 'page_folio' [-Werror=implicit-function-declaration]
1488 | return mem_cgroup_folio_lruvec(page_folio(page));
| ^~~~~~~~~~
include/linux/memcontrol.h:1488:33: warning: passing argument 1 of 'mem_cgroup_folio_lruvec' makes pointer from integer without a cast [-Wint-conversion]
1488 | return mem_cgroup_folio_lruvec(page_folio(page));
| ^~~~~~~~~~~~~~~~
| |
| int
include/linux/memcontrol.h:1224:68: note: expected 'struct folio *' but argument is of type 'int'
1224 | static inline struct lruvec *mem_cgroup_folio_lruvec(struct folio *folio)
| ~~~~~~~~~~~~~~^~~~~
cc1: some warnings being treated as errors
--
In file included from arch/arm64/kernel/asm-offsets.c:12:
include/linux/mm.h:1382:42: warning: 'struct folio' declared inside parameter list will not be visible outside of this definition or declaration
1382 | static inline int folio_nid(const struct folio *folio)
| ^~~~~
include/linux/mm.h: In function 'folio_nid':
include/linux/mm.h:1384:27: error: dereferencing pointer to incomplete type 'const struct folio'
1384 | return page_to_nid(&folio->page);
| ^~
In file included from include/linux/swap.h:9,
from include/linux/suspend.h:5,
from arch/arm64/kernel/asm-offsets.c:16:
include/linux/memcontrol.h: At top level:
include/linux/memcontrol.h:1138:44: warning: 'struct folio' declared inside parameter list will not be visible outside of this definition or declaration
1138 | static inline bool folio_memcg_kmem(struct folio *folio)
| ^~~~~
include/linux/memcontrol.h:1190:44: warning: 'struct folio' declared inside parameter list will not be visible outside of this definition or declaration
1190 | static inline int mem_cgroup_charge(struct folio *folio,
| ^~~~~
include/linux/memcontrol.h:1206:47: warning: 'struct folio' declared inside parameter list will not be visible outside of this definition or declaration
1206 | static inline void mem_cgroup_uncharge(struct folio *folio)
| ^~~~~
include/linux/memcontrol.h:1214:46: warning: 'struct folio' declared inside parameter list will not be visible outside of this definition or declaration
1214 | static inline void mem_cgroup_migrate(struct folio *old, struct folio *new)
| ^~~~~
include/linux/memcontrol.h:1224:61: warning: 'struct folio' declared inside parameter list will not be visible outside of this definition or declaration
1224 | static inline struct lruvec *mem_cgroup_folio_lruvec(struct folio *folio)
| ^~~~~
include/linux/memcontrol.h: In function 'mem_cgroup_folio_lruvec':
include/linux/memcontrol.h:1226:30: error: implicit declaration of function 'folio_pgdat'; did you mean 'folio_nid'? [-Werror=implicit-function-declaration]
1226 | struct pglist_data *pgdat = folio_pgdat(folio);
| ^~~~~~~~~~~
| folio_nid
>> include/linux/memcontrol.h:1226:30: warning: initialization of 'struct pglist_data *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
include/linux/memcontrol.h: At top level:
include/linux/memcontrol.h:1366:44: warning: 'struct folio' declared inside parameter list will not be visible outside of this definition or declaration
1366 | static inline void folio_memcg_lock(struct folio *folio)
| ^~~~~
include/linux/memcontrol.h:1370:46: warning: 'struct folio' declared inside parameter list will not be visible outside of this definition or declaration
1370 | static inline void folio_memcg_unlock(struct folio *folio)
| ^~~~~
include/linux/memcontrol.h: In function 'mem_cgroup_page_lruvec':
include/linux/memcontrol.h:1488:33: error: implicit declaration of function 'page_folio' [-Werror=implicit-function-declaration]
1488 | return mem_cgroup_folio_lruvec(page_folio(page));
| ^~~~~~~~~~
include/linux/memcontrol.h:1488:33: warning: passing argument 1 of 'mem_cgroup_folio_lruvec' makes pointer from integer without a cast [-Wint-conversion]
1488 | return mem_cgroup_folio_lruvec(page_folio(page));
| ^~~~~~~~~~~~~~~~
| |
| int
include/linux/memcontrol.h:1224:68: note: expected 'struct folio *' but argument is of type 'int'
1224 | static inline struct lruvec *mem_cgroup_folio_lruvec(struct folio *folio)
| ~~~~~~~~~~~~~~^~~~~
cc1: some warnings being treated as errors
make[2]: *** [scripts/Makefile.build:117: arch/arm64/kernel/asm-offsets.s] Error 1
make[2]: Target '__build' not remade because of errors.
make[1]: *** [Makefile:1235: prepare0] Error 2
make[1]: Target 'prepare' not remade because of errors.
make: *** [Makefile:215: __sub-make] Error 2
make: Target 'prepare' not remade because of errors.
vim +1226 include/linux/memcontrol.h
1223
1224 static inline struct lruvec *mem_cgroup_folio_lruvec(struct folio *folio)
1225 {
> 1226 struct pglist_data *pgdat = folio_pgdat(folio);
1227 return &pgdat->__lruvec;
1228 }
1229
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
On Wed, Jun 30, 2021 at 05:00:31AM +0100, Matthew Wilcox (Oracle) wrote:
> This is the folio equivalent of mem_cgroup_page_lruvec().
I'm just going through and removing the wrappers.
Why is this function called this? There's an odd mix of
lock_page_memcg()
page_memcg()
count_memcg_page_event()
split_page_memcg()
vs
mem_cgroup_charge()
mem_cgroup_swapin_charge_page()
mem_cgroup_lruvec()
mem_cgroup_from_task()
I'd really like to call this function folio_lruvec(). It happens to
behave differently if the folio is part of a memcg, but conceptually,
lruvecs aren't particularly tied to memcgs.
... are there any other functions you'd like to rename while I'm
changing pages to folios?
On Wed, Jun 30, 2021 at 08:18:33PM +0100, Matthew Wilcox wrote: > On Wed, Jun 30, 2021 at 05:00:31AM +0100, Matthew Wilcox (Oracle) wrote: > > This is the folio equivalent of mem_cgroup_page_lruvec(). > > I'm just going through and removing the wrappers. > > Why is this function called this? There's an odd mix of > > lock_page_memcg() This was chosen to match lock_page(). > page_memcg() And this to match page_mapping(), page_zone() etc. > count_memcg_page_event() count_vm_event() > split_page_memcg() split_page(), split_page_owner() > mem_cgroup_charge() > mem_cgroup_swapin_charge_page() These are larger, heavier subsystem API calls that modify all kinds of state, not just the page. Hence the namespacing. With the smaller getter/setter type functions on pages we have traditionally used <verb>_<object> rather than page_<verb>, simply because the page is such a low-level object and many functions do sequences of page manipulations. Namespacing would turn them into: page_do_this(page); page_set_that(page); page_lock(page); if (page_is_blah(page)) page_mark_foo(page); page_unlock(page); page_put(page); which is hard on the reader because it obscures the salient part of each line behind repetetive boiler plate. > mem_cgroup_lruvec() This is arguably not a namespace prefix, but rather an accessor function to look up the memcg's lruvec. > mem_cgroup_from_task() This is a pattern to look up memcgs from various objects: - mem_cgroup_from_css() - mem_cgroup_from_counter() - mem_cgroup_from_id() - mem_cgroup_from_seq() - mem_cgroup_from_obj() and we used to have mem_cgroup_from_page() at some point... > I'd really like to call this function folio_lruvec(). That would be a better name indeed. However, pairing renames with conversion is worrisome because it means having two unintuitively diverging names for the same operation in the API during a time where everybody has to relearn the code base already. Please either precede it with a rename to page_lruvec(), or keep the existing naming pattern in the conversion. > It happens to behave differently if the folio is part of a memcg, > but conceptually, lruvecs aren't particularly tied to memcgs. Conceptually, lruvecs are always tied to a memcg. On !CONFIG_MEMCG kernels that just happens to be the root cgroup. But because it's an accessor to a page attribute, dropping the namespacing prefix is in line with things like page_memcg().
diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h index 279ea2640365..a7e1ccbc7ed6 100644 --- a/include/linux/memcontrol.h +++ b/include/linux/memcontrol.h @@ -752,18 +752,17 @@ static inline struct lruvec *mem_cgroup_lruvec(struct mem_cgroup *memcg, } /** - * mem_cgroup_page_lruvec - return lruvec for isolating/putting an LRU page - * @page: the page + * mem_cgroup_folio_lruvec - return lruvec for isolating/putting an LRU folio + * @folio: Pointer to the folio. * - * This function relies on page->mem_cgroup being stable. + * This function relies on folio->mem_cgroup being stable. */ -static inline struct lruvec *mem_cgroup_page_lruvec(struct page *page) +static inline struct lruvec *mem_cgroup_folio_lruvec(struct folio *folio) { - pg_data_t *pgdat = page_pgdat(page); - struct mem_cgroup *memcg = page_memcg(page); + struct mem_cgroup *memcg = folio_memcg(folio); - VM_WARN_ON_ONCE_PAGE(!memcg && !mem_cgroup_disabled(), page); - return mem_cgroup_lruvec(memcg, pgdat); + VM_WARN_ON_ONCE_FOLIO(!memcg && !mem_cgroup_disabled(), folio); + return mem_cgroup_lruvec(memcg, folio_pgdat(folio)); } struct mem_cgroup *mem_cgroup_from_task(struct task_struct *p); @@ -1222,10 +1221,9 @@ static inline struct lruvec *mem_cgroup_lruvec(struct mem_cgroup *memcg, return &pgdat->__lruvec; } -static inline struct lruvec *mem_cgroup_page_lruvec(struct page *page) +static inline struct lruvec *mem_cgroup_folio_lruvec(struct folio *folio) { - pg_data_t *pgdat = page_pgdat(page); - + struct pglist_data *pgdat = folio_pgdat(folio); return &pgdat->__lruvec; } @@ -1485,6 +1483,11 @@ unsigned long mem_cgroup_soft_limit_reclaim(pg_data_t *pgdat, int order, } #endif /* CONFIG_MEMCG */ +static inline struct lruvec *mem_cgroup_page_lruvec(struct page *page) +{ + return mem_cgroup_folio_lruvec(page_folio(page)); +} + static inline void __inc_lruvec_kmem_state(void *p, enum node_stat_item idx) { __mod_lruvec_kmem_state(p, idx, 1);
This is the folio equivalent of mem_cgroup_page_lruvec(). Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> --- include/linux/memcontrol.h | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-)