Files can be created and mapped in an explicitly mounted hugetlbfs filesystem. If pages in such files are migrated, the filesystem usage will not be decremented for the associated pages. This can result in mmap or page allocation failures as it appears there are fewer pages in the filesystem than there should be.
For example, a test program which hole punches, faults and migrates pages in such a file (1G in size) will eventually fail because it can not allocate a page. Reported counts and usage at time of failure:
node0 537 free_hugepages 1024 nr_hugepages 0 surplus_hugepages node1 1000 free_hugepages 1024 nr_hugepages 0 surplus_hugepages
Filesystem Size Used Avail Use% Mounted on nodev 4.0G 4.0G 0 100% /var/opt/hugepool
Note that the filesystem shows 4G of pages used, while actual usage is 511 pages (just under 1G). Failed trying to allocate page 512.
If a hugetlb page is associated with an explicitly mounted filesystem, this information in contained in the page_private field. At migration time, this information is not preserved. To fix, simply transfer page_private from old to new page at migration time if necessary. Also, migrate_page_states() unconditionally clears page_private and PagePrivate of the old page. It is unlikely, but possible that these fields could be non-NULL and are needed at hugetlb free page time. So, do not touch these fields for hugetlb pages.
Cc: stable@vger.kernel.org Fixes: 290408d4a250 ("hugetlb: hugepage migration core") Signed-off-by: Mike Kravetz mike.kravetz@oracle.com --- fs/hugetlbfs/inode.c | 10 ++++++++++ mm/migrate.c | 10 ++++++++-- 2 files changed, 18 insertions(+), 2 deletions(-)
diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c index 32920a10100e..fb6de1db8806 100644 --- a/fs/hugetlbfs/inode.c +++ b/fs/hugetlbfs/inode.c @@ -859,6 +859,16 @@ static int hugetlbfs_migrate_page(struct address_space *mapping, rc = migrate_huge_page_move_mapping(mapping, newpage, page); if (rc != MIGRATEPAGE_SUCCESS) return rc; + + /* + * page_private is subpool pointer in hugetlb pages, transfer + * if needed. + */ + if (page_private(page) && !page_private(newpage)) { + set_page_private(newpage, page_private(page)); + set_page_private(page, 0); + } + if (mode != MIGRATE_SYNC_NO_COPY) migrate_page_copy(newpage, page); else diff --git a/mm/migrate.c b/mm/migrate.c index f7e4bfdc13b7..0d9708803553 100644 --- a/mm/migrate.c +++ b/mm/migrate.c @@ -703,8 +703,14 @@ void migrate_page_states(struct page *newpage, struct page *page) */ if (PageSwapCache(page)) ClearPageSwapCache(page); - ClearPagePrivate(page); - set_page_private(page, 0); + /* + * Unlikely, but PagePrivate and page_private could potentially + * contain information needed at hugetlb free page time. + */ + if (!PageHuge(page)) { + ClearPagePrivate(page); + set_page_private(page, 0); + }
/* * If any waiters have accumulated on the new page then
On 1/30/19 1:14 PM, Mike Kravetz wrote:
Files can be created and mapped in an explicitly mounted hugetlbfs filesystem. If pages in such files are migrated, the filesystem usage will not be decremented for the associated pages. This can result in mmap or page allocation failures as it appears there are fewer pages in the filesystem than there should be.
Does anyone have a little time to take a look at this?
While migration of hugetlb pages 'should' not be a common issue, we have seen it happen via soft memory errors/page poisoning in production environments. Didn't see a leak in that case as it was with pages in a Sys V shared mem segment. However, our DB code is starting to make use of files in explicitly mounted hugetlbfs filesystems. Therefore, we are more likely to hit this bug in the field.
On Thu, Feb 07, 2019 at 10:50:55AM -0800, Mike Kravetz wrote:
On 1/30/19 1:14 PM, Mike Kravetz wrote:
Files can be created and mapped in an explicitly mounted hugetlbfs filesystem. If pages in such files are migrated, the filesystem usage will not be decremented for the associated pages. This can result in mmap or page allocation failures as it appears there are fewer pages in the filesystem than there should be.
Does anyone have a little time to take a look at this?
While migration of hugetlb pages 'should' not be a common issue, we have seen it happen via soft memory errors/page poisoning in production environments. Didn't see a leak in that case as it was with pages in a Sys V shared mem segment. However, our DB code is starting to make use of files in explicitly mounted hugetlbfs filesystems. Therefore, we are more likely to hit this bug in the field.
Hi Mike,
Thank you for finding/reporting the problem. # sorry for my late response.
For example, a test program which hole punches, faults and migrates pages in such a file (1G in size) will eventually fail because it can not allocate a page. Reported counts and usage at time of failure:
node0 537 free_hugepages 1024 nr_hugepages 0 surplus_hugepages node1 1000 free_hugepages 1024 nr_hugepages 0 surplus_hugepages
Filesystem Size Used Avail Use% Mounted on nodev 4.0G 4.0G 0 100% /var/opt/hugepool
Note that the filesystem shows 4G of pages used, while actual usage is 511 pages (just under 1G). Failed trying to allocate page 512.
If a hugetlb page is associated with an explicitly mounted filesystem, this information in contained in the page_private field. At migration time, this information is not preserved. To fix, simply transfer page_private from old to new page at migration time if necessary. Also, migrate_page_states() unconditionally clears page_private and PagePrivate of the old page. It is unlikely, but possible that these fields could be non-NULL and are needed at hugetlb free page time. So, do not touch these fields for hugetlb pages.
Cc: stable@vger.kernel.org Fixes: 290408d4a250 ("hugetlb: hugepage migration core") Signed-off-by: Mike Kravetz mike.kravetz@oracle.com
fs/hugetlbfs/inode.c | 10 ++++++++++ mm/migrate.c | 10 ++++++++-- 2 files changed, 18 insertions(+), 2 deletions(-)
diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c index 32920a10100e..fb6de1db8806 100644 --- a/fs/hugetlbfs/inode.c +++ b/fs/hugetlbfs/inode.c @@ -859,6 +859,16 @@ static int hugetlbfs_migrate_page(struct address_space *mapping, rc = migrate_huge_page_move_mapping(mapping, newpage, page); if (rc != MIGRATEPAGE_SUCCESS) return rc;
- /*
* page_private is subpool pointer in hugetlb pages, transfer* if needed.*/- if (page_private(page) && !page_private(newpage)) {
set_page_private(newpage, page_private(page));set_page_private(page, 0);
You don't have to copy PagePrivate flag?
- }
- if (mode != MIGRATE_SYNC_NO_COPY) migrate_page_copy(newpage, page); else
diff --git a/mm/migrate.c b/mm/migrate.c index f7e4bfdc13b7..0d9708803553 100644 --- a/mm/migrate.c +++ b/mm/migrate.c @@ -703,8 +703,14 @@ void migrate_page_states(struct page *newpage, struct page *page) */ if (PageSwapCache(page)) ClearPageSwapCache(page);
- ClearPagePrivate(page);
- set_page_private(page, 0);
- /*
* Unlikely, but PagePrivate and page_private could potentially* contain information needed at hugetlb free page time.*/- if (!PageHuge(page)) {
ClearPagePrivate(page);set_page_private(page, 0);- }
# This argument is mainly for existing code...
According to the comment on migrate_page():
/* * Common logic to directly migrate a single LRU page suitable for * pages that do not use PagePrivate/PagePrivate2. * * Pages are locked upon entry and exit. */ int migrate_page(struct address_space *mapping, ...
So this common logic assumes that page_private is not used, so why do we explicitly clear page_private in migrate_page_states()? buffer_migrate_page(), which is commonly used for the case when page_private is used, does that clearing outside migrate_page_states(). So I thought that hugetlbfs_migrate_page() could do in the similar manner. IOW, migrate_page_states() should not do anything on PagePrivate. But there're a few other .migratepage callbacks, and I'm not sure all of them are safe for the change, so this approach might not fit for a small fix.
# BTW, there seems a typo in $SUBJECT.
Thanks, Naoya Horiguchi
On 2/7/19 6:31 PM, Naoya Horiguchi wrote:
On Thu, Feb 07, 2019 at 10:50:55AM -0800, Mike Kravetz wrote:
On 1/30/19 1:14 PM, Mike Kravetz wrote:
+++ b/fs/hugetlbfs/inode.c @@ -859,6 +859,16 @@ static int hugetlbfs_migrate_page(struct address_space *mapping, rc = migrate_huge_page_move_mapping(mapping, newpage, page); if (rc != MIGRATEPAGE_SUCCESS) return rc;
- /*
* page_private is subpool pointer in hugetlb pages, transfer* if needed.*/- if (page_private(page) && !page_private(newpage)) {
set_page_private(newpage, page_private(page));set_page_private(page, 0);You don't have to copy PagePrivate flag?
Well my original thought was no. For hugetlb pages, PagePrivate is not associated with page_private. It indicates a reservation was consumed. It is set when a hugetlb page is newly allocated and the allocation is associated with a reservation and the global reservation count is decremented. When the page is added to the page cache or rmap, PagePrivate is cleared. If the page is free'ed before being added to page cache or rmap, PagePrivate tells free_huge_page to restore (increment) the reserve count as we did not 'instantiate' the page.
So, PagePrivate is only set from the time a huge page is allocated until it is added to page cache or rmap. My original thought was that the page could not be migrated during this time. However, I am not sure if that reasoning is correct. The page is not locked, so it would appear that it could be migrated? But, if it can be migrated at this time then perhaps there are bigger issues for the (hugetlb) page fault code?
- }
- if (mode != MIGRATE_SYNC_NO_COPY) migrate_page_copy(newpage, page); else
diff --git a/mm/migrate.c b/mm/migrate.c index f7e4bfdc13b7..0d9708803553 100644 --- a/mm/migrate.c +++ b/mm/migrate.c @@ -703,8 +703,14 @@ void migrate_page_states(struct page *newpage, struct page *page) */ if (PageSwapCache(page)) ClearPageSwapCache(page);
- ClearPagePrivate(page);
- set_page_private(page, 0);
- /*
* Unlikely, but PagePrivate and page_private could potentially* contain information needed at hugetlb free page time.*/- if (!PageHuge(page)) {
ClearPagePrivate(page);set_page_private(page, 0);- }
# This argument is mainly for existing code...
According to the comment on migrate_page():
/* * Common logic to directly migrate a single LRU page suitable for * pages that do not use PagePrivate/PagePrivate2. * * Pages are locked upon entry and exit. */ int migrate_page(struct address_space *mapping, ...So this common logic assumes that page_private is not used, so why do we explicitly clear page_private in migrate_page_states()?
Perhaps someone else knows. If not, I can do some git research and try to find out why.
buffer_migrate_page(), which is commonly used for the case when page_private is used, does that clearing outside migrate_page_states(). So I thought that hugetlbfs_migrate_page() could do in the similar manner. IOW, migrate_page_states() should not do anything on PagePrivate. But there're a few other .migratepage callbacks, and I'm not sure all of them are safe for the change, so this approach might not fit for a small fix.
I will look at those as well unless someone knows without researching.
# BTW, there seems a typo in $SUBJECT.
Thanks!
On Thu, Feb 07, 2019 at 09:50:30PM -0800, Mike Kravetz wrote:
On 2/7/19 6:31 PM, Naoya Horiguchi wrote:
On Thu, Feb 07, 2019 at 10:50:55AM -0800, Mike Kravetz wrote:
On 1/30/19 1:14 PM, Mike Kravetz wrote:
+++ b/fs/hugetlbfs/inode.c @@ -859,6 +859,16 @@ static int hugetlbfs_migrate_page(struct address_space *mapping, rc = migrate_huge_page_move_mapping(mapping, newpage, page); if (rc != MIGRATEPAGE_SUCCESS) return rc;
- /*
* page_private is subpool pointer in hugetlb pages, transfer* if needed.*/- if (page_private(page) && !page_private(newpage)) {
set_page_private(newpage, page_private(page));set_page_private(page, 0);You don't have to copy PagePrivate flag?
Well my original thought was no. For hugetlb pages, PagePrivate is not associated with page_private. It indicates a reservation was consumed. It is set when a hugetlb page is newly allocated and the allocation is associated with a reservation and the global reservation count is decremented. When the page is added to the page cache or rmap, PagePrivate is cleared. If the page is free'ed before being added to page cache or rmap, PagePrivate tells free_huge_page to restore (increment) the reserve count as we did not 'instantiate' the page.
So, PagePrivate is only set from the time a huge page is allocated until it is added to page cache or rmap. My original thought was that the page could not be migrated during this time. However, I am not sure if that reasoning is correct. The page is not locked, so it would appear that it could be migrated? But, if it can be migrated at this time then perhaps there are bigger issues for the (hugetlb) page fault code?
In my understanding, free hugetlb pages are not expected to be passed to migrate_pages(), and currently that's ensured by each migration caller which checks and avoids free hugetlb pages on its own. migrate_pages() and its internal code are probably not aware of handling free hugetlb pages, so if they are accidentally passed to migration code, that's a big problem as you are concerned. So the above reasoning should work at least this assumption is correct.
Most of migration callers are not intersted in moving free hugepages. The one I'm not sure of is the code path from alloc_contig_range(). If someone think it's worthwhile to migrate free hugepage to get bigger contiguous memory, he/she tries to enable that code path and the assumption will be broken.
Thanks, Naoya Horiguchi
- }
- if (mode != MIGRATE_SYNC_NO_COPY) migrate_page_copy(newpage, page); else
diff --git a/mm/migrate.c b/mm/migrate.c index f7e4bfdc13b7..0d9708803553 100644 --- a/mm/migrate.c +++ b/mm/migrate.c @@ -703,8 +703,14 @@ void migrate_page_states(struct page *newpage, struct page *page) */ if (PageSwapCache(page)) ClearPageSwapCache(page);
- ClearPagePrivate(page);
- set_page_private(page, 0);
- /*
* Unlikely, but PagePrivate and page_private could potentially* contain information needed at hugetlb free page time.*/- if (!PageHuge(page)) {
ClearPagePrivate(page);set_page_private(page, 0);- }
# This argument is mainly for existing code...
According to the comment on migrate_page():
/* * Common logic to directly migrate a single LRU page suitable for * pages that do not use PagePrivate/PagePrivate2. * * Pages are locked upon entry and exit. */ int migrate_page(struct address_space *mapping, ...So this common logic assumes that page_private is not used, so why do we explicitly clear page_private in migrate_page_states()?
Perhaps someone else knows. If not, I can do some git research and try to find out why.
buffer_migrate_page(), which is commonly used for the case when page_private is used, does that clearing outside migrate_page_states(). So I thought that hugetlbfs_migrate_page() could do in the similar manner. IOW, migrate_page_states() should not do anything on PagePrivate. But there're a few other .migratepage callbacks, and I'm not sure all of them are safe for the change, so this approach might not fit for a small fix.
I will look at those as well unless someone knows without researching.
# BTW, there seems a typo in $SUBJECT.
Thanks!
-- Mike Kravetz
On 2/7/19 11:31 PM, Naoya Horiguchi wrote:
On Thu, Feb 07, 2019 at 09:50:30PM -0800, Mike Kravetz wrote:
On 2/7/19 6:31 PM, Naoya Horiguchi wrote:
On Thu, Feb 07, 2019 at 10:50:55AM -0800, Mike Kravetz wrote:
On 1/30/19 1:14 PM, Mike Kravetz wrote:
+++ b/fs/hugetlbfs/inode.c @@ -859,6 +859,16 @@ static int hugetlbfs_migrate_page(struct address_space *mapping, rc = migrate_huge_page_move_mapping(mapping, newpage, page); if (rc != MIGRATEPAGE_SUCCESS) return rc;
- /*
* page_private is subpool pointer in hugetlb pages, transfer* if needed.*/- if (page_private(page) && !page_private(newpage)) {
set_page_private(newpage, page_private(page));set_page_private(page, 0);You don't have to copy PagePrivate flag?
Well my original thought was no. For hugetlb pages, PagePrivate is not associated with page_private. It indicates a reservation was consumed. It is set when a hugetlb page is newly allocated and the allocation is associated with a reservation and the global reservation count is decremented. When the page is added to the page cache or rmap, PagePrivate is cleared. If the page is free'ed before being added to page cache or rmap, PagePrivate tells free_huge_page to restore (increment) the reserve count as we did not 'instantiate' the page.
So, PagePrivate is only set from the time a huge page is allocated until it is added to page cache or rmap. My original thought was that the page could not be migrated during this time. However, I am not sure if that reasoning is correct. The page is not locked, so it would appear that it could be migrated? But, if it can be migrated at this time then perhaps there are bigger issues for the (hugetlb) page fault code?
In my understanding, free hugetlb pages are not expected to be passed to migrate_pages(), and currently that's ensured by each migration caller which checks and avoids free hugetlb pages on its own. migrate_pages() and its internal code are probably not aware of handling free hugetlb pages, so if they are accidentally passed to migration code, that's a big problem as you are concerned. So the above reasoning should work at least this assumption is correct.
Most of migration callers are not intersted in moving free hugepages. The one I'm not sure of is the code path from alloc_contig_range(). If someone think it's worthwhile to migrate free hugepage to get bigger contiguous memory, he/she tries to enable that code path and the assumption will be broken.
You are correct. We do not migrate free huge pages. I was thinking more about problems if we migrate a page while it is being added to a task's page table as in hugetlb_no_page.
Commit bcc54222309c ("mm: hugetlb: introduce page_huge_active") addresses this issue, but I believe there is a bug in the implementation. isolate_huge_page contains this test:
if (!page_huge_active(page) || !get_page_unless_zero(page)) { ret = false; goto unlock; }
If the condition is not met, then the huge page can be isolated and migrated.
In hugetlb_no_page, there is this block of code:
page = alloc_huge_page(vma, haddr, 0); if (IS_ERR(page)) { ret = vmf_error(PTR_ERR(page)); goto out; } clear_huge_page(page, address, pages_per_huge_page(h)); __SetPageUptodate(page); set_page_huge_active(page);
if (vma->vm_flags & VM_MAYSHARE) { int err = huge_add_to_page_cache(page, mapping, idx); if (err) { put_page(page); if (err == -EEXIST) goto retry; goto out; } } else { lock_page(page); if (unlikely(anon_vma_prepare(vma))) { ret = VM_FAULT_OOM; goto backout_unlocked; } anon_rmap = 1; } } else {
Note that we call set_page_huge_active BEFORE locking the page. This means that we can isolate the page and have migration take place while we continue to add the page to page tables. I was able to make this happen by adding a udelay() after set_page_huge_active to simulate worst case scheduling behavior. It resulted in VM_BUG_ON while unlocking page. My test had several threads faulting in huge pages. Another thread was offlining the memory blocks forcing migration.
To fix this, we need to delay the set_page_huge_active call until after the page is locked. I am testing a patch with this change. Perhaps we should even delay calling set_page_huge_active until we know there are no errors and we know the page is actually in page tables?
While looking at this, I think there is another issue. When a hugetlb page is migrated, we do not migrate the 'page_huge_active' state of the page. That should be moved as the page is migrated. Correct?
On Mon, Feb 11, 2019 at 03:06:27PM -0800, Mike Kravetz wrote:
On 2/7/19 11:31 PM, Naoya Horiguchi wrote:
On Thu, Feb 07, 2019 at 09:50:30PM -0800, Mike Kravetz wrote:
On 2/7/19 6:31 PM, Naoya Horiguchi wrote:
On Thu, Feb 07, 2019 at 10:50:55AM -0800, Mike Kravetz wrote:
On 1/30/19 1:14 PM, Mike Kravetz wrote:
+++ b/fs/hugetlbfs/inode.c @@ -859,6 +859,16 @@ static int hugetlbfs_migrate_page(struct address_space *mapping, rc = migrate_huge_page_move_mapping(mapping, newpage, page); if (rc != MIGRATEPAGE_SUCCESS) return rc;
- /*
* page_private is subpool pointer in hugetlb pages, transfer* if needed.*/- if (page_private(page) && !page_private(newpage)) {
set_page_private(newpage, page_private(page));set_page_private(page, 0);You don't have to copy PagePrivate flag?
Well my original thought was no. For hugetlb pages, PagePrivate is not associated with page_private. It indicates a reservation was consumed. It is set when a hugetlb page is newly allocated and the allocation is associated with a reservation and the global reservation count is decremented. When the page is added to the page cache or rmap, PagePrivate is cleared. If the page is free'ed before being added to page cache or rmap, PagePrivate tells free_huge_page to restore (increment) the reserve count as we did not 'instantiate' the page.
So, PagePrivate is only set from the time a huge page is allocated until it is added to page cache or rmap. My original thought was that the page could not be migrated during this time. However, I am not sure if that reasoning is correct. The page is not locked, so it would appear that it could be migrated? But, if it can be migrated at this time then perhaps there are bigger issues for the (hugetlb) page fault code?
In my understanding, free hugetlb pages are not expected to be passed to migrate_pages(), and currently that's ensured by each migration caller which checks and avoids free hugetlb pages on its own. migrate_pages() and its internal code are probably not aware of handling free hugetlb pages, so if they are accidentally passed to migration code, that's a big problem as you are concerned. So the above reasoning should work at least this assumption is correct.
Most of migration callers are not intersted in moving free hugepages. The one I'm not sure of is the code path from alloc_contig_range(). If someone think it's worthwhile to migrate free hugepage to get bigger contiguous memory, he/she tries to enable that code path and the assumption will be broken.
You are correct. We do not migrate free huge pages. I was thinking more about problems if we migrate a page while it is being added to a task's page table as in hugetlb_no_page.
Commit bcc54222309c ("mm: hugetlb: introduce page_huge_active") addresses this issue, but I believe there is a bug in the implementation. isolate_huge_page contains this test:
if (!page_huge_active(page) || !get_page_unless_zero(page)) { ret = false; goto unlock; }
If the condition is not met, then the huge page can be isolated and migrated.
In hugetlb_no_page, there is this block of code:
page = alloc_huge_page(vma, haddr, 0); if (IS_ERR(page)) { ret = vmf_error(PTR_ERR(page)); goto out; } clear_huge_page(page, address, pages_per_huge_page(h)); __SetPageUptodate(page); set_page_huge_active(page); if (vma->vm_flags & VM_MAYSHARE) { int err = huge_add_to_page_cache(page, mapping, idx); if (err) { put_page(page); if (err == -EEXIST) goto retry; goto out; } } else { lock_page(page); if (unlikely(anon_vma_prepare(vma))) { ret = VM_FAULT_OOM; goto backout_unlocked; } anon_rmap = 1; } } else {Note that we call set_page_huge_active BEFORE locking the page. This means that we can isolate the page and have migration take place while we continue to add the page to page tables. I was able to make this happen by adding a udelay() after set_page_huge_active to simulate worst case scheduling behavior. It resulted in VM_BUG_ON while unlocking page. My test had several threads faulting in huge pages. Another thread was offlining the memory blocks forcing migration.
This shows another problem, so I agree we need a fix.
To fix this, we need to delay the set_page_huge_active call until after the page is locked. I am testing a patch with this change. Perhaps we should even delay calling set_page_huge_active until we know there are no errors and we know the page is actually in page tables?
Yes, calling set_page_huge_active after page table is set up sounds nice to me.
While looking at this, I think there is another issue. When a hugetlb page is migrated, we do not migrate the 'page_huge_active' state of the page. That should be moved as the page is migrated. Correct?
Yes, and I think that putback_active_hugepage(new_hpage) at the last step of migration sequence handles the copying of 'page_huge_active' state.
Thanks, Naoya Horiguchi
On 2/11/19 6:24 PM, Naoya Horiguchi wrote:
On Mon, Feb 11, 2019 at 03:06:27PM -0800, Mike Kravetz wrote:
While looking at this, I think there is another issue. When a hugetlb page is migrated, we do not migrate the 'page_huge_active' state of the page. That should be moved as the page is migrated. Correct?
Yes, and I think that putback_active_hugepage(new_hpage) at the last step of migration sequence handles the copying of 'page_huge_active' state.
Thanks! I missed the putback_active_hugepage that takes care of making the target migration page active.
hugetlb pages should only be migrated if they are 'active'. The routines set/clear_page_huge_active() modify the active state of hugetlb pages. When a new hugetlb page is allocated at fault time, set_page_huge_active is called before the page is locked. Therefore, another thread could race and migrate the page while it is being added to page table by the fault code. This race is somewhat hard to trigger, but can be seen by strategically adding udelay to simulate worst case scheduling behavior. Depending on 'how' the code races, various BUG()s could be triggered.
To address this issue, simply delay the set_page_huge_active call until after the page is successfully added to the page table.
Hugetlb pages can also be leaked at migration time if the pages are associated with a file in an explicitly mounted hugetlbfs filesystem. For example, a test program which hole punches, faults and migrates pages in such a file (1G in size) will eventually fail because it can not allocate a page. Reported counts and usage at time of failure:
node0 537 free_hugepages 1024 nr_hugepages 0 surplus_hugepages node1 1000 free_hugepages 1024 nr_hugepages 0 surplus_hugepages
Filesystem Size Used Avail Use% Mounted on nodev 4.0G 4.0G 0 100% /var/opt/hugepool
Note that the filesystem shows 4G of pages used, while actual usage is 511 pages (just under 1G). Failed trying to allocate page 512.
If a hugetlb page is associated with an explicitly mounted filesystem, this information in contained in the page_private field. At migration time, this information is not preserved. To fix, simply transfer page_private from old to new page at migration time if necessary.
Cc: stable@vger.kernel.org Fixes: bcc54222309c ("mm: hugetlb: introduce page_huge_active") Signed-off-by: Mike Kravetz mike.kravetz@oracle.com --- fs/hugetlbfs/inode.c | 12 ++++++++++++ mm/hugetlb.c | 9 ++++++--- 2 files changed, 18 insertions(+), 3 deletions(-)
diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c index 32920a10100e..a7fa037b876b 100644 --- a/fs/hugetlbfs/inode.c +++ b/fs/hugetlbfs/inode.c @@ -859,6 +859,18 @@ static int hugetlbfs_migrate_page(struct address_space *mapping, rc = migrate_huge_page_move_mapping(mapping, newpage, page); if (rc != MIGRATEPAGE_SUCCESS) return rc; + + /* + * page_private is subpool pointer in hugetlb pages. Transfer to + * new page. PagePrivate is not associated with page_private for + * hugetlb pages and can not be set here as only page_huge_active + * pages can be migrated. + */ + if (page_private(page)) { + set_page_private(newpage, page_private(page)); + set_page_private(page, 0); + } + if (mode != MIGRATE_SYNC_NO_COPY) migrate_page_copy(newpage, page); else diff --git a/mm/hugetlb.c b/mm/hugetlb.c index a80832487981..f859e319e3eb 100644 --- a/mm/hugetlb.c +++ b/mm/hugetlb.c @@ -3625,7 +3625,6 @@ static vm_fault_t hugetlb_cow(struct mm_struct *mm, struct vm_area_struct *vma, copy_user_huge_page(new_page, old_page, address, vma, pages_per_huge_page(h)); __SetPageUptodate(new_page); - set_page_huge_active(new_page);
mmun_start = haddr; mmun_end = mmun_start + huge_page_size(h); @@ -3647,6 +3646,7 @@ static vm_fault_t hugetlb_cow(struct mm_struct *mm, struct vm_area_struct *vma, make_huge_pte(vma, new_page, 1)); page_remove_rmap(old_page, true); hugepage_add_new_anon_rmap(new_page, vma, haddr); + set_page_huge_active(new_page); /* Make the old page be freed below */ new_page = old_page; } @@ -3792,7 +3792,6 @@ static vm_fault_t hugetlb_no_page(struct mm_struct *mm, } clear_huge_page(page, address, pages_per_huge_page(h)); __SetPageUptodate(page); - set_page_huge_active(page);
if (vma->vm_flags & VM_MAYSHARE) { int err = huge_add_to_page_cache(page, mapping, idx); @@ -3863,6 +3862,10 @@ static vm_fault_t hugetlb_no_page(struct mm_struct *mm, }
spin_unlock(ptl); + + /* May already be set if not newly allocated page */ + set_page_huge_active(page); + unlock_page(page); out: return ret; @@ -4097,7 +4100,6 @@ int hugetlb_mcopy_atomic_pte(struct mm_struct *dst_mm, * the set_pte_at() write. */ __SetPageUptodate(page); - set_page_huge_active(page);
mapping = dst_vma->vm_file->f_mapping; idx = vma_hugecache_offset(h, dst_vma, dst_addr); @@ -4165,6 +4167,7 @@ int hugetlb_mcopy_atomic_pte(struct mm_struct *dst_mm, update_mmu_cache(dst_vma, dst_addr, dst_pte);
spin_unlock(ptl); + set_page_huge_active(page); if (vm_shared) unlock_page(page); ret = 0;
On 2/12/19 2:14 PM, Mike Kravetz wrote:
Hugetlb pages can also be leaked at migration time if the pages are associated with a file in an explicitly mounted hugetlbfs filesystem. For example, a test program which hole punches, faults and migrates pages in such a file (1G in size) will eventually fail because it can not allocate a page. Reported counts and usage at time of failure:
node0 537 free_hugepages 1024 nr_hugepages 0 surplus_hugepages node1 1000 free_hugepages 1024 nr_hugepages 0 surplus_hugepages
Filesystem Size Used Avail Use% Mounted on nodev 4.0G 4.0G 0 100% /var/opt/hugepool
Note that the filesystem shows 4G of pages used, while actual usage is 511 pages (just under 1G). Failed trying to allocate page 512.
My apologies. The test scenario described above does not trigger the page leak issue fixed with this patch. It actually triggers another undiagnosed and unfixed issue with huge page migration that I will be working on. Sigh!
The leak with migration of huge pages in explicitly mounted filesystem is still fixed by this patch. However, the commit message should be changed to more accurately reflect testing and observed outcomes. The patch with only commit message changes is below:
From: Mike Kravetz mike.kravetz@oracle.com Date: Tue, 12 Feb 2019 10:58:28 -0800 Subject: [PATCH] huegtlbfs: fix races and page leaks during migration
hugetlb pages should only be migrated if they are 'active'. The routines set/clear_page_huge_active() modify the active state of hugetlb pages. When a new hugetlb page is allocated at fault time, set_page_huge_active is called before the page is locked. Therefore, another thread could race and migrate the page while it is being added to page table by the fault code. This race is somewhat hard to trigger, but can be seen by strategically adding udelay to simulate worst case scheduling behavior. Depending on 'how' the code races, various BUG()s could be triggered.
To address this issue, simply delay the set_page_huge_active call until after the page is successfully added to the page table.
Hugetlb pages can also be leaked at migration time if the pages are associated with a file in an explicitly mounted hugetlbfs filesystem. For example, consider a two node system with 4GB worth of huge pages available. A program mmaps a 2G file in a hugetlbfs filesystem. It then migrates the pages associated with the file from one node to another. When the program exits, huge page counts are as follows:
node0 1024 free_hugepages 1024 nr_hugepages
node1 0 free_hugepages 1024 nr_hugepages
Filesystem Size Used Avail Use% Mounted on nodev 4.0G 2.0G 2.0G 50% /var/opt/hugepool
That is as expected. 2G of huge pages are taken from the free_hugepages counts, and 2G is the size of the file in the explicitly mounted filesystem. If the file is then removed, the counts become:
node0 1024 free_hugepages 1024 nr_hugepages
node1 1024 free_hugepages 1024 nr_hugepages
Filesystem Size Used Avail Use% Mounted on nodev 4.0G 2.0G 2.0G 50% /var/opt/hugepool
Note that the filesystem still shows 2G of pages used, while there actually are no huge pages in use. The only way to 'fix' the filesystem accounting is to unmount the filesystem
If a hugetlb page is associated with an explicitly mounted filesystem, this information in contained in the page_private field. At migration time, this information is not preserved. To fix, simply transfer page_private from old to new page at migration time if necessary.
Cc: stable@vger.kernel.org Fixes: bcc54222309c ("mm: hugetlb: introduce page_huge_active") Signed-off-by: Mike Kravetz mike.kravetz@oracle.com --- fs/hugetlbfs/inode.c | 12 ++++++++++++ mm/hugetlb.c | 9 ++++++--- 2 files changed, 18 insertions(+), 3 deletions(-)
diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c index 32920a10100e..a7fa037b876b 100644 --- a/fs/hugetlbfs/inode.c +++ b/fs/hugetlbfs/inode.c @@ -859,6 +859,18 @@ static int hugetlbfs_migrate_page(struct address_space *mapping, rc = migrate_huge_page_move_mapping(mapping, newpage, page); if (rc != MIGRATEPAGE_SUCCESS) return rc; + + /* + * page_private is subpool pointer in hugetlb pages. Transfer to + * new page. PagePrivate is not associated with page_private for + * hugetlb pages and can not be set here as only page_huge_active + * pages can be migrated. + */ + if (page_private(page)) { + set_page_private(newpage, page_private(page)); + set_page_private(page, 0); + } + if (mode != MIGRATE_SYNC_NO_COPY) migrate_page_copy(newpage, page); else diff --git a/mm/hugetlb.c b/mm/hugetlb.c index a80832487981..f859e319e3eb 100644 --- a/mm/hugetlb.c +++ b/mm/hugetlb.c @@ -3625,7 +3625,6 @@ static vm_fault_t hugetlb_cow(struct mm_struct *mm, struct vm_area_struct *vma, copy_user_huge_page(new_page, old_page, address, vma, pages_per_huge_page(h)); __SetPageUptodate(new_page); - set_page_huge_active(new_page);
mmun_start = haddr; mmun_end = mmun_start + huge_page_size(h); @@ -3647,6 +3646,7 @@ static vm_fault_t hugetlb_cow(struct mm_struct *mm, struct vm_area_struct *vma, make_huge_pte(vma, new_page, 1)); page_remove_rmap(old_page, true); hugepage_add_new_anon_rmap(new_page, vma, haddr); + set_page_huge_active(new_page); /* Make the old page be freed below */ new_page = old_page; } @@ -3792,7 +3792,6 @@ static vm_fault_t hugetlb_no_page(struct mm_struct *mm, } clear_huge_page(page, address, pages_per_huge_page(h)); __SetPageUptodate(page); - set_page_huge_active(page);
if (vma->vm_flags & VM_MAYSHARE) { int err = huge_add_to_page_cache(page, mapping, idx); @@ -3863,6 +3862,10 @@ static vm_fault_t hugetlb_no_page(struct mm_struct *mm, }
spin_unlock(ptl); + + /* May already be set if not newly allocated page */ + set_page_huge_active(page); + unlock_page(page); out: return ret; @@ -4097,7 +4100,6 @@ int hugetlb_mcopy_atomic_pte(struct mm_struct *dst_mm, * the set_pte_at() write. */ __SetPageUptodate(page); - set_page_huge_active(page);
mapping = dst_vma->vm_file->f_mapping; idx = vma_hugecache_offset(h, dst_vma, dst_addr); @@ -4165,6 +4167,7 @@ int hugetlb_mcopy_atomic_pte(struct mm_struct *dst_mm, update_mmu_cache(dst_vma, dst_addr, dst_pte);
spin_unlock(ptl); + set_page_huge_active(page); if (vm_shared) unlock_page(page); ret = 0;
On Tue, 12 Feb 2019 14:14:00 -0800 Mike Kravetz mike.kravetz@oracle.com wrote:
hugetlb pages should only be migrated if they are 'active'. The routines set/clear_page_huge_active() modify the active state of hugetlb pages. When a new hugetlb page is allocated at fault time, set_page_huge_active is called before the page is locked. Therefore, another thread could race and migrate the page while it is being added to page table by the fault code. This race is somewhat hard to trigger, but can be seen by strategically adding udelay to simulate worst case scheduling behavior. Depending on 'how' the code races, various BUG()s could be triggered.
To address this issue, simply delay the set_page_huge_active call until after the page is successfully added to the page table.
Hugetlb pages can also be leaked at migration time if the pages are associated with a file in an explicitly mounted hugetlbfs filesystem. For example, a test program which hole punches, faults and migrates pages in such a file (1G in size) will eventually fail because it can not allocate a page. Reported counts and usage at time of failure:
node0 537 free_hugepages 1024 nr_hugepages 0 surplus_hugepages node1 1000 free_hugepages 1024 nr_hugepages 0 surplus_hugepages
Filesystem Size Used Avail Use% Mounted on nodev 4.0G 4.0G 0 100% /var/opt/hugepool
Note that the filesystem shows 4G of pages used, while actual usage is 511 pages (just under 1G). Failed trying to allocate page 512.
If a hugetlb page is associated with an explicitly mounted filesystem, this information in contained in the page_private field. At migration time, this information is not preserved. To fix, simply transfer page_private from old to new page at migration time if necessary.
Cc: stable@vger.kernel.org Fixes: bcc54222309c ("mm: hugetlb: introduce page_huge_active") Signed-off-by: Mike Kravetz mike.kravetz@oracle.com
cc:stable. It would be nice to get some review of this one, please?
--- a/fs/hugetlbfs/inode.c +++ b/fs/hugetlbfs/inode.c @@ -859,6 +859,18 @@ static int hugetlbfs_migrate_page(struct address_space *mapping, rc = migrate_huge_page_move_mapping(mapping, newpage, page); if (rc != MIGRATEPAGE_SUCCESS) return rc;
- /*
* page_private is subpool pointer in hugetlb pages. Transfer to* new page. PagePrivate is not associated with page_private for* hugetlb pages and can not be set here as only page_huge_active* pages can be migrated.*/- if (page_private(page)) {
set_page_private(newpage, page_private(page));set_page_private(page, 0);- }
- if (mode != MIGRATE_SYNC_NO_COPY) migrate_page_copy(newpage, page); else
diff --git a/mm/hugetlb.c b/mm/hugetlb.c index a80832487981..f859e319e3eb 100644 --- a/mm/hugetlb.c +++ b/mm/hugetlb.c @@ -3625,7 +3625,6 @@ static vm_fault_t hugetlb_cow(struct mm_struct *mm, struct vm_area_struct *vma, copy_user_huge_page(new_page, old_page, address, vma, pages_per_huge_page(h)); __SetPageUptodate(new_page);
- set_page_huge_active(new_page);
mmun_start = haddr; mmun_end = mmun_start + huge_page_size(h); @@ -3647,6 +3646,7 @@ static vm_fault_t hugetlb_cow(struct mm_struct *mm, struct vm_area_struct *vma, make_huge_pte(vma, new_page, 1)); page_remove_rmap(old_page, true); hugepage_add_new_anon_rmap(new_page, vma, haddr);
set_page_huge_active(new_page);@@ -3792,7 +3792,6 @@ static vm_fault_t hugetlb_no_page(struct mm_struct *mm, } clear_huge_page(page, address, pages_per_huge_page(h)); __SetPageUptodate(page);
set_page_huge_active(page);if (vma->vm_flags & VM_MAYSHARE) { int err = huge_add_to_page_cache(page, mapping, idx); @@ -3863,6 +3862,10 @@ static vm_fault_t hugetlb_no_page(struct mm_struct *mm, } spin_unlock(ptl);
- /* May already be set if not newly allocated page */
- set_page_huge_active(page);
- unlock_page(page);
out: return ret; @@ -4097,7 +4100,6 @@ int hugetlb_mcopy_atomic_pte(struct mm_struct *dst_mm, * the set_pte_at() write. */ __SetPageUptodate(page);
- set_page_huge_active(page);
mapping = dst_vma->vm_file->f_mapping; idx = vma_hugecache_offset(h, dst_vma, dst_addr); @@ -4165,6 +4167,7 @@ int hugetlb_mcopy_atomic_pte(struct mm_struct *dst_mm, update_mmu_cache(dst_vma, dst_addr, dst_pte); spin_unlock(ptl);
- set_page_huge_active(page); if (vm_shared) unlock_page(page); ret = 0;
-- 2.17.2
On 2/20/19 10:09 PM, Andrew Morton wrote:
On Tue, 12 Feb 2019 14:14:00 -0800 Mike Kravetz mike.kravetz@oracle.com wrote:
cc:stable. It would be nice to get some review of this one, please?
diff --git a/mm/hugetlb.c b/mm/hugetlb.c index a80832487981..f859e319e3eb 100644 --- a/mm/hugetlb.c +++ b/mm/hugetlb.c @@ -3625,7 +3625,6 @@ static vm_fault_t hugetlb_cow(struct mm_struct *mm, struct vm_area_struct *vma, copy_user_huge_page(new_page, old_page, address, vma, pages_per_huge_page(h)); __SetPageUptodate(new_page);
- set_page_huge_active(new_page);
mmun_start = haddr; mmun_end = mmun_start + huge_page_size(h); @@ -3647,6 +3646,7 @@ static vm_fault_t hugetlb_cow(struct mm_struct *mm, struct vm_area_struct *vma, make_huge_pte(vma, new_page, 1)); page_remove_rmap(old_page, true); hugepage_add_new_anon_rmap(new_page, vma, haddr);
set_page_huge_active(new_page);@@ -3792,7 +3792,6 @@ static vm_fault_t hugetlb_no_page(struct mm_struct *mm, } clear_huge_page(page, address, pages_per_huge_page(h)); __SetPageUptodate(page);
set_page_huge_active(page);if (vma->vm_flags & VM_MAYSHARE) { int err = huge_add_to_page_cache(page, mapping, idx); @@ -3863,6 +3862,10 @@ static vm_fault_t hugetlb_no_page(struct mm_struct *mm, } spin_unlock(ptl);
- /* May already be set if not newly allocated page */
- set_page_huge_active(page);
This is wrong. We need to only set_page_huge_active() for newly allocated pages. Why? We could have got the page from the pagecache, and it could be that the page is !page_huge_active() because it has been isolated for migration. Therefore, we do not want to set it active here.
I have also found another race with migration when removing a page from a file. When a huge page is removed from the pagecache, the page_mapping() field is cleared yet page_private continues to point to the subpool until the page is actually freed by free_huge_page(). free_huge_page is what adjusts the counts for the subpool. A page could be migrated while in this state. However, since page_mapping() is not set the hugetlbfs specific routine to transfer page_private is not called and we leak the page count in the filesystem. To fix, check for this condition before migrating a huge page. If the condition is detected, return EBUSY for the page.
Both issues are addressed in the updated patch below.
Sorry for the churn. As I find and fix one issue I seem to discover another. There is still at least one more issue with private pages when COW comes into play. I continue to work that. I wanted to send this patch earlier as it is pretty easy to hit the bugs if you try. If you would prefer another approach, let me know.
From: Mike Kravetz mike.kravetz@oracle.com Date: Thu, 21 Feb 2019 11:01:04 -0800 Subject: [PATCH] huegtlbfs: fix races and page leaks during migration
hugetlb pages should only be migrated if they are 'active'. The routines set/clear_page_huge_active() modify the active state of hugetlb pages. When a new hugetlb page is allocated at fault time, set_page_huge_active is called before the page is locked. Therefore, another thread could race and migrate the page while it is being added to page table by the fault code. This race is somewhat hard to trigger, but can be seen by strategically adding udelay to simulate worst case scheduling behavior. Depending on 'how' the code races, various BUG()s could be triggered.
To address this issue, simply delay the set_page_huge_active call until after the page is successfully added to the page table.
Hugetlb pages can also be leaked at migration time if the pages are associated with a file in an explicitly mounted hugetlbfs filesystem. For example, consider a two node system with 4GB worth of huge pages available. A program mmaps a 2G file in a hugetlbfs filesystem. It then migrates the pages associated with the file from one node to another. When the program exits, huge page counts are as follows:
node0 1024 free_hugepages 1024 nr_hugepages
node1 0 free_hugepages 1024 nr_hugepages
Filesystem Size Used Avail Use% Mounted on nodev 4.0G 2.0G 2.0G 50% /var/opt/hugepool
That is as expected. 2G of huge pages are taken from the free_hugepages counts, and 2G is the size of the file in the explicitly mounted filesystem. If the file is then removed, the counts become:
node0 1024 free_hugepages 1024 nr_hugepages
node1 1024 free_hugepages 1024 nr_hugepages
Filesystem Size Used Avail Use% Mounted on nodev 4.0G 2.0G 2.0G 50% /var/opt/hugepool
Note that the filesystem still shows 2G of pages used, while there actually are no huge pages in use. The only way to 'fix' the filesystem accounting is to unmount the filesystem
If a hugetlb page is associated with an explicitly mounted filesystem, this information in contained in the page_private field. At migration time, this information is not preserved. To fix, simply transfer page_private from old to new page at migration time if necessary.
There is a related race with removing a huge page from a file migration. When a huge page is removed from the pagecache, the page_mapping() field is cleared yet page_private remains set until the page is actually freed by free_huge_page(). A page could be migrated while in this state. However, since page_mapping() is not set the hugetlbfs specific routine to transfer page_private is not called and we leak the page count in the filesystem. To fix, check for this condition before migrating a huge page. If the condition is detected, return EBUSY for the page.
Cc: stable@vger.kernel.org Fixes: bcc54222309c ("mm: hugetlb: introduce page_huge_active") Signed-off-by: Mike Kravetz mike.kravetz@oracle.com --- fs/hugetlbfs/inode.c | 12 ++++++++++++ mm/hugetlb.c | 12 +++++++++--- mm/migrate.c | 11 +++++++++++ 3 files changed, 32 insertions(+), 3 deletions(-)
diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c index 32920a10100e..a7fa037b876b 100644 --- a/fs/hugetlbfs/inode.c +++ b/fs/hugetlbfs/inode.c @@ -859,6 +859,18 @@ static int hugetlbfs_migrate_page(struct address_space *mapping, rc = migrate_huge_page_move_mapping(mapping, newpage, page); if (rc != MIGRATEPAGE_SUCCESS) return rc; + + /* + * page_private is subpool pointer in hugetlb pages. Transfer to + * new page. PagePrivate is not associated with page_private for + * hugetlb pages and can not be set here as only page_huge_active + * pages can be migrated. + */ + if (page_private(page)) { + set_page_private(newpage, page_private(page)); + set_page_private(page, 0); + } + if (mode != MIGRATE_SYNC_NO_COPY) migrate_page_copy(newpage, page); else diff --git a/mm/hugetlb.c b/mm/hugetlb.c index a80832487981..e9c92e925b7e 100644 --- a/mm/hugetlb.c +++ b/mm/hugetlb.c @@ -3625,7 +3625,6 @@ static vm_fault_t hugetlb_cow(struct mm_struct *mm, struct vm_area_struct *vma, copy_user_huge_page(new_page, old_page, address, vma, pages_per_huge_page(h)); __SetPageUptodate(new_page); - set_page_huge_active(new_page);
mmun_start = haddr; mmun_end = mmun_start + huge_page_size(h); @@ -3647,6 +3646,7 @@ static vm_fault_t hugetlb_cow(struct mm_struct *mm, struct vm_area_struct *vma, make_huge_pte(vma, new_page, 1)); page_remove_rmap(old_page, true); hugepage_add_new_anon_rmap(new_page, vma, haddr); + set_page_huge_active(new_page); /* Make the old page be freed below */ new_page = old_page; } @@ -3731,6 +3731,7 @@ static vm_fault_t hugetlb_no_page(struct mm_struct *mm, pte_t new_pte; spinlock_t *ptl; unsigned long haddr = address & huge_page_mask(h); + bool new_page = false;
/* * Currently, we are forced to kill the process in the event the @@ -3792,7 +3793,7 @@ static vm_fault_t hugetlb_no_page(struct mm_struct *mm, } clear_huge_page(page, address, pages_per_huge_page(h)); __SetPageUptodate(page); - set_page_huge_active(page); + new_page = true;
if (vma->vm_flags & VM_MAYSHARE) { int err = huge_add_to_page_cache(page, mapping, idx); @@ -3863,6 +3864,11 @@ static vm_fault_t hugetlb_no_page(struct mm_struct *mm, }
spin_unlock(ptl); + + /* Make newly allocated pages active */ + if (new_page) + set_page_huge_active(page); + unlock_page(page); out: return ret; @@ -4097,7 +4103,6 @@ int hugetlb_mcopy_atomic_pte(struct mm_struct *dst_mm, * the set_pte_at() write. */ __SetPageUptodate(page); - set_page_huge_active(page);
mapping = dst_vma->vm_file->f_mapping; idx = vma_hugecache_offset(h, dst_vma, dst_addr); @@ -4165,6 +4170,7 @@ int hugetlb_mcopy_atomic_pte(struct mm_struct *dst_mm, update_mmu_cache(dst_vma, dst_addr, dst_pte);
spin_unlock(ptl); + set_page_huge_active(page); if (vm_shared) unlock_page(page); ret = 0; diff --git a/mm/migrate.c b/mm/migrate.c index f7e4bfdc13b7..23d91146052b 100644 --- a/mm/migrate.c +++ b/mm/migrate.c @@ -1290,6 +1290,16 @@ static int unmap_and_move_huge_page(new_page_t get_new_page, lock_page(hpage); }
+ /* + * Check for pages which are in the process of being freed. Without + * page_mapping() set, hugetlbfs specific move page routine will not + * be called and we could leak usage counts for subpools. + */ + if (page_private(hpage) && !page_mapping(hpage)) { + rc = -EBUSY; + goto out_unlock; + } + if (PageAnon(hpage)) anon_vma = page_get_anon_vma(hpage);
@@ -1320,6 +1330,7 @@ static int unmap_and_move_huge_page(new_page_t get_new_page, put_new_page = NULL; }
+out_unlock: unlock_page(hpage); out: if (rc != -EAGAIN)
On Thu, 21 Feb 2019 11:11:06 -0800 Mike Kravetz mike.kravetz@oracle.com wrote:
Sorry for the churn. As I find and fix one issue I seem to discover another. There is still at least one more issue with private pages when COW comes into play. I continue to work that. I wanted to send this patch earlier as it is pretty easy to hit the bugs if you try. If you would prefer another approach, let me know.
No probs, the bug doesn't seem to be causing a lot of bother out there and it's cc:stable; there's time to get this right ;)
Here's the delta I queued:
--- a/mm/hugetlb.c~huegtlbfs-fix-races-and-page-leaks-during-migration-update +++ a/mm/hugetlb.c @@ -3729,6 +3729,7 @@ static vm_fault_t hugetlb_no_page(struct pte_t new_pte; spinlock_t *ptl; unsigned long haddr = address & huge_page_mask(h); + bool new_page = false;
/* * Currently, we are forced to kill the process in the event the @@ -3790,6 +3791,7 @@ retry: } clear_huge_page(page, address, pages_per_huge_page(h)); __SetPageUptodate(page); + new_page = true;
if (vma->vm_flags & VM_MAYSHARE) { int err = huge_add_to_page_cache(page, mapping, idx); @@ -3861,8 +3863,9 @@ retry:
spin_unlock(ptl);
- /* May already be set if not newly allocated page */ - set_page_huge_active(page); + /* Make newly allocated pages active */ + if (new_page) + set_page_huge_active(page);
unlock_page(page); out: --- a/mm/migrate.c~huegtlbfs-fix-races-and-page-leaks-during-migration-update +++ a/mm/migrate.c @@ -1315,6 +1315,16 @@ static int unmap_and_move_huge_page(new_ lock_page(hpage); }
+ /* + * Check for pages which are in the process of being freed. Without + * page_mapping() set, hugetlbfs specific move page routine will not + * be called and we could leak usage counts for subpools. + */ + if (page_private(hpage) && !page_mapping(hpage)) { + rc = -EBUSY; + goto out_unlock; + } + if (PageAnon(hpage)) anon_vma = page_get_anon_vma(hpage);
@@ -1345,6 +1355,7 @@ put_anon: put_new_page = NULL; }
+out_unlock: unlock_page(hpage); out: if (rc != -EAGAIN) _
Hi Mike,
On Thu, Feb 21, 2019 at 11:11:06AM -0800, Mike Kravetz wrote:
On 2/20/19 10:09 PM, Andrew Morton wrote:
On Tue, 12 Feb 2019 14:14:00 -0800 Mike Kravetz mike.kravetz@oracle.com wrote:
diff --git a/mm/hugetlb.c b/mm/hugetlb.c index a80832487981..f859e319e3eb 100644 --- a/mm/hugetlb.c +++ b/mm/hugetlb.c
...
@@ -3863,6 +3862,10 @@ static vm_fault_t hugetlb_no_page(struct mm_struct *mm, } spin_unlock(ptl);
- /* May already be set if not newly allocated page */
- set_page_huge_active(page);
This is wrong. We need to only set_page_huge_active() for newly allocated pages. Why? We could have got the page from the pagecache, and it could be that the page is !page_huge_active() because it has been isolated for migration. Therefore, we do not want to set it active here.
I have also found another race with migration when removing a page from a file. When a huge page is removed from the pagecache, the page_mapping() field is cleared yet page_private continues to point to the subpool until the page is actually freed by free_huge_page(). free_huge_page is what adjusts the counts for the subpool. A page could be migrated while in this state. However, since page_mapping() is not set the hugetlbfs specific routine to transfer page_private is not called and we leak the page count in the filesystem. To fix, check for this condition before migrating a huge page. If the condition is detected, return EBUSY for the page.
Both issues are addressed in the updated patch below.
Sorry for the churn. As I find and fix one issue I seem to discover another. There is still at least one more issue with private pages when COW comes into play. I continue to work that. I wanted to send this patch earlier as it is pretty easy to hit the bugs if you try. If you would prefer another approach, let me know.
From: Mike Kravetz mike.kravetz@oracle.com Date: Thu, 21 Feb 2019 11:01:04 -0800 Subject: [PATCH] huegtlbfs: fix races and page leaks during migration
Subject still contains a typo.
hugetlb pages should only be migrated if they are 'active'. The routines set/clear_page_huge_active() modify the active state of hugetlb pages. When a new hugetlb page is allocated at fault time, set_page_huge_active is called before the page is locked. Therefore, another thread could race and migrate the page while it is being added to page table by the fault code. This race is somewhat hard to trigger, but can be seen by strategically adding udelay to simulate worst case scheduling behavior. Depending on 'how' the code races, various BUG()s could be triggered.
To address this issue, simply delay the set_page_huge_active call until after the page is successfully added to the page table.
Hugetlb pages can also be leaked at migration time if the pages are associated with a file in an explicitly mounted hugetlbfs filesystem. For example, consider a two node system with 4GB worth of huge pages available. A program mmaps a 2G file in a hugetlbfs filesystem. It then migrates the pages associated with the file from one node to another. When the program exits, huge page counts are as follows:
node0 1024 free_hugepages 1024 nr_hugepages
node1 0 free_hugepages 1024 nr_hugepages
Filesystem Size Used Avail Use% Mounted on nodev 4.0G 2.0G 2.0G 50% /var/opt/hugepool
That is as expected. 2G of huge pages are taken from the free_hugepages counts, and 2G is the size of the file in the explicitly mounted filesystem. If the file is then removed, the counts become:
node0 1024 free_hugepages 1024 nr_hugepages
node1 1024 free_hugepages 1024 nr_hugepages
Filesystem Size Used Avail Use% Mounted on nodev 4.0G 2.0G 2.0G 50% /var/opt/hugepool
Note that the filesystem still shows 2G of pages used, while there actually are no huge pages in use. The only way to 'fix' the filesystem accounting is to unmount the filesystem
If a hugetlb page is associated with an explicitly mounted filesystem, this information in contained in the page_private field. At migration time, this information is not preserved. To fix, simply transfer page_private from old to new page at migration time if necessary.
There is a related race with removing a huge page from a file migration. When a huge page is removed from the pagecache, the page_mapping() field is cleared yet page_private remains set until the page is actually freed by free_huge_page(). A page could be migrated while in this state. However, since page_mapping() is not set the hugetlbfs specific routine to transfer page_private is not called and we leak the page count in the filesystem. To fix, check for this condition before migrating a huge page. If the condition is detected, return EBUSY for the page.
Cc: stable@vger.kernel.org Fixes: bcc54222309c ("mm: hugetlb: introduce page_huge_active") Signed-off-by: Mike Kravetz mike.kravetz@oracle.com
fs/hugetlbfs/inode.c | 12 ++++++++++++ mm/hugetlb.c | 12 +++++++++--- mm/migrate.c | 11 +++++++++++ 3 files changed, 32 insertions(+), 3 deletions(-)
diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c index 32920a10100e..a7fa037b876b 100644 --- a/fs/hugetlbfs/inode.c +++ b/fs/hugetlbfs/inode.c @@ -859,6 +859,18 @@ static int hugetlbfs_migrate_page(struct address_space *mapping, rc = migrate_huge_page_move_mapping(mapping, newpage, page); if (rc != MIGRATEPAGE_SUCCESS) return rc;
- /*
* page_private is subpool pointer in hugetlb pages. Transfer to* new page. PagePrivate is not associated with page_private for* hugetlb pages and can not be set here as only page_huge_active* pages can be migrated.*/- if (page_private(page)) {
set_page_private(newpage, page_private(page));set_page_private(page, 0);- }
- if (mode != MIGRATE_SYNC_NO_COPY) migrate_page_copy(newpage, page); else
diff --git a/mm/hugetlb.c b/mm/hugetlb.c index a80832487981..e9c92e925b7e 100644 --- a/mm/hugetlb.c +++ b/mm/hugetlb.c
...
@@ -3863,6 +3864,11 @@ static vm_fault_t hugetlb_no_page(struct mm_struct *mm, }
spin_unlock(ptl);
- /* Make newly allocated pages active */
You already have a perfect explanation about why we need this "if",
... We could have got the page from the pagecache, and it could be that the page is !page_huge_active() because it has been isolated for migration.
so you could improve this comment with it.
Anyway, I agree to what/how you try to fix.
Reviewed-by: Naoya Horiguchi n-horiguchi@ah.jp.nec.com
Thanks, Naoya Horiguchi
On 2/25/19 11:44 PM, Naoya Horiguchi wrote:
Hi Mike,
On Thu, Feb 21, 2019 at 11:11:06AM -0800, Mike Kravetz wrote:
...
From: Mike Kravetz mike.kravetz@oracle.com Date: Thu, 21 Feb 2019 11:01:04 -0800 Subject: [PATCH] huegtlbfs: fix races and page leaks during migration
Subject still contains a typo.
Yes
--- a/mm/hugetlb.c +++ b/mm/hugetlb.c
...
@@ -3863,6 +3864,11 @@ static vm_fault_t hugetlb_no_page(struct mm_struct *mm, }
spin_unlock(ptl);
- /* Make newly allocated pages active */
You already have a perfect explanation about why we need this "if",
... We could have got the page from the pagecache, and it could be that the page is !page_huge_active() because it has been isolated for migration.
so you could improve this comment with it.
You are correct, the explanation in the commit message should be in the comment.
Anyway, I agree to what/how you try to fix.
Reviewed-by: Naoya Horiguchi n-horiguchi@ah.jp.nec.com
Thank you for reviewing!
Andrew, I am not sure if this helps but I have updated the patch and included below. Changes are: - Rebased on v5.0-rc6, so some context is different. - Fixed subject typo and improved comment as suggested by Naoya - Reformatted a couple paragraphs in commit message that had too long lines If you prefer something else, let me know.
From: Mike Kravetz mike.kravetz@oracle.com Date: Tue, 26 Feb 2019 14:19:36 -0800 Subject: [PATCH] hugetlbfs: fix races and page leaks during migration
hugetlb pages should only be migrated if they are 'active'. The routines set/clear_page_huge_active() modify the active state of hugetlb pages. When a new hugetlb page is allocated at fault time, set_page_huge_active is called before the page is locked. Therefore, another thread could race and migrate the page while it is being added to page table by the fault code. This race is somewhat hard to trigger, but can be seen by strategically adding udelay to simulate worst case scheduling behavior. Depending on 'how' the code races, various BUG()s could be triggered.
To address this issue, simply delay the set_page_huge_active call until after the page is successfully added to the page table.
Hugetlb pages can also be leaked at migration time if the pages are associated with a file in an explicitly mounted hugetlbfs filesystem. For example, consider a two node system with 4GB worth of huge pages available. A program mmaps a 2G file in a hugetlbfs filesystem. It then migrates the pages associated with the file from one node to another. When the program exits, huge page counts are as follows:
node0 1024 free_hugepages 1024 nr_hugepages
node1 0 free_hugepages 1024 nr_hugepages
Filesystem Size Used Avail Use% Mounted on nodev 4.0G 2.0G 2.0G 50% /var/opt/hugepool
That is as expected. 2G of huge pages are taken from the free_hugepages counts, and 2G is the size of the file in the explicitly mounted filesystem. If the file is then removed, the counts become:
node0 1024 free_hugepages 1024 nr_hugepages
node1 1024 free_hugepages 1024 nr_hugepages
Filesystem Size Used Avail Use% Mounted on nodev 4.0G 2.0G 2.0G 50% /var/opt/hugepool
Note that the filesystem still shows 2G of pages used, while there actually are no huge pages in use. The only way to 'fix' the filesystem accounting is to unmount the filesystem
If a hugetlb page is associated with an explicitly mounted filesystem, this information in contained in the page_private field. At migration time, this information is not preserved. To fix, simply transfer page_private from old to new page at migration time if necessary.
There is a related race with removing a huge page from a file and migration. When a huge page is removed from the pagecache, the page_mapping() field is cleared, yet page_private remains set until the page is actually freed by free_huge_page(). A page could be migrated while in this state. However, since page_mapping() is not set the hugetlbfs specific routine to transfer page_private is not called and we leak the page count in the filesystem. To fix, check for this condition before migrating a huge page. If the condition is detected, return EBUSY for the page.
Cc: stable@vger.kernel.org Fixes: bcc54222309c ("mm: hugetlb: introduce page_huge_active") Signed-off-by: Mike Kravetz mike.kravetz@oracle.com Reviewed-by: Naoya Horiguchi n-horiguchi@ah.jp.nec.com --- fs/hugetlbfs/inode.c | 12 ++++++++++++ mm/hugetlb.c | 16 +++++++++++++--- mm/migrate.c | 11 +++++++++++ 3 files changed, 36 insertions(+), 3 deletions(-)
diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c index 32920a10100e..a7fa037b876b 100644 --- a/fs/hugetlbfs/inode.c +++ b/fs/hugetlbfs/inode.c @@ -859,6 +859,18 @@ static int hugetlbfs_migrate_page(struct address_space *mapping, rc = migrate_huge_page_move_mapping(mapping, newpage, page); if (rc != MIGRATEPAGE_SUCCESS) return rc; + + /* + * page_private is subpool pointer in hugetlb pages. Transfer to + * new page. PagePrivate is not associated with page_private for + * hugetlb pages and can not be set here as only page_huge_active + * pages can be migrated. + */ + if (page_private(page)) { + set_page_private(newpage, page_private(page)); + set_page_private(page, 0); + } + if (mode != MIGRATE_SYNC_NO_COPY) migrate_page_copy(newpage, page); else diff --git a/mm/hugetlb.c b/mm/hugetlb.c index afef61656c1e..8dfdffc34a99 100644 --- a/mm/hugetlb.c +++ b/mm/hugetlb.c @@ -3624,7 +3624,6 @@ static vm_fault_t hugetlb_cow(struct mm_struct *mm, struct vm_area_struct *vma, copy_user_huge_page(new_page, old_page, address, vma, pages_per_huge_page(h)); __SetPageUptodate(new_page); - set_page_huge_active(new_page);
mmu_notifier_range_init(&range, mm, haddr, haddr + huge_page_size(h)); mmu_notifier_invalidate_range_start(&range); @@ -3645,6 +3644,7 @@ static vm_fault_t hugetlb_cow(struct mm_struct *mm, struct vm_area_struct *vma, make_huge_pte(vma, new_page, 1)); page_remove_rmap(old_page, true); hugepage_add_new_anon_rmap(new_page, vma, haddr); + set_page_huge_active(new_page); /* Make the old page be freed below */ new_page = old_page; } @@ -3729,6 +3729,7 @@ static vm_fault_t hugetlb_no_page(struct mm_struct *mm, pte_t new_pte; spinlock_t *ptl; unsigned long haddr = address & huge_page_mask(h); + bool new_page = false;
/* * Currently, we are forced to kill the process in the event the @@ -3790,7 +3791,7 @@ static vm_fault_t hugetlb_no_page(struct mm_struct *mm, } clear_huge_page(page, address, pages_per_huge_page(h)); __SetPageUptodate(page); - set_page_huge_active(page); + new_page = true;
if (vma->vm_flags & VM_MAYSHARE) { int err = huge_add_to_page_cache(page, mapping, idx); @@ -3861,6 +3862,15 @@ static vm_fault_t hugetlb_no_page(struct mm_struct *mm, }
spin_unlock(ptl); + + /* + * Only make newly allocated pages active. Existing pages found + * in the pagecache could be !page_huge_active() if they have been + * isolated for migration. + */ + if (new_page) + set_page_huge_active(page); + unlock_page(page); out: return ret; @@ -4095,7 +4105,6 @@ int hugetlb_mcopy_atomic_pte(struct mm_struct *dst_mm, * the set_pte_at() write. */ __SetPageUptodate(page); - set_page_huge_active(page);
mapping = dst_vma->vm_file->f_mapping; idx = vma_hugecache_offset(h, dst_vma, dst_addr); @@ -4163,6 +4172,7 @@ int hugetlb_mcopy_atomic_pte(struct mm_struct *dst_mm, update_mmu_cache(dst_vma, dst_addr, dst_pte);
spin_unlock(ptl); + set_page_huge_active(page); if (vm_shared) unlock_page(page); ret = 0; diff --git a/mm/migrate.c b/mm/migrate.c index d4fd680be3b0..181f5d2718a9 100644 --- a/mm/migrate.c +++ b/mm/migrate.c @@ -1315,6 +1315,16 @@ static int unmap_and_move_huge_page(new_page_t get_new_page, lock_page(hpage); }
+ /* + * Check for pages which are in the process of being freed. Without + * page_mapping() set, hugetlbfs specific move page routine will not + * be called and we could leak usage counts for subpools. + */ + if (page_private(hpage) && !page_mapping(hpage)) { + rc = -EBUSY; + goto out_unlock; + } + if (PageAnon(hpage)) anon_vma = page_get_anon_vma(hpage);
@@ -1345,6 +1355,7 @@ static int unmap_and_move_huge_page(new_page_t get_new_page, put_new_page = NULL; }
+out_unlock: unlock_page(hpage); out: if (rc != -EAGAIN)
linux-stable-mirror@lists.linaro.org
-
Andrew Morton -
Mike Kravetz -
Naoya Horiguchi