linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] hugetlb_cgroup: fix reservation accounting
@ 2020-10-21 20:44 Mike Kravetz
  2020-10-22  7:42 ` Michal Privoznik
  2020-10-22 12:21 ` Michael S. Tsirkin
  0 siblings, 2 replies; 4+ messages in thread
From: Mike Kravetz @ 2020-10-21 20:44 UTC (permalink / raw)
  To: linux-mm, linux-kernel
  Cc: David Hildenbrand, Mina Almasry, Michal Privoznik,
	Michael S . Tsirkin, Michal Hocko, Muchun Song,
	Aneesh Kumar K . V, Tejun Heo, Andrew Morton, Mike Kravetz,
	stable

Michal Privoznik was using "free page reporting" in QEMU/virtio-balloon
with hugetlbfs and hit the warning below.  QEMU with free page hinting
uses fallocate(FALLOC_FL_PUNCH_HOLE) to discard pages that are reported
as free by a VM. The reporting granularity is in pageblock granularity.
So when the guest reports 2M chunks, we fallocate(FALLOC_FL_PUNCH_HOLE)
one huge page in QEMU.

[  315.251417] ------------[ cut here ]------------
[  315.251424] WARNING: CPU: 7 PID: 6636 at mm/page_counter.c:57 page_counter_uncharge+0x4b/0x50
[  315.251425] Modules linked in: ...
[  315.251466] CPU: 7 PID: 6636 Comm: qemu-system-x86 Not tainted 5.9.0 #137
[  315.251467] Hardware name: Gigabyte Technology Co., Ltd. X570 AORUS PRO/X570 AORUS PRO, BIOS F21 07/31/2020
[  315.251469] RIP: 0010:page_counter_uncharge+0x4b/0x50
...
[  315.251479] Call Trace:
[  315.251485]  hugetlb_cgroup_uncharge_file_region+0x4b/0x80
[  315.251487]  region_del+0x1d3/0x300
[  315.251489]  hugetlb_unreserve_pages+0x39/0xb0
[  315.251492]  remove_inode_hugepages+0x1a8/0x3d0
[  315.251495]  ? tlb_finish_mmu+0x7a/0x1d0
[  315.251497]  hugetlbfs_fallocate+0x3c4/0x5c0
[  315.251519]  ? kvm_arch_vcpu_ioctl_run+0x614/0x1700 [kvm]
[  315.251522]  ? file_has_perm+0xa2/0xb0
[  315.251524]  ? inode_security+0xc/0x60
[  315.251525]  ? selinux_file_permission+0x4e/0x120
[  315.251527]  vfs_fallocate+0x146/0x290
[  315.251529]  __x64_sys_fallocate+0x3e/0x70
[  315.251531]  do_syscall_64+0x33/0x40
[  315.251533]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
...
[  315.251542] ---[ end trace 4c88c62ccb1349c9 ]---

Investigation of the issue uncovered bugs in hugetlb cgroup reservation
accounting.  This patch addresses the found issues.

Fixes: 075a61d07a8e ("hugetlb_cgroup: add accounting for shared mappings")
Cc: <stable@vger.kernel.org>
Reported-by: Michal Privoznik <mprivozn@redhat.com>
Co-developed-by: David Hildenbrand <david@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
---
 mm/hugetlb.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 67fc6383995b..b853a11de14f 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -655,6 +655,8 @@ static long region_del(struct resv_map *resv, long f, long t)
 			}
 
 			del += t - f;
+			hugetlb_cgroup_uncharge_file_region(
+				resv, rg, t - f);
 
 			/* New entry for end of split region */
 			nrg->from = t;
@@ -667,9 +669,6 @@ static long region_del(struct resv_map *resv, long f, long t)
 			/* Original entry is trimmed */
 			rg->to = f;
 
-			hugetlb_cgroup_uncharge_file_region(
-				resv, rg, nrg->to - nrg->from);
-
 			list_add(&nrg->link, &rg->link);
 			nrg = NULL;
 			break;
