remove unnecessary condition in remove_inode_hugepages

At present, we need to call hugetlb_fix_reserve_count when hugetlb_unrserve_pages fails, and PagePrivate will decide hugetlb reserves counts.

we obtain the page from page cache. and use page both lock_page and mutex_lock. alloc_huge_page add page to page chace always hold lock page, then bail out clearpageprivate before unlock page.

but I' m not sure it is right or I miss the points.

fs/hugetlbfs/inode.c +1 -3

index 4ea71eb..010723b 100644
@@ -462,14 +462,12 @@ static void remove_inode_hugepages(struct inode *inode, loff_t lstart,
                         * the page, note PagePrivate which is used in case
                         * of error.
                         */
-                       rsv_on_error = !PagePrivate(page);
                        remove_huge_page(page);
                        freed++;
                        if (!truncate_op) {
                                if (unlikely(hugetlb_unreserve_pages(inode,
                                                        next, next + 1, 1)))
-                                       hugetlb_fix_reserve_counts(inode,
-                                                               rsv_on_error);
+                                       hugetlb_fix_reserve_counts(inode)
                        }
 
                        unlock_page(page);

include/linux/hugetlb.h +1 -1

index c26d463..d2e0fc5 100644
@@ -90,7 +90,7 @@ int dequeue_hwpoisoned_huge_page(struct page *page);
 bool isolate_huge_page(struct page *page, struct list_head *list);
 void putback_active_hugepage(struct page *page);
 void free_huge_page(struct page *page);
-void hugetlb_fix_reserve_counts(struct inode *inode, bool restore_reserve);
+void hugetlb_fix_reserve_counts(struct inode *inode);
 extern struct mutex *hugetlb_fault_mutex_table;
 u32 hugetlb_fault_mutex_hash(struct hstate *h, struct mm_struct *mm,
                                struct vm_area_struct *vma,

mm/hugetlb.c +2 -3

index 87e11d8..28a079a 100644
@@ -567,13 +567,13 @@ retry:
  * appear as a "reserved" entry instead of simply dangling with incorrect
  * counts.
  */
-void hugetlb_fix_reserve_counts(struct inode *inode, bool restore_reserve)
+void hugetlb_fix_reserve_counts(struct inode *inode)
 {
        struct hugepage_subpool *spool = subpool_inode(inode);
        long rsv_adjust;
 
        rsv_adjust = hugepage_subpool_get_pages(spool, 1);
-       if (restore_reserve && rsv_adjust) {
+       if (rsv_adjust) {
                struct hstate *h = hstate_inode(inode);
 
                hugetlb_acct_memory(h, 1);
 
 
--

To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

At present, we need to call hugetlb_fix_reserve_count when hugetlb_unrserve_pages fails, and PagePrivate will decide hugetlb reserves counts.

we obtain the page from page cache. and use page both lock_page and mutex_lock. alloc_huge_page add page to page chace always hold lock page, then bail out clearpageprivate before unlock page.

but I' m not sure it is right or I miss the points.

fs/hugetlbfs/inode.c +1 -2

-                                       hugetlb_fix_reserve_counts(inode,
-                                                               rsv_on_error);
+                                       hugetlb_fix_reserve_counts(inode)
                        }
 
                        unlock_page(page);

include/linux/hugetlb.h +1 -1

index c26d463..d2e0fc5 100644
@@ -90,7 +90,7 @@ int dequeue_hwpoisoned_huge_page(struct page *page);
 bool isolate_huge_page(struct page *page, struct list_head *list);
 void putback_active_hugepage(struct page *page);
 void free_huge_page(struct page *page);
-void hugetlb_fix_reserve_counts(struct inode *inode, bool restore_reserve);
+void hugetlb_fix_reserve_counts(struct inode *inode);
 extern struct mutex *hugetlb_fault_mutex_table;
 u32 hugetlb_fault_mutex_hash(struct hstate *h, struct mm_struct *mm,
                                struct vm_area_struct *vma,

mm/hugetlb.c +2 -2

index 87e11d8..28a079a 100644
@@ -567,13 +567,13 @@ retry:
  * appear as a "reserved" entry instead of simply dangling with incorrect
  * counts.
  */
-void hugetlb_fix_reserve_counts(struct inode *inode, bool restore_reserve)
+void hugetlb_fix_reserve_counts(struct inode *inode)
 {
        struct hugepage_subpool *spool = subpool_inode(inode);
        long rsv_adjust;
 
        rsv_adjust = hugepage_subpool_get_pages(spool, 1);
-       if (restore_reserve && rsv_adjust) {
+       if (rsv_adjust) {
                struct hstate *h = hstate_inode(inode);
 
                hugetlb_acct_memory(h, 1);

Quoted 8 lines · zhong jiang

At present, we need to call hugetlb_fix_reserve_count when hugetlb_unrserve_pages fails, and PagePrivate will decide hugetlb reserves counts.

we obtain the page from page cache. and use page both lock_page and mutex_lock. alloc_huge_page add page to page chace always hold lock page, then bail out clearpageprivate before unlock page.

but I' m not sure it is right or I miss the points.

Let me try to explain the code you suggest is unnecessary.

The PagePrivate flag is used in huge page allocation/deallocation to indicate that the page was globally reserved. For example, in dequeue_huge_page_vma() there is this code:

                        if (page) {
                                if (avoid_reserve)
                                        break;
                                if (!vma_has_reserves(vma, chg))
                                        break;

                                SetPagePrivate(page);
                                h->resv_huge_pages--;
                                break;
                        }

and in free_huge_page():

        restore_reserve = PagePrivate(page);
        ClearPagePrivate(page);
	.
	<snip>
	.
        if (restore_reserve)
                h->resv_huge_pages++;

This helps maintains the global huge page reserve count.

In addition to the global reserve count, there are per VMA reservation structures. Unfortunately, these structures have different meanings depending on the context in which they are used.

If there is a VMA reservation entry for a page, and the page has not been instantiated in the VMA this indicates there is a huge page reserved and the global resv_huge_pages count reflects that reservation. Even if a page was not reserved, a VMA reservation entry is added when a page is instantiated in the VMA.

With that background, let's look at the existing code/proposed changes.

fs/hugetlbfs/inode.c static void remove_inode_hugepages(struct inode *inode, loff_t lstart, · line 465

Quoted 9 lines

fs/hugetlbfs/inode.c +0 -1

index 4ea71eb..010723b 100644
@@ -462,14 +462,12 @@ static void remove_inode_hugepages(struct inode *inode, loff_t lstart,
                         * the page, note PagePrivate which is used in case
                         * of error.
                         */
-                       rsv_on_error = !PagePrivate(page);

This rsv_on_error flag indicates that when the huge page was allocated,

it was NOT counted against the global reserve count. So, when remove_huge_page eventually calls free_huge_page(), the global count resv_huge_pages is not incremented. So far, no problem.

fs/hugetlbfs/inode.c static void remove_inode_hugepages(struct inode *inode, loff_t lstart, · line 469

Quoted 5 lines

fs/hugetlbfs/inode.c +0 -0

                        remove_huge_page(page);
                        freed++;
                        if (!truncate_op) {
                                if (unlikely(hugetlb_unreserve_pages(inode,
                                                        next, next + 1, 1)))

We now have this VERY unlikely situation that hugetlb_unreserve_pages fails. This means that the VMA reservation entry for the page was not removed. So, we are in a bit of a mess. The page has already been removed, but the VMA reservation entry can not. This LOOKS like there is a reservation for the page in the VMA reservation structure. But, the global count resv_huge_pages does not reflect this reservation.

If we do nothing, when the VMA is eventually removed the VMA reservation structure will be completely removed and the global count resv_huge_pages will be decremented for each entry in the structure. Since, there is a VMA reservation entry without a corresponding global count, the global count will be one less than it should (will eventually go to -1).

To 'fix' this, hugetlb_fix_reserve_counts is called. In this case, it will increment the global count so that it is consistent with the entries in the VMA reservation structure.

This is all quite confusing and really unlikely to happen. I tried to explain in code comments:

Before removing the page:
/*
* We must free the huge page and remove from page
* cache (remove_huge_page) BEFORE removing the
* region/reserve map (hugetlb_unreserve_pages). In
* rare out of memory conditions, removal of the
* region/reserve map could fail. Before free'ing
* the page, note PagePrivate which is used in case
* of error.
*/

And, the routine hugetlb_fix_reserve_counts:
/*
* A rare out of memory error was encountered which prevented removal of
* the reserve map region for a page. The huge page itself was free'ed
* and removed from the page cache. This routine will adjust the subpool
* usage count, and the global reserve count if needed. By incrementing
* these counts, the reserve map entry which could not be deleted will
* appear as a "reserved" entry instead of simply dangling with incorrect
* counts.
*/

Quoted 42 lines · Mike Kravetz

Quoted 8 lines · zhong jiang

At present, we need to call hugetlb_fix_reserve_count when hugetlb_unrserve_pages fails, and PagePrivate will decide hugetlb reserves counts.

we obtain the page from page cache. and use page both lock_page and mutex_lock. alloc_huge_page add page to page chace always hold lock page, then bail out clearpageprivate before unlock page.

but I' m not sure it is right or I miss the points.

Let me try to explain the code you suggest is unnecessary.

The PagePrivate flag is used in huge page allocation/deallocation to indicate that the page was globally reserved. For example, in dequeue_huge_page_vma() there is this code:

                        if (page) {
                                if (avoid_reserve)
                                        break;
                                if (!vma_has_reserves(vma, chg))
                                        break;

                                SetPagePrivate(page);
                                h->resv_huge_pages--;
                                break;
                        }

and in free_huge_page():

        restore_reserve = PagePrivate(page);
        ClearPagePrivate(page);
	.
	<snip>
	.
        if (restore_reserve)
                h->resv_huge_pages++;

This helps maintains the global huge page reserve count.

In addition to the global reserve count, there are per VMA reservation structures. Unfortunately, these structures have different meanings depending on the context in which they are used.

Quoted 7 lines

If there is a VMA reservation entry for a page, and the page has not been instantiated in the VMA this indicates there is a huge page reserved and the global resv_huge_pages count reflects that reservation. Even if a page was not reserved, a VMA reservation entry is added when a page is instantiated in the VMA.

With that background, let's look at the existing code/proposed changes.

Clearly.

Quoted 10 lines

fs/hugetlbfs/inode.c static void remove_inode_hugepages(struct inode *inode, loff_t lstart, · line 465

Quoted 9 lines

fs/hugetlbfs/inode.c +0 -1

index 4ea71eb..010723b 100644
@@ -462,14 +462,12 @@ static void remove_inode_hugepages(struct inode *inode, loff_t lstart,
                         * the page, note PagePrivate which is used in case
                         * of error.
                         */
-                       rsv_on_error = !PagePrivate(page);

This rsv_on_error flag indicates that when the huge page was allocated,

yes

Quoted 3 lines

it was NOT counted against the global reserve count. So, when remove_huge_page eventually calls free_huge_page(), the global count resv_huge_pages is not incremented. So far, no problem.

but the page comes from the page cache. if it is. it should implement
ClearPageprivate(page) when lock page. This condition always true.

The key point is why it need still check the PagePrivate(page) when page from page cache and hold lock.

Thanks you
zhongjiang

Quoted 47 lines

fs/hugetlbfs/inode.c static void remove_inode_hugepages(struct inode *inode, loff_t lstart, · line 469

Quoted 5 lines

fs/hugetlbfs/inode.c +0 -0

                        remove_huge_page(page);
                        freed++;
                        if (!truncate_op) {
                                if (unlikely(hugetlb_unreserve_pages(inode,
                                                        next, next + 1, 1)))

We now have this VERY unlikely situation that hugetlb_unreserve_pages fails. This means that the VMA reservation entry for the page was not removed. So, we are in a bit of a mess. The page has already been removed, but the VMA reservation entry can not. This LOOKS like there is a reservation for the page in the VMA reservation structure. But, the global count resv_huge_pages does not reflect this reservation.

If we do nothing, when the VMA is eventually removed the VMA reservation structure will be completely removed and the global count resv_huge_pages will be decremented for each entry in the structure. Since, there is a VMA reservation entry without a corresponding global count, the global count will be one less than it should (will eventually go to -1).

To 'fix' this, hugetlb_fix_reserve_counts is called. In this case, it will increment the global count so that it is consistent with the entries in the VMA reservation structure.

This is all quite confusing and really unlikely to happen. I tried to explain in code comments:

Before removing the page:
/*
* We must free the huge page and remove from page
* cache (remove_huge_page) BEFORE removing the
* region/reserve map (hugetlb_unreserve_pages). In
* rare out of memory conditions, removal of the
* region/reserve map could fail. Before free'ing
* the page, note PagePrivate which is used in case
* of error.
*/

And, the routine hugetlb_fix_reserve_counts:
/*
* A rare out of memory error was encountered which prevented removal of
* the reserve map region for a page. The huge page itself was free'ed
* and removed from the page cache. This routine will adjust the subpool
* usage count, and the global reserve count if needed. By incrementing
* these counts, the reserve map entry which could not be deleted will
* appear as a "reserved" entry instead of simply dangling with incorrect
* counts.
*/

Quoted 62 lines · zhong jiang

Quoted 42 lines · Mike Kravetz

Quoted 8 lines · zhong jiang

At present, we need to call hugetlb_fix_reserve_count when hugetlb_unrserve_pages fails, and PagePrivate will decide hugetlb reserves counts.

we obtain the page from page cache. and use page both lock_page and mutex_lock. alloc_huge_page add page to page chace always hold lock page, then bail out clearpageprivate before unlock page.

but I' m not sure it is right or I miss the points.

Let me try to explain the code you suggest is unnecessary.

The PagePrivate flag is used in huge page allocation/deallocation to indicate that the page was globally reserved. For example, in dequeue_huge_page_vma() there is this code:

                        if (page) {
                                if (avoid_reserve)
                                        break;
                                if (!vma_has_reserves(vma, chg))
                                        break;

                                SetPagePrivate(page);
                                h->resv_huge_pages--;
                                break;
                        }

and in free_huge_page():

        restore_reserve = PagePrivate(page);
        ClearPagePrivate(page);
	.
	<snip>
	.
        if (restore_reserve)
                h->resv_huge_pages++;

This helps maintains the global huge page reserve count.

In addition to the global reserve count, there are per VMA reservation structures. Unfortunately, these structures have different meanings depending on the context in which they are used.

Quoted 7 lines

If there is a VMA reservation entry for a page, and the page has not been instantiated in the VMA this indicates there is a huge page reserved and the global resv_huge_pages count reflects that reservation. Even if a page was not reserved, a VMA reservation entry is added when a page is instantiated in the VMA.

With that background, let's look at the existing code/proposed changes.

Clearly.

Quoted 10 lines

fs/hugetlbfs/inode.c static void remove_inode_hugepages(struct inode *inode, loff_t lstart, · line 465

Quoted 9 lines

fs/hugetlbfs/inode.c +0 -1

index 4ea71eb..010723b 100644
@@ -462,14 +462,12 @@ static void remove_inode_hugepages(struct inode *inode, loff_t lstart,
                         * the page, note PagePrivate which is used in case
                         * of error.
                         */
-                       rsv_on_error = !PagePrivate(page);

This rsv_on_error flag indicates that when the huge page was allocated,

yes

Quoted 8 lines

Quoted 3 lines

it was NOT counted against the global reserve count. So, when remove_huge_page eventually calls free_huge_page(), the global count resv_huge_pages is not incremented. So far, no problem.

but the page comes from the page cache. if it is. it should implement
ClearPageprivate(page) when lock page. This condition always true.

The key point is why it need still check the PagePrivate(page) when page from page cache and hold lock.

You are correct. My apologies for not seeing your point in the original post.

When the huge page is added to the page cache (huge_add_to_page_cache), the Page Private flag will be cleared. Since this code (remove_inode_hugepages) will only be called for pages in the page cache, PagePrivate(page) will always be false.

The comments in this area should be changed along with the code.