Re: [PATCH] hugetlb_cgroup: fix reservation accounting
On Thu, Oct 22, 2020 at 5:21 AM Michael S. Tsirkin 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:
> > Reported-by: Michal Privoznik
> > Co-developed-by: David Hildenbrand
> > Signed-off-by: David Hildenbrand
> > Signed-off-by: Mike Kravetz
>
> Acked-by: Michael S. Tsirkin
>
> > ---
> > 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(>link, >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
Re: [PATCH] hugetlb_cgroup: fix reservation accounting
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:
> Reported-by: Michal Privoznik
> Co-developed-by: David Hildenbrand
> Signed-off-by: David Hildenbrand
> Signed-off-by: Mike Kravetz
Acked-by: Michael S. Tsirkin
> ---
> 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(>link, >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
Re: [PATCH] hugetlb_cgroup: fix reservation accounting
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:
Reported-by: Michal Privoznik
Co-developed-by: David Hildenbrand
Signed-off-by: David Hildenbrand
Signed-off-by: Mike Kravetz
---
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
Thank you!
Michal
[PATCH] hugetlb_cgroup: fix reservation accounting
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:
Reported-by: Michal Privoznik
Co-developed-by: David Hildenbrand
Signed-off-by: David Hildenbrand
Signed-off-by: Mike Kravetz
---
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(>link, >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