@@ -685,17 +684,17 @@ static long region_del(struct resv_map *resv, long f, long t)
 		}
 
 		if (f <= rg->from) {	/* Trim beginning of region */
-			del += t - rg->from;
-			rg->from = t;
-
 			hugetlb_cgroup_uncharge_file_region(resv, rg,
 							    t - rg->from);
-		} else {		/* Trim end of region */
-			del += rg->to - f;
-			rg->to = f;
 
+			del += t - rg->from;
+			rg->from = t;
+		} else {		/* Trim end of region */
 			hugetlb_cgroup_uncharge_file_region(resv, rg,
 							    rg->to - f);
+
+			del += rg->to - f;
+			rg->to = f;
 		}
 	}
 
@@ -2454,6 +2453,9 @@ struct page *alloc_huge_page(struct vm_area_struct *vma,
 
 		rsv_adjust = hugepage_subpool_put_pages(spool, 1);
 		hugetlb_acct_memory(h, -rsv_adjust);
+		if (deferred_reserve)
+			hugetlb_cgroup_uncharge_page_rsvd(hstate_index(h),
+					pages_per_huge_page(h), page);
 	}
 	return page;
 
-- 
2.25.4



^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] hugetlb_cgroup: fix reservation accounting
  2020-10-21 20:44 [PATCH] hugetlb_cgroup: fix reservation accounting Mike Kravetz
@ 2020-10-22  7:42 ` Michal Privoznik
  2020-10-22 12:21 ` Michael S. Tsirkin
  1 sibling, 0 replies; 4+ messages in thread
From: Michal Privoznik @ 2020-10-22  7:42 UTC (permalink / raw)
  To: Mike Kravetz, linux-mm, linux-kernel
  Cc: David Hildenbrand, Mina Almasry, Michael S . Tsirkin,
	Michal Hocko, Muchun Song, Aneesh Kumar K . V, Tejun Heo,
	Andrew Morton, stable

On 10/21/20 10:44 PM, Mike Kravetz wrote:
> Michal Privoznik was using "free page reporting" in QEMU/virtio-balloon
> with hugetlbfs and hit the warning below.  QEMU with free page hinting
> uses fallocate(FALLOC_FL_PUNCH_HOLE) to discard pages that are reported
> as free by a VM. The reporting granularity is in pageblock granularity.
> So when the guest reports 2M chunks, we fallocate(FALLOC_FL_PUNCH_HOLE)
> one huge page in QEMU.
> 
> [  315.251417] ------------[ cut here ]------------
> [  315.251424] WARNING: CPU: 7 PID: 6636 at mm/page_counter.c:57 page_counter_uncharge+0x4b/0x50
> [  315.251425] Modules linked in: ...
> [  315.251466] CPU: 7 PID: 6636 Comm: qemu-system-x86 Not tainted 5.9.0 #137
> [  315.251467] Hardware name: Gigabyte Technology Co., Ltd. X570 AORUS PRO/X570 AORUS PRO, BIOS F21 07/31/2020
> [  315.251469] RIP: 0010:page_counter_uncharge+0x4b/0x50
> ...
> [  315.251479] Call Trace:
> [  315.251485]  hugetlb_cgroup_uncharge_file_region+0x4b/0x80
> [  315.251487]  region_del+0x1d3/0x300
> [  315.251489]  hugetlb_unreserve_pages+0x39/0xb0
> [  315.251492]  remove_inode_hugepages+0x1a8/0x3d0
> [  315.251495]  ? tlb_finish_mmu+0x7a/0x1d0
> [  315.251497]  hugetlbfs_fallocate+0x3c4/0x5c0
> [  315.251519]  ? kvm_arch_vcpu_ioctl_run+0x614/0x1700 [kvm]
> [  315.251522]  ? file_has_perm+0xa2/0xb0
> [  315.251524]  ? inode_security+0xc/0x60
> [  315.251525]  ? selinux_file_permission+0x4e/0x120
> [  315.251527]  vfs_fallocate+0x146/0x290
> [  315.251529]  __x64_sys_fallocate+0x3e/0x70
> [  315.251531]  do_syscall_64+0x33/0x40
> [  315.251533]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> ...
> [  315.251542] ---[ end trace 4c88c62ccb1349c9 ]---
> 
> Investigation of the issue uncovered bugs in hugetlb cgroup reservation
> accounting.  This patch addresses the found issues.
> 
> Fixes: 075a61d07a8e ("hugetlb_cgroup: add accounting for shared mappings")
> Cc: <stable@vger.kernel.org>
> Reported-by: Michal Privoznik <mprivozn@redhat.com>
> Co-developed-by: David Hildenbrand <david@redhat.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
> ---
>   mm/hugetlb.c | 20 +++++++++++---------
>   1 file changed, 11 insertions(+), 9 deletions(-)

