| Message ID | 20240319183212.17156-2-osalvador@suse.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | page_owner: Refcount fixups | expand |
On Tue, 19 Mar 2024 19:32:11 +0100 Oscar Salvador <osalvador@suse.de> wrote: > Current code does not contemplate scenarios were an allocation and > free operation on the same pages do not handle it in the same amount > at once. > To give an example, page_alloc_exact(), where we will allocate a page > of enough order to stafisfy the size request, but we will free the > remainings right away. > > In the above example, we will increment the stack_record refcount > only once, but we will decrease it the same number of times as number > of unused pages we have to free. > This will lead to a warning because of refcount imbalance. > > Fix this by recording the number of base pages in the refcount field. > > ... > > -static void dec_stack_record_count(depot_stack_handle_t handle) > +static void dec_stack_record_count(depot_stack_handle_t handle, > + int nr_base_pages) > { > struct stack_record *stack_record = __stack_depot_get_stack_record(handle); > > if (stack_record) > - refcount_dec(&stack_record->count); > + refcount_sub_and_test(nr_base_pages, &stack_record->count); > } mm/page_owner.c: In function 'dec_stack_record_count': mm/page_owner.c:226:17: error: ignoring return value of 'refcount_sub_and_test' declared with attribute 'warn_unused_result' [-Werror=unused-result] 226 | refcount_sub_and_test(nr_base_pages, &stack_record->count); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ cc1: all warnings being treated as errors
On 2024/03/20 8:24, Andrew Morton wrote: > On Tue, 19 Mar 2024 19:32:11 +0100 Oscar Salvador <osalvador@suse.de> wrote: >> -static void dec_stack_record_count(depot_stack_handle_t handle) >> +static void dec_stack_record_count(depot_stack_handle_t handle, >> + int nr_base_pages) >> { >> struct stack_record *stack_record = __stack_depot_get_stack_record(handle); >> >> if (stack_record) >> - refcount_dec(&stack_record->count); >> + refcount_sub_and_test(nr_base_pages, &stack_record->count); >> } > > mm/page_owner.c: In function 'dec_stack_record_count': > mm/page_owner.c:226:17: error: ignoring return value of 'refcount_sub_and_test' declared with attribute 'warn_unused_result' [-Werror=unused-result] > 226 | refcount_sub_and_test(nr_base_pages, &stack_record->count); > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > cc1: all warnings being treated as errors > Hmm, I guess that this is not an expected user of refcount API. If it is correct behavior that refcount becomes 0 here, you need to explain like - refcount_sub_and_test(nr_base_pages, &stack_record->count); + if (refcount_sub_and_test(nr_base_pages, &stack_record->count)) { + // Explain why nothing to do here, and explain where/how + // refcount again becomes positive value using refcount_set(). + } or replace refcount_t with atomic_t where it is legal to make refcount positive without using atomic_set().
On Wed, Mar 20, 2024 at 01:40:05PM +0900, Tetsuo Handa wrote: > Hmm, I guess that this is not an expected user of refcount API. > If it is correct behavior that refcount becomes 0 here, you need to explain like > > - refcount_sub_and_test(nr_base_pages, &stack_record->count); > + if (refcount_sub_and_test(nr_base_pages, &stack_record->count)) { > + // Explain why nothing to do here, and explain where/how > + // refcount again becomes positive value using refcount_set(). > + } > > or replace refcount_t with atomic_t where it is legal to make refcount positive > without using atomic_set(). No, it is not expected for the refcount to become 0. I do know why, but I lost a chunk in the middle of a rebase. This should have the follwing on top: diff --git a/mm/page_owner.c b/mm/page_owner.c index 2613805cb665..e477a71d6adc 100644 --- a/mm/page_owner.c +++ b/mm/page_owner.c @@ -222,8 +222,11 @@ static void dec_stack_record_count(depot_stack_handle_t handle, { struct stack_record *stack_record = __stack_depot_get_stack_record(handle); - if (stack_record) - refcount_sub_and_test(nr_base_pages, &stack_record->count); + if (!stack_record) + return; + + if (refcount_sub_and_test(nr_base_pages, &stack_record->count)) + WARN(1, "%s refcount went to 0 for %u handle\n", __func__, handle); }
On 2024/03/20 14:49, Oscar Salvador wrote: > This should have the follwing on top: > > diff --git a/mm/page_owner.c b/mm/page_owner.c > index 2613805cb665..e477a71d6adc 100644 > --- a/mm/page_owner.c > +++ b/mm/page_owner.c > @@ -222,8 +222,11 @@ static void dec_stack_record_count(depot_stack_handle_t handle, > { > struct stack_record *stack_record = __stack_depot_get_stack_record(handle); > > - if (stack_record) > - refcount_sub_and_test(nr_base_pages, &stack_record->count); > + if (!stack_record) > + return; > + > + if (refcount_sub_and_test(nr_base_pages, &stack_record->count)) > + WARN(1, "%s refcount went to 0 for %u handle\n", __func__, handle); > } > > That fixed this problem. Thank you. -------- Forwarded Message -------- Date: Wed, 20 Mar 2024 01:31:03 -0700 In-Reply-To: <4362246e-d804-43de-800b-a7840b70919a@I-love.SAKURA.ne.jp> Message-ID: <000000000000410e3b061413692b@google.com> Subject: Re: [syzbot] [mm?] WARNING: refcount bug in __reset_page_owner From: syzbot <syzbot+98c1a1753a0731df2dd4@syzkaller.appspotmail.com> To: linux-kernel@vger.kernel.org, penguin-kernel@i-love.sakura.ne.jp, syzkaller-bugs@googlegroups.com Hello, syzbot has tested the proposed patch and the reproducer did not trigger any issue: Reported-and-tested-by: syzbot+98c1a1753a0731df2dd4@syzkaller.appspotmail.com Tested on: commit: a4145ce1 Merge tag 'bcachefs-2024-03-19' of https://ev.. git tree: upstream console output: https://syzkaller.appspot.com/x/log.txt?x=131b1d66180000 kernel config: https://syzkaller.appspot.com/x/.config?x=9f47e8dfa53b0b11 dashboard link: https://syzkaller.appspot.com/bug?extid=98c1a1753a0731df2dd4 compiler: gcc (Debian 12.2.0-14) 12.2.0, GNU ld (GNU Binutils for Debian) 2.40 patch: https://syzkaller.appspot.com/x/patch.diff?x=13972985180000 Note: testing is done by a robot and is best-effort only.
Hi Oscar, kernel test robot noticed the following build warnings: [auto build test WARNING on akpm-mm/mm-everything] [also build test WARNING on linus/master next-20240320] [cannot apply to v6.8] [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#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Oscar-Salvador/mm-page_owner-Fix-refcount-imbalance/20240320-023302 base: https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git mm-everything patch link: https://lore.kernel.org/r/20240319183212.17156-2-osalvador%40suse.de patch subject: [PATCH v2 1/2] mm,page_owner: Fix refcount imbalance config: x86_64-buildonly-randconfig-004-20240320 (https://download.01.org/0day-ci/archive/20240321/202403210131.YLR6USxm-lkp@intel.com/config) compiler: clang version 17.0.6 (https://github.com/llvm/llvm-project 6009708b4367171ccdbf4b5905cb6a803753fe18) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240321/202403210131.YLR6USxm-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202403210131.YLR6USxm-lkp@intel.com/ All warnings (new ones prefixed by >>): >> mm/page_owner.c:213:3: warning: ignoring return value of function declared with 'warn_unused_result' attribute [-Wunused-result] 213 | refcount_sub_and_test(nr_base_pages, &stack_record->count); | ^~~~~~~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 1 warning generated. vim +/warn_unused_result +213 mm/page_owner.c 206 207 static void dec_stack_record_count(depot_stack_handle_t handle, 208 int nr_base_pages) 209 { 210 struct stack_record *stack_record = __stack_depot_get_stack_record(handle); 211 212 if (stack_record) > 213 refcount_sub_and_test(nr_base_pages, &stack_record->count); 214 } 215
Hi Oscar, kernel test robot noticed the following build warnings: [auto build test WARNING on akpm-mm/mm-everything] [also build test WARNING on linus/master next-20240320] [cannot apply to v6.8] [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#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Oscar-Salvador/mm-page_owner-Fix-refcount-imbalance/20240320-023302 base: https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git mm-everything patch link: https://lore.kernel.org/r/20240319183212.17156-2-osalvador%40suse.de patch subject: [PATCH v2 1/2] mm,page_owner: Fix refcount imbalance config: x86_64-randconfig-r081-20240320 (https://download.01.org/0day-ci/archive/20240321/202403210718.9IiE8NIU-lkp@intel.com/config) compiler: gcc-9 (Debian 9.3.0-22) 9.3.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240321/202403210718.9IiE8NIU-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202403210718.9IiE8NIU-lkp@intel.com/ All warnings (new ones prefixed by >>): mm/page_owner.c: In function 'dec_stack_record_count': >> mm/page_owner.c:213:3: warning: ignoring return value of 'refcount_sub_and_test', declared with attribute warn_unused_result [-Wunused-result] 213 | refcount_sub_and_test(nr_base_pages, &stack_record->count); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ vim +/refcount_sub_and_test +213 mm/page_owner.c 206 207 static void dec_stack_record_count(depot_stack_handle_t handle, 208 int nr_base_pages) 209 { 210 struct stack_record *stack_record = __stack_depot_get_stack_record(handle); 211 212 if (stack_record) > 213 refcount_sub_and_test(nr_base_pages, &stack_record->count); 214 } 215
On 3/19/24 19:32, Oscar Salvador wrote: > Current code does not contemplate scenarios were an allocation and > free operation on the same pages do not handle it in the same amount > at once. > To give an example, page_alloc_exact(), where we will allocate a page > of enough order to stafisfy the size request, but we will free the > remainings right away. > > In the above example, we will increment the stack_record refcount > only once, but we will decrease it the same number of times as number > of unused pages we have to free. > This will lead to a warning because of refcount imbalance. > > Fix this by recording the number of base pages in the refcount field. > > Reported-by: syzbot+41bbfdb8d41003d12c0f@syzkaller.appspotmail.com > Closes: https://lore.kernel.org/linux-mm/00000000000090e8ff0613eda0e5@google.com > Fixes: 217b2119b9e2 ("mm,page_owner: implement the tracking of the stacks count") > Signed-off-by: Oscar Salvador <osalvador@suse.de> With the fixup, Reviewed-by: Vlastimil Babka <vbabka@suse.cz> But I think you'll need to resend with the missing hunk already applied, it had broken whitespace in your email and IIUC this is was dropped from mm tree. Also I'd suggest a change: > +++ b/mm/page_owner.c > @@ -196,9 +196,11 @@ static void add_stack_record_to_list(struct stack_record *stack_record, > spin_unlock_irqrestore(&stack_list_lock, flags); > } > > -static void inc_stack_record_count(depot_stack_handle_t handle, gfp_t gfp_mask) > +static void inc_stack_record_count(depot_stack_handle_t handle, gfp_t gfp_mask, > + int nr_base_pages) > { > struct stack_record *stack_record = __stack_depot_get_stack_record(handle); > + int old = REFCOUNT_SATURATED; > > if (!stack_record) > return; > @@ -210,22 +212,18 @@ static void inc_stack_record_count(depot_stack_handle_t handle, gfp_t gfp_mask) > * Since we do not use STACK_DEPOT_FLAG_GET API, let us > * set a refcount of 1 ourselves. > */ > - if (refcount_read(&stack_record->count) == REFCOUNT_SATURATED) { > - int old = REFCOUNT_SATURATED; I think this was useful optimization in that most cases the count is not REFCOUNT_SATURATED so we don't have to go for the expensive cmpxchg all the time. Or do I miss a reason why this was changed? > - > - if (atomic_try_cmpxchg_relaxed(&stack_record->count.refs, &old, 1)) > - /* Add the new stack_record to our list */ > - add_stack_record_to_list(stack_record, gfp_mask); > - } > - refcount_inc(&stack_record->count); > + if (atomic_try_cmpxchg_relaxed(&stack_record->count.refs, &old, 1)) > + add_stack_record_to_list(stack_record, gfp_mask); > + refcount_add(nr_base_pages, &stack_record->count); > } >
diff --git a/Documentation/mm/page_owner.rst b/Documentation/mm/page_owner.rst index 0d0334cd5179..3a45a20fc05a 100644 --- a/Documentation/mm/page_owner.rst +++ b/Documentation/mm/page_owner.rst @@ -24,10 +24,10 @@ fragmentation statistics can be obtained through gfp flag information of each page. It is already implemented and activated if page owner is enabled. Other usages are more than welcome. -It can also be used to show all the stacks and their outstanding -allocations, which gives us a quick overview of where the memory is going -without the need to screen through all the pages and match the allocation -and free operation. +It can also be used to show all the stacks and their current number of +allocated base pages, which gives us a quick overview of where the memory +is going without the need to screen through all the pages and match the +allocation and free operation. page owner is disabled by default. So, if you'd like to use it, you need to add "page_owner=on" to your boot cmdline. If the kernel is built @@ -75,42 +75,45 @@ Usage cat /sys/kernel/debug/page_owner_stacks/show_stacks > stacks.txt cat stacks.txt - prep_new_page+0xa9/0x120 - get_page_from_freelist+0x7e6/0x2140 - __alloc_pages+0x18a/0x370 - new_slab+0xc8/0x580 - ___slab_alloc+0x1f2/0xaf0 - __slab_alloc.isra.86+0x22/0x40 - kmem_cache_alloc+0x31b/0x350 - __khugepaged_enter+0x39/0x100 - dup_mmap+0x1c7/0x5ce - copy_process+0x1afe/0x1c90 - kernel_clone+0x9a/0x3c0 - __do_sys_clone+0x66/0x90 - do_syscall_64+0x7f/0x160 - entry_SYSCALL_64_after_hwframe+0x6c/0x74 - stack_count: 234 + post_alloc_hook+0x177/0x1a0 + get_page_from_freelist+0xd01/0xd80 + __alloc_pages+0x39e/0x7e0 + allocate_slab+0xbc/0x3f0 + ___slab_alloc+0x528/0x8a0 + kmem_cache_alloc+0x224/0x3b0 + sk_prot_alloc+0x58/0x1a0 + sk_alloc+0x32/0x4f0 + inet_create+0x427/0xb50 + __sock_create+0x2e4/0x650 + inet_ctl_sock_create+0x30/0x180 + igmp_net_init+0xc1/0x130 + ops_init+0x167/0x410 + setup_net+0x304/0xa60 + copy_net_ns+0x29b/0x4a0 + create_new_namespaces+0x4a1/0x820 + nr_base_pages: 16 ... ... echo 7000 > /sys/kernel/debug/page_owner_stacks/count_threshold cat /sys/kernel/debug/page_owner_stacks/show_stacks> stacks_7000.txt cat stacks_7000.txt - prep_new_page+0xa9/0x120 - get_page_from_freelist+0x7e6/0x2140 - __alloc_pages+0x18a/0x370 - alloc_pages_mpol+0xdf/0x1e0 - folio_alloc+0x14/0x50 - filemap_alloc_folio+0xb0/0x100 - page_cache_ra_unbounded+0x97/0x180 - filemap_fault+0x4b4/0x1200 - __do_fault+0x2d/0x110 - do_pte_missing+0x4b0/0xa30 - __handle_mm_fault+0x7fa/0xb70 - handle_mm_fault+0x125/0x300 - do_user_addr_fault+0x3c9/0x840 - exc_page_fault+0x68/0x150 - asm_exc_page_fault+0x22/0x30 - stack_count: 8248 + post_alloc_hook+0x177/0x1a0 + get_page_from_freelist+0xd01/0xd80 + __alloc_pages+0x39e/0x7e0 + alloc_pages_mpol+0x22e/0x490 + folio_alloc+0xd5/0x110 + filemap_alloc_folio+0x78/0x230 + page_cache_ra_order+0x287/0x6f0 + filemap_get_pages+0x517/0x1160 + filemap_read+0x304/0x9f0 + xfs_file_buffered_read+0xe6/0x1d0 [xfs] + xfs_file_read_iter+0x1f0/0x380 [xfs] + __kernel_read+0x3b9/0x730 + kernel_read_file+0x309/0x4d0 + __do_sys_finit_module+0x381/0x730 + do_syscall_64+0x8d/0x150 + entry_SYSCALL_64_after_hwframe+0x62/0x6a + nr_base_pages: 20824 ... cat /sys/kernel/debug/page_owner > page_owner_full.txt diff --git a/mm/page_owner.c b/mm/page_owner.c index d17d1351ec84..2613805cb665 100644 --- a/mm/page_owner.c +++ b/mm/page_owner.c @@ -196,9 +196,11 @@ static void add_stack_record_to_list(struct stack_record *stack_record, spin_unlock_irqrestore(&stack_list_lock, flags); } -static void inc_stack_record_count(depot_stack_handle_t handle, gfp_t gfp_mask) +static void inc_stack_record_count(depot_stack_handle_t handle, gfp_t gfp_mask, + int nr_base_pages) { struct stack_record *stack_record = __stack_depot_get_stack_record(handle); + int old = REFCOUNT_SATURATED; if (!stack_record) return; @@ -210,22 +212,18 @@ static void inc_stack_record_count(depot_stack_handle_t handle, gfp_t gfp_mask) * Since we do not use STACK_DEPOT_FLAG_GET API, let us * set a refcount of 1 ourselves. */ - if (refcount_read(&stack_record->count) == REFCOUNT_SATURATED) { - int old = REFCOUNT_SATURATED; - - if (atomic_try_cmpxchg_relaxed(&stack_record->count.refs, &old, 1)) - /* Add the new stack_record to our list */ - add_stack_record_to_list(stack_record, gfp_mask); - } - refcount_inc(&stack_record->count); + if (atomic_try_cmpxchg_relaxed(&stack_record->count.refs, &old, 1)) + add_stack_record_to_list(stack_record, gfp_mask); + refcount_add(nr_base_pages, &stack_record->count); } -static void dec_stack_record_count(depot_stack_handle_t handle) +static void dec_stack_record_count(depot_stack_handle_t handle, + int nr_base_pages) { struct stack_record *stack_record = __stack_depot_get_stack_record(handle); if (stack_record) - refcount_dec(&stack_record->count); + refcount_sub_and_test(nr_base_pages, &stack_record->count); } void __reset_page_owner(struct page *page, unsigned short order) @@ -263,7 +261,7 @@ void __reset_page_owner(struct page *page, unsigned short order) * the machinery is not ready yet, we cannot decrement * their refcount either. */ - dec_stack_record_count(alloc_handle); + dec_stack_record_count(alloc_handle, 1 << order); } static inline void __set_page_owner_handle(struct page_ext *page_ext, @@ -305,7 +303,7 @@ noinline void __set_page_owner(struct page *page, unsigned short order, return; __set_page_owner_handle(page_ext, handle, order, gfp_mask); page_ext_put(page_ext); - inc_stack_record_count(handle, gfp_mask); + inc_stack_record_count(handle, gfp_mask, 1 << order); } void __set_page_owner_migrate_reason(struct page *page, int reason) @@ -861,11 +859,11 @@ static void *stack_next(struct seq_file *m, void *v, loff_t *ppos) return stack; } -static unsigned long page_owner_stack_threshold; +static unsigned long page_owner_pages_threshold; static int stack_print(struct seq_file *m, void *v) { - int i, stack_count; + int i, nr_base_pages; struct stack *stack = v; unsigned long *entries; unsigned long nr_entries; @@ -876,14 +874,14 @@ static int stack_print(struct seq_file *m, void *v) nr_entries = stack_record->size; entries = stack_record->entries; - stack_count = refcount_read(&stack_record->count) - 1; + nr_base_pages = refcount_read(&stack_record->count) - 1; - if (stack_count < 1 || stack_count < page_owner_stack_threshold) + if (nr_base_pages < 1 || nr_base_pages < page_owner_pages_threshold) return 0; for (i = 0; i < nr_entries; i++) seq_printf(m, " %pS\n", (void *)entries[i]); - seq_printf(m, "stack_count: %d\n\n", stack_count); + seq_printf(m, "nr_base_pages: %d\n\n", nr_base_pages); return 0; } @@ -913,13 +911,13 @@ static const struct file_operations page_owner_stack_operations = { static int page_owner_threshold_get(void *data, u64 *val) { - *val = READ_ONCE(page_owner_stack_threshold); + *val = READ_ONCE(page_owner_pages_threshold); return 0; } static int page_owner_threshold_set(void *data, u64 val) { - WRITE_ONCE(page_owner_stack_threshold, val); + WRITE_ONCE(page_owner_pages_threshold, val); return 0; }
Current code does not contemplate scenarios were an allocation and free operation on the same pages do not handle it in the same amount at once. To give an example, page_alloc_exact(), where we will allocate a page of enough order to stafisfy the size request, but we will free the remainings right away. In the above example, we will increment the stack_record refcount only once, but we will decrease it the same number of times as number of unused pages we have to free. This will lead to a warning because of refcount imbalance. Fix this by recording the number of base pages in the refcount field. Reported-by: syzbot+41bbfdb8d41003d12c0f@syzkaller.appspotmail.com Closes: https://lore.kernel.org/linux-mm/00000000000090e8ff0613eda0e5@google.com Fixes: 217b2119b9e2 ("mm,page_owner: implement the tracking of the stacks count") Signed-off-by: Oscar Salvador <osalvador@suse.de> --- Documentation/mm/page_owner.rst | 73 +++++++++++++++++---------------- mm/page_owner.c | 38 ++++++++--------- 2 files changed, 56 insertions(+), 55 deletions(-)