I just tested this and can confirm it fixes the problem:

Tested-by: Michal Privoznik <mprivozn@redhat.com>

Thank you!
Michal



^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] hugetlb_cgroup: fix reservation accounting
  2020-10-21 20:44 [PATCH] hugetlb_cgroup: fix reservation accounting Mike Kravetz
  2020-10-22  7:42 ` Michal Privoznik
@ 2020-10-22 12:21 ` Michael S. Tsirkin
  2020-10-28 19:37   ` Mina Almasry
  1 sibling, 1 reply; 4+ messages in thread
From: Michael S. Tsirkin @ 2020-10-22 12:21 UTC (permalink / raw)
  To: Mike Kravetz
  Cc: linux-mm, linux-kernel, David Hildenbrand, Mina Almasry,
	Michal Privoznik, Michal Hocko, Muchun Song, Aneesh Kumar K . V,
	Tejun Heo, Andrew Morton, stable

On Wed, Oct 21, 2020 at 01:44:26PM -0700, Mike Kravetz wrote:
> Michal Privoznik was using "free page reporting" in QEMU/virtio-balloon
> with hugetlbfs and hit the warning below.  QEMU with free page hinting
> uses fallocate(FALLOC_FL_PUNCH_HOLE) to discard pages that are reported
> as free by a VM. The reporting granularity is in pageblock granularity.
> So when the guest reports 2M chunks, we fallocate(FALLOC_FL_PUNCH_HOLE)
> one huge page in QEMU.
> 
> [  315.251417] ------------[ cut here ]------------
> [  315.251424] WARNING: CPU: 7 PID: 6636 at mm/page_counter.c:57 page_counter_uncharge+0x4b/0x50
> [  315.251425] Modules linked in: ...
> [  315.251466] CPU: 7 PID: 6636 Comm: qemu-system-x86 Not tainted 5.9.0 #137
> [  315.251467] Hardware name: Gigabyte Technology Co., Ltd. X570 AORUS PRO/X570 AORUS PRO, BIOS F21 07/31/2020
> [  315.251469] RIP: 0010:page_counter_uncharge+0x4b/0x50
> ...
> [  315.251479] Call Trace:
> [  315.251485]  hugetlb_cgroup_uncharge_file_region+0x4b/0x80
> [  315.251487]  region_del+0x1d3/0x300
> [  315.251489]  hugetlb_unreserve_pages+0x39/0xb0
> [  315.251492]  remove_inode_hugepages+0x1a8/0x3d0
> [  315.251495]  ? tlb_finish_mmu+0x7a/0x1d0
> [  315.251497]  hugetlbfs_fallocate+0x3c4/0x5c0
> [  315.251519]  ? kvm_arch_vcpu_ioctl_run+0x614/0x1700 [kvm]
> [  315.251522]  ? file_has_perm+0xa2/0xb0
> [  315.251524]  ? inode_security+0xc/0x60
> [  315.251525]  ? selinux_file_permission+0x4e/0x120
> [  315.251527]  vfs_fallocate+0x146/0x290
> [  315.251529]  __x64_sys_fallocate+0x3e/0x70
> [  315.251531]  do_syscall_64+0x33/0x40
> [  315.251533]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> ...
> [  315.251542] ---[ end trace 4c88c62ccb1349c9 ]---
> 
> Investigation of the issue uncovered bugs in hugetlb cgroup reservation
> accounting.  This patch addresses the found issues.
> 
> Fixes: 075a61d07a8e ("hugetlb_cgroup: add accounting for shared mappings")
> Cc: <stable@vger.kernel.org>
> Reported-by: Michal Privoznik <mprivozn@redhat.com>
> Co-developed-by: David Hildenbrand <david@redhat.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>

Acked-by: Michael S. Tsirkin <mst@redhat.com>

> ---
>  mm/hugetlb.c | 20 +++++++++++---------
>  1 file changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 67fc6383995b..b853a11de14f 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -655,6 +655,8 @@ static long region_del(struct resv_map *resv, long f, long t)
>  			}
>  
>  			del += t - f;
> +			hugetlb_cgroup_uncharge_file_region(
> +				resv, rg, t - f);
>  
>  			/* New entry for end of split region */
>  			nrg->from = t;
> @@ -667,9 +669,6 @@ static long region_del(struct resv_map *resv, long f, long t)
>  			/* Original entry is trimmed */
>  			rg->to = f;
>  
> -			hugetlb_cgroup_uncharge_file_region(
> -				resv, rg, nrg->to - nrg->from);
> -
>  			list_add(&nrg->link, &rg->link);
>  			nrg = NULL;
>  			break;
> @@ -685,17 +684,17 @@ static long region_del(struct resv_map *resv, long f, long t)
>  		}
>  
>  		if (f <= rg->from) {	/* Trim beginning of region */
> -			del += t - rg->from;
> -			rg->from = t;
> -
>  			hugetlb_cgroup_uncharge_file_region(resv, rg,
>  							    t - rg->from);
> -		} else {		/* Trim end of region */
> -			del += rg->to - f;
> -			rg->to = f;
>  
> +			del += t - rg->from;
> +			rg->from = t;
> +		} else {		/* Trim end of region */
>  			hugetlb_cgroup_uncharge_file_region(resv, rg,
>  							    rg->to - f);
> +
> +			del += rg->to - f;
> +			rg->to = f;
>  		}
>  	}
>  
> @@ -2454,6 +2453,9 @@ struct page *alloc_huge_page(struct vm_area_struct *vma,
>  
>  		rsv_adjust = hugepage_subpool_put_pages(spool, 1);
>  		hugetlb_acct_memory(h, -rsv_adjust);
> +		if (deferred_reserve)
> +			hugetlb_cgroup_uncharge_page_rsvd(hstate_index(h),
> +					pages_per_huge_page(h), page);
>  	}
>  	return page;
>  
> -- 
> 2.25.4



^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] hugetlb_cgroup: fix reservation accounting
  2020-10-22 12:21 ` Michael S. Tsirkin
@ 2020-10-28 19:37   ` Mina Almasry
  0 siblings, 0 replies; 4+ messages in thread
From: Mina Almasry @ 2020-10-28 19:37 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: Mike Kravetz, Linux-MM, open list, David Hildenbrand,
	Michal Privoznik, Michal Hocko, Muchun Song, Aneesh Kumar K . V,
	Tejun Heo, Andrew Morton, stable

On Thu, Oct 22, 2020 at 5:21 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Wed, Oct 21, 2020 at 01:44:26PM -0700, Mike Kravetz wrote:
> > Michal Privoznik was using "free page reporting" in QEMU/virtio-balloon
> > with hugetlbfs and hit the warning below.  QEMU with free page hinting
> > uses fallocate(FALLOC_FL_PUNCH_HOLE) to discard pages that are reported
> > as free by a VM. The reporting granularity is in pageblock granularity.
> > So when the guest reports 2M chunks, we fallocate(FALLOC_FL_PUNCH_HOLE)
> > one huge page in QEMU.
> >
> > [  315.251417] ------------[ cut here ]------------
> > [  315.251424] WARNING: CPU: 7 PID: 6636 at mm/page_counter.c:57 page_counter_uncharge+0x4b/0x50
> > [  315.251425] Modules linked in: ...
> > [  315.251466] CPU: 7 PID: 6636 Comm: qemu-system-x86 Not tainted 5.9.0 #137
> > [  315.251467] Hardware name: Gigabyte Technology Co., Ltd. X570 AORUS PRO/X570 AORUS PRO, BIOS F21 07/31/2020
> > [  315.251469] RIP: 0010:page_counter_uncharge+0x4b/0x50
> > ...
> > [  315.251479] Call Trace:
> > [  315.251485]  hugetlb_cgroup_uncharge_file_region+0x4b/0x80
> > [  315.251487]  region_del+0x1d3/0x300
> > [  315.251489]  hugetlb_unreserve_pages+0x39/0xb0
> > [  315.251492]  remove_inode_hugepages+0x1a8/0x3d0
> > [  315.251495]  ? tlb_finish_mmu+0x7a/0x1d0
> > [  315.251497]  hugetlbfs_fallocate+0x3c4/0x5c0
> > [  315.251519]  ? kvm_arch_vcpu_ioctl_run+0x614/0x1700 [kvm]
> > [  315.251522]  ? file_has_perm+0xa2/0xb0
> > [  315.251524]  ? inode_security+0xc/0x60
> > [  315.251525]  ? selinux_file_permission+0x4e/0x120
> > [  315.251527]  vfs_fallocate+0x146/0x290
> > [  315.251529]  __x64_sys_fallocate+0x3e/0x70
> > [  315.251531]  do_syscall_64+0x33/0x40
> > [  315.251533]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> > ...
> > [  315.251542] ---[ end trace 4c88c62ccb1349c9 ]---
> >
> > Investigation of the issue uncovered bugs in hugetlb cgroup reservation
> > accounting.  This patch addresses the found issues.
> >
> > Fixes: 075a61d07a8e ("hugetlb_cgroup: add accounting for shared mappings")
> > Cc: <stable@vger.kernel.org>
> > Reported-by: Michal Privoznik <mprivozn@redhat.com>
> > Co-developed-by: David Hildenbrand <david@redhat.com>
> > Signed-off-by: David Hildenbrand <david@redhat.com>
> > Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
>
> Acked-by: Michael S. Tsirkin <mst@redhat.com>
>
> > ---
> >  mm/hugetlb.c | 20 +++++++++++---------
> >  1 file changed, 11 insertions(+), 9 deletions(-)
> >
> > diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> > index 67fc6383995b..b853a11de14f 100644
> > --- a/mm/hugetlb.c
> > +++ b/mm/hugetlb.c
> > @@ -655,6 +655,8 @@ static long region_del(struct resv_map *resv, long f, long t)
> >                       }
> >
> >                       del += t - f;
> > +                     hugetlb_cgroup_uncharge_file_region(
> > +                             resv, rg, t - f);
> >
> >                       /* New entry for end of split region */
> >                       nrg->from = t;
> > @@ -667,9 +669,6 @@ static long region_del(struct resv_map *resv, long f, long t)
> >                       /* Original entry is trimmed */
> >                       rg->to = f;
> >
> > -                     hugetlb_cgroup_uncharge_file_region(
> > -                             resv, rg, nrg->to - nrg->from);
> > -
> >                       list_add(&nrg->link, &rg->link);
> >                       nrg = NULL;
> >                       break;
> > @@ -685,17 +684,17 @@ static long region_del(struct resv_map *resv, long f, long t)
> >               }
> >
> >               if (f <= rg->from) {    /* Trim beginning of region */
> > -                     del += t - rg->from;
> > -                     rg->from = t;
> > -
> >                       hugetlb_cgroup_uncharge_file_region(resv, rg,
> >                                                           t - rg->from);
> > -             } else {                /* Trim end of region */
> > -                     del += rg->to - f;
> > -                     rg->to = f;
> >
> > +                     del += t - rg->from;
> > +                     rg->from = t;
> > +             } else {                /* Trim end of region */
> >                       hugetlb_cgroup_uncharge_file_region(resv, rg,
> >                                                           rg->to - f);
> > +
> > +                     del += rg->to - f;
> > +                     rg->to = f;
> >               }
> >       }
> >
> > @@ -2454,6 +2453,9 @@ struct page *alloc_huge_page(struct vm_area_struct *vma,
> >
> >               rsv_adjust = hugepage_subpool_put_pages(spool, 1);
> >               hugetlb_acct_memory(h, -rsv_adjust);
> > +             if (deferred_reserve)
> > +                     hugetlb_cgroup_uncharge_page_rsvd(hstate_index(h),
> > +                                     pages_per_huge_page(h), page);
> >       }
> >       return page;
> >
> > --
> > 2.25.4
>

Sorry for the late review. Looks good to me.

Reviewed-by: Mina Almasry <almasrymina@google.com>


^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2020-10-28 19:37 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-21 20:44 [PATCH] hugetlb_cgroup: fix reservation accounting Mike Kravetz
2020-10-22  7:42 ` Michal Privoznik
2020-10-22 12:21 ` Michael S. Tsirkin
2020-10-28 19:37   ` Mina Almasry

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).