mm/hugetlb: Small cleanup and improvement
Subsystems (4)
cover mm/hugetlb: Small cleanup and improvement
Patch 1~4 are small cleanup.
Patch 5 is to add warning message when failed to increase or decrease the expected number of persistent huge pages by writing into /proc/sys/vm/nr_hugepages /sys/kernel/mm/hugepages/hugepages-xxx/nr_hugepages.
Baoquan He (5):
mm/hugetlb.c: Fix typo of glb_reserve
mm/hugetlb.c: make is_hugetlb_entry_hwpoisoned return bool mm/hugetlb.c: Remove the unnecessary non_swap_entry() doc/vm: fix typo in in the hugetlb admin documentation mm/hugetl.c: warn out if expected count of huge pages adjustment is
not achieved
Documentation/admin-guide/mm/hugetlbpage.rst | 2 +- mm/hugetlb.c | 49 ++++++++++++-------- 2 files changed, 31 insertions(+), 20 deletions(-)
Signature · 1 line
2.17.2
PATCH 1/5 mm/hugetlb.c: Fix typo of glb_reserve
The local variable is for global reservation of region.
--- mm/hugetlb.c | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-)
mm/hugetlb.c +12 -12
@@ -3649,7 +3649,7 @@ static void hugetlb_vm_op_close(struct vm_area_struct *vma)struct resv_map *resv =vma_resv_map (vma);struct hugepage_subpool *spool =subpool_vma (vma);unsigned long reserve, start, end; -long gbl_reserve; +long glb_reserve;if (!resv || !is_vma_resv_set (vma,HPAGE_RESV_OWNER ))return ;
@@ -3664,8 +3664,8 @@ static void hugetlb_vm_op_close(struct vm_area_struct *vma)* Decrement reserve counts. The global reserve count may be * adjusted if the subpool has a minimum size. */ - gbl_reserve =hugepage_subpool_put_pages (spool, reserve); -hugetlb_acct_memory (h, -gbl_reserve); + glb_reserve =hugepage_subpool_put_pages (spool, reserve); +hugetlb_acct_memory (h, -glb_reserve); }kref_put (&resv->refs, resv_map_release);
@@ -5054,7 +5054,7 @@ int hugetlb_reserve_pages(struct inode *inode,struct hugepage_subpool *spool =subpool_inode (inode);struct resv_map *resv_map;struct hugetlb_cgroup *h_cg =NULL ; -long gbl_reserve, regions_needed =0 ; +long glb_reserve, regions_needed =0 ;/* This should never happen */ if (from > to) {
@@ -5121,10 +5121,10 @@ int hugetlb_reserve_pages(struct inode *inode,/* * There must be enough pages in the subpool for the mapping. If * the subpool has a minimum size, there may be some global -* reservations already in place (gbl_reserve). +* reservations already in place (glb_reserve). */ - gbl_reserve =hugepage_subpool_get_pages (spool, chg); -if (gbl_reserve <0 ) { + glb_reserve =hugepage_subpool_get_pages (spool, chg); +if (glb_reserve <0 ) { ret = -ENOSPC ; goto out_uncharge_cgroup; }
@@ -5133,7 +5133,7 @@ int hugetlb_reserve_pages(struct inode *inode,* Check enough hugepages are available for the reservation. * Hand the pages back to the subpool if there are not */ - ret =hugetlb_acct_memory (h, gbl_reserve); + ret =hugetlb_acct_memory (h, glb_reserve);if (ret <0 ) { goto out_put_pages; }
@@ -5153,7 +5153,7 @@ int hugetlb_reserve_pages(struct inode *inode, add =region_add (resv_map, from, to, regions_needed, h, h_cg);if (unlikely (add <0 )) { -hugetlb_acct_memory (h, -gbl_reserve); +hugetlb_acct_memory (h, -glb_reserve); goto out_put_pages; }else if (unlikely (chg > add)) {/*
@@ -5200,7 +5200,7 @@ long hugetlb_unreserve_pages(struct inode *inode, long start, long end,struct resv_map *resv_map =inode_resv_map (inode);long chg =0 ;struct hugepage_subpool *spool =subpool_inode (inode); -long gbl_reserve; +long glb_reserve;/* * Since this routine can be called in the evict inode path for all
@@ -5225,8 +5225,8 @@ long hugetlb_unreserve_pages(struct inode *inode, long start, long end,* If the subpool has a minimum size, the number of global * reservations to be released may be adjusted. */ - gbl_reserve =hugepage_subpool_put_pages (spool, (chg - freed)); -hugetlb_acct_memory (h, -gbl_reserve); + glb_reserve =hugepage_subpool_put_pages (spool, (chg - freed)); +hugetlb_acct_memory (h, -glb_reserve);return 0 ; }
Signature · 1 line
2.17.2
Re: [PATCH 1/5] mm/hugetlb.c: Fix typo of glb_reserve
Quoted 12 lines · Baoquan He
The local variable is for global reservation of region.
Signed-off-by Baoquan He <[email protected]>--- mm/hugetlb.c | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-)mm/hugetlb.c +0
-0
mm/hugetlb.c static void hugetlb_vm_op_close(struct vm_area_struct *vma) · line 3652
Quoted 5 lines
mm/hugetlb.c +1
-1@@ -3649,7 +3649,7 @@ static void hugetlb_vm_op_close(struct vm_area_struct *vma)struct resv_map *resv =vma_resv_map (vma);struct hugepage_subpool *spool =subpool_vma (vma);unsigned long reserve, start, end; -long gbl_reserve; +long glb_reserve;
I see both 'gbl' and 'glb' being used for global in variable names. grep will actually return more hits for gbl than glb. Unless there is consensus that 'glb' should be used for global, I would prefer not to make this change.
Signature · 1 line
Mike Kravetz
Re: [PATCH 1/5] mm/hugetlb.c: Fix typo of glb_reserve
Quoted 8 lines · Mike Kravetz
Quoted 7 lines · Baoquan He
The local variable is for global reservation of region.
Signed-off-by Baoquan He <[email protected]>--- mm/hugetlb.c | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-)
Quoted 14 lines
mm/hugetlb.c static void hugetlb_vm_op_close(struct vm_area_struct *vma) · line 3652
Quoted 10 lines
mm/hugetlb.c +1
-1@@ -3649,7 +3649,7 @@ static void hugetlb_vm_op_close(struct vm_area_struct *vma)struct resv_map *resv =vma_resv_map (vma);struct hugepage_subpool *spool =subpool_vma (vma);unsigned long reserve, start, end; -long gbl_reserve; +long glb_reserve;I see both 'gbl' and 'glb' being used for global in variable names. grep will actually return more hits for gbl than glb. Unless there is consensus that 'glb' should be used for global, I would prefer not to make this change.
Ah, I thought it's typo, so it's just another kind of abbreviation. Then I am fine to drop this patch.
Thanks a lot for careful reviewing on this patchset.
Thanks
Baoquan
PATCH 2/5 mm/hugetlb.c: make is_hugetlb_entry_hwpoisoned return bool
In reply to Baoquan HeJust like his neighbour is_hugetlb_entry_migration() has done.
--- mm/hugetlb.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
mm/hugetlb.c +4 -4
@@ -3754,17 +3754,17 @@ bool is_hugetlb_entry_migration(pte_t pte)return false; } -static int is_hugetlb_entry_hwpoisoned (pte_t pte) +static bool is_hugetlb_entry_hwpoisoned (pte_t pte) {swp_entry_t swp;if (huge_pte_none (pte) ||pte_present (pte)) -return 0 ; +return false; swp =pte_to_swp_entry (pte);if (non_swap_entry (swp) &&is_hwpoison_entry (swp)) -return 1 ; +return true;else -return 0 ; +return false; } int copy_hugetlb_page_range(struct mm_struct *dst, struct mm_struct *src,
Signature · 1 line
2.17.2
Re: [PATCH 2/5] mm/hugetlb.c: make is_hugetlb_entry_hwpoisoned return bool
Quoted 3 lines · Baoquan He
Just like his neighbour
is_hugetlb_entry_migration()has done.Signed-off-by Baoquan He <[email protected]>
Thanks,
Signature · 1 line
Mike Kravetz
Quoted 31 lines
--- mm/hugetlb.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)mm/hugetlb.c +4
-4@@ -3754,17 +3754,17 @@ bool is_hugetlb_entry_migration(pte_t pte)return false; } -static int is_hugetlb_entry_hwpoisoned (pte_t pte) +static bool is_hugetlb_entry_hwpoisoned (pte_t pte) {swp_entry_t swp;if (huge_pte_none (pte) ||pte_present (pte)) -return 0 ; +return false; swp =pte_to_swp_entry (pte);if (non_swap_entry (swp) &&is_hwpoison_entry (swp)) -return 1 ; +return true;else -return 0 ; +return false; } int copy_hugetlb_page_range(struct mm_struct *dst, struct mm_struct *src,
Re: [PATCH 2/5] mm/hugetlb.c: make is_hugetlb_entry_hwpoisoned return bool
In reply to Baoquan HeQuoted 24 lines · Baoquan He
Just like his neighbour
is_hugetlb_entry_migration()has done.Signed-off-by Baoquan He <[email protected]>--- mm/hugetlb.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)mm/hugetlb.c +2
-2@@ -3754,17 +3754,17 @@ bool is_hugetlb_entry_migration(pte_t pte)return false; } -static int is_hugetlb_entry_hwpoisoned (pte_t pte) +static bool is_hugetlb_entry_hwpoisoned (pte_t pte) {swp_entry_t swp;if (huge_pte_none (pte) ||pte_present (pte)) -return 0 ; +return false;
mm/hugetlb.c bool is_hugetlb_entry_migration(pte_t pte) · line 3770
Quoted 10 lines
mm/hugetlb.c +2
-2swp =pte_to_swp_entry (pte);if (non_swap_entry (swp) &&is_hwpoison_entry (swp)) -return 1 ; +return true;else -return 0 ; +return false; } int copy_hugetlb_page_range(struct mm_struct *dst, struct mm_struct *src,
Signature · 3 lines
Thanks, David / dhildenb
PATCH 3/5 mm/hugetlb.c: Remove the unnecessary non_swap_entry()
In reply to Baoquan HeThe checking is_migration_entry() and is_hwpoison_entry() are stricter than non_swap_entry(), means they have covered the conditional check which non_swap_entry() is doing.
Hence remove the unnecessary non_swap_entry() in is_hugetlb_entry_migration() and is_hugetlb_entry_hwpoisoned() to simplify code.
--- mm/hugetlb.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
mm/hugetlb.c +2 -2
@@ -3748,7 +3748,7 @@ bool is_hugetlb_entry_migration(pte_t pte)if (huge_pte_none (pte) ||pte_present (pte))return false; swp =pte_to_swp_entry (pte); -if (non_swap_entry (swp) &&is_migration_entry (swp)) +if (is_migration_entry (swp))return true;else return false;
@@ -3761,7 +3761,7 @@ static bool is_hugetlb_entry_hwpoisoned(pte_t pte)if (huge_pte_none (pte) ||pte_present (pte))return false; swp =pte_to_swp_entry (pte); -if (non_swap_entry (swp) &&is_hwpoison_entry (swp)) +if (is_hwpoison_entry (swp))return true;else return false;
Signature · 1 line
2.17.2
Re: [PATCH 3/5] mm/hugetlb.c: Remove the unnecessary non_swap_entry()
Quoted 8 lines · Baoquan He
The checking
is_migration_entry()andis_hwpoison_entry()are stricter thannon_swap_entry(), means they have covered the conditional check whichnon_swap_entry()is doing.Hence remove the unnecessary
non_swap_entry()inis_hugetlb_entry_migration()andis_hugetlb_entry_hwpoisoned()to simplify code.Signed-off-by Baoquan He <[email protected]>
Agreed, we can remove the checks for non_swap_entry.
Signature · 1 line
Mike Kravetz
Quoted 27 lines
--- mm/hugetlb.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)mm/hugetlb.c +2
-2@@ -3748,7 +3748,7 @@ bool is_hugetlb_entry_migration(pte_t pte)if (huge_pte_none (pte) ||pte_present (pte))return false; swp =pte_to_swp_entry (pte); -if (non_swap_entry (swp) &&is_migration_entry (swp)) +if (is_migration_entry (swp))return true;else return false;@@ -3761,7 +3761,7 @@ static bool is_hugetlb_entry_hwpoisoned(pte_t pte)if (huge_pte_none (pte) ||pte_present (pte))return false; swp =pte_to_swp_entry (pte); -if (non_swap_entry (swp) &&is_hwpoison_entry (swp)) +if (is_hwpoison_entry (swp))return true;else return false;
Re: [PATCH 3/5] mm/hugetlb.c: Remove the unnecessary non_swap_entry()
In reply to Baoquan HeQuoted 27 lines · Baoquan He
The checking
is_migration_entry()andis_hwpoison_entry()are stricter thannon_swap_entry(), means they have covered the conditional check whichnon_swap_entry()is doing.Hence remove the unnecessary
non_swap_entry()inis_hugetlb_entry_migration()andis_hugetlb_entry_hwpoisoned()to simplify code.Signed-off-by Baoquan He <[email protected]>--- mm/hugetlb.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)mm/hugetlb.c +1
-1@@ -3748,7 +3748,7 @@ bool is_hugetlb_entry_migration(pte_t pte)if (huge_pte_none (pte) ||pte_present (pte))return false; swp =pte_to_swp_entry (pte); -if (non_swap_entry (swp) &&is_migration_entry (swp)) +if (is_migration_entry (swp))return true;else return false;
mm/hugetlb.c static bool is_hugetlb_entry_hwpoisoned(pte_t pte) · line 3767
Quoted 8 lines
mm/hugetlb.c +1
-1@@ -3761,7 +3761,7 @@ static bool is_hugetlb_entry_hwpoisoned(pte_t pte)if (huge_pte_none (pte) ||pte_present (pte))return false; swp =pte_to_swp_entry (pte); -if (non_swap_entry (swp) &&is_hwpoison_entry (swp)) +if (is_hwpoison_entry (swp))return true;else return false;
Signature · 3 lines
Thanks, David / dhildenb
PATCH 4/5 doc/vm: fix typo in in the hugetlb admin documentation
In reply to Baoquan HeChange 'pecify' to 'Specify'.
--- Documentation/admin-guide/mm/hugetlbpage.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Documentation/admin-guide/mm/hugetlbpage.rst +1 -1
@@ -131,7 +131,7 @@ hugepages parameter is preceded by an invalid hugepagesz parameter, it will be ignored. default_hugepagesz - pecify the default huge page size. This parameter can + Specify the default huge page size. This parameter can only be specified once on the command line. default_hugepagesz can optionally be followed by the hugepages parameter to preallocate a specific number of huge pages of default size. The number of default
Signature · 1 line
2.17.2
Re: [PATCH 4/5] doc/vm: fix typo in in the hugetlb admin documentation
Quoted 13 lines · Baoquan He
Change 'pecify' to 'Specify'.
Signed-off-by Baoquan He <[email protected]>--- Documentation/admin-guide/mm/hugetlbpage.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)Documentation/admin-guide/mm/hugetlbpage.rst +0
-0
Documentation/admin-guide/mm/hugetlbpage.rst hugepages · line 137
Quoted 8 lines
Documentation/admin-guide/mm/hugetlbpage.rst +1
-1@@ -131,7 +131,7 @@ hugepages parameter is preceded by an invalid hugepagesz parameter, it will be ignored. default_hugepagesz - pecify the default huge page size. This parameter can + Specify the default huge page size. This parameter can only be specified once on the command line. default_hugepagesz can optionally be followed by the hugepages parameter to preallocate a specific number of huge pages of default size. The number of default
Unfortunately, this review comment was missed when the typo was introduced.
https://lore.kernel.org/lkml/[email protected]/
Thanks for fixing,
Signature · 1 line
Mike Kravetz
Re: [PATCH 4/5] doc/vm: fix typo in in the hugetlb admin documentation
In reply to Baoquan HeQuoted 13 lines · Baoquan He
Change 'pecify' to 'Specify'.
Signed-off-by Baoquan He <[email protected]>--- Documentation/admin-guide/mm/hugetlbpage.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)Documentation/admin-guide/mm/hugetlbpage.rst +0
-0
Documentation/admin-guide/mm/hugetlbpage.rst hugepages · line 137
Quoted 8 lines
Documentation/admin-guide/mm/hugetlbpage.rst +1
-1@@ -131,7 +131,7 @@ hugepages parameter is preceded by an invalid hugepagesz parameter, it will be ignored. default_hugepagesz - pecify the default huge page size. This parameter can + Specify the default huge page size. This parameter can only be specified once on the command line. default_hugepagesz can optionally be followed by the hugepages parameter to preallocate a specific number of huge pages of default size. The number of default
Signature · 3 lines
Thanks, David / dhildenb
PATCH 5/5 mm/hugetl.c: warn out if expected count of huge pages adjustment is not achieved
In reply to Baoquan HeA customer complained that no any message is printed out when failed to allocate explicitly specified number of persistent huge pages. That specifying can be done by writing into /proc/sys/vm/nr_hugepages to increase the persisten huge pages.
In the current code, it takes the best effort way to allocate the expected number of huge pages. If only succeeding to get part of them, no any information is printed out.
Here try to send out warning message if the expected number of huge pages adjustment is not achieved, including increasing and decreasing the count of persistent huge pages.
--- mm/hugetlb.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)
mm/hugetlb.c +12 -1
@@ -2661,7 +2661,7 @@ static int adjust_pool_surplus(struct hstate *h, nodemask_t *nodes_allowed,static int set_max_huge_pages (struct hstate *h,unsigned long count,int nid,nodemask_t *nodes_allowed) { -unsigned long min_count, ret; +unsigned long min_count, ret, old_max;NODEMASK_ALLOC (nodemask_t, node_alloc_noretry,GFP_KERNEL );/*
@@ -2723,6 +2723,7 @@ static int set_max_huge_pages(struct hstate *h, unsigned long count, int nid,* pool might be one hugepage larger than it needs to be, but * within all the constraints specified by the sysctls. */ + old_max =persistent_huge_pages (h);while (h->surplus_huge_pages && count >persistent_huge_pages (h)) {if (!adjust_pool_surplus (h, nodes_allowed,-1 ))break ;
@@ -2779,6 +2780,16 @@ static int set_max_huge_pages(struct hstate *h, unsigned long count, int nid, } out: h->max_huge_pages =persistent_huge_pages (h); +if (count != h->max_huge_pages) { +char buf[32 ]; + +string_get_size (huge_page_size (h),1 ,STRING_UNITS_2 , buf,32 ); +pr_warn ("HugeTLB: %s %lu of page size %s failed. Only %s %lu hugepages.\n" , + count > old_max ?"increasing" :"decreasing" , +abs (count - old_max), buf, + count > old_max ?"increased" :"decreased" , +abs (old_max - h->max_huge_pages)); + }spin_unlock (&hugetlb_lock);NODEMASK_FREE (node_alloc_noretry);
Signature · 1 line
2.17.2
Re: [PATCH 5/5] mm/hugetl.c: warn out if expected count of huge pages adjustment is not achieved
Quoted 5 lines · Baoquan He
A customer complained that no any message is printed out when failed to allocate explicitly specified number of persistent huge pages. That specifying can be done by writing into
/proc/sys/vm/nr_hugepagesto increase the persisten huge pages.
Quoted 7 lines
In the current code, it takes the best effort way to allocate the expected number of huge pages. If only succeeding to get part of them, no any information is printed out.
Here try to send out warning message if the expected number of huge pages adjustment is not achieved, including increasing and decreasing the count of persistent huge pages.
Perhaps change the wording a bit,
A customer complained that no message is logged when the number of persistent huge pages is not changed to the exact value written to the sysfs or proc nr_hugepages file.
In the current code, a best effort is made to satisfy requests made via the nr_hugepages file. However, requests may be only partially satisfied.
Log a message if the code was unsuccessful in fully satisfying a request. This includes both increasing and decreasing the number of persistent huge pages.
Quoted 5 lines
Signed-off-by Baoquan He <[email protected]>--- mm/hugetlb.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)
I am not opposed to this patch. However, I believe the best way for a user to determine if their request was successful is to compare the value of nr_hugepages to the value which was written.
Quoted 27 lines
mm/hugetlb.c +3
-1@@ -2661,7 +2661,7 @@ static int adjust_pool_surplus(struct hstate *h, nodemask_t *nodes_allowed,static int set_max_huge_pages (struct hstate *h,unsigned long count,int nid,nodemask_t *nodes_allowed) { -unsigned long min_count, ret; +unsigned long min_count, ret, old_max;NODEMASK_ALLOC (nodemask_t, node_alloc_noretry,GFP_KERNEL );/* @@ -2723,6 +2723,7 @@ static int set_max_huge_pages(struct hstate *h, unsigned long count, int nid,* pool might be one hugepage larger than it needs to be, but * within all the constraints specified by the sysctls. */ + old_max =persistent_huge_pages (h);while (h->surplus_huge_pages && count >persistent_huge_pages (h)) {if (!adjust_pool_surplus (h, nodes_allowed,-1 ))break ;@@ -2779,6 +2780,16 @@ static int set_max_huge_pages(struct hstate *h, unsigned long count, int nid, } out: h->max_huge_pages =persistent_huge_pages (h); +if (count != h->max_huge_pages) {
mm/hugetlb.c static int set_max_huge_pages(struct hstate *h, unsigned long count, int nid, · line 2793
Quoted 10 lines
mm/hugetlb.c +9
-0+char buf[32 ]; + +string_get_size (huge_page_size (h),1 ,STRING_UNITS_2 , buf,32 ); +pr_warn ("HugeTLB: %s %lu of page size %s failed. Only %s %lu hugepages.\n" , + count > old_max ?"increasing" :"decreasing" , +abs (count - old_max), buf, + count > old_max ?"increased" :"decreased" , +abs (old_max - h->max_huge_pages)); + }spin_unlock (&hugetlb_lock);
I would prefer if we drop the lock before logging the message. That would involve grabbing the value of h->max_huge_pages before dropping the lock.
Signature · 1 line
Mike Kravetz
Quoted 3 lines
mm/hugetlb.c +0
-0NODEMASK_FREE (node_alloc_noretry);
Re: [PATCH 5/5] mm/hugetl.c: warn out if expected count of huge pages adjustment is not achieved
Quoted 20 lines · Mike Kravetz
Quoted 5 lines · Baoquan He
A customer complained that no any message is printed out when failed to allocate explicitly specified number of persistent huge pages. That specifying can be done by writing into
/proc/sys/vm/nr_hugepagesto increase the persisten huge pages.Quoted 7 lines
In the current code, it takes the best effort way to allocate the expected number of huge pages. If only succeeding to get part of them, no any information is printed out.
Here try to send out warning message if the expected number of huge pages adjustment is not achieved, including increasing and decreasing the count of persistent huge pages.
Perhaps change the wording a bit,
A customer complained that no message is logged when the number of persistent huge pages is not changed to the exact value written to the sysfs or proc
nr_hugepagesfile.
Quoted 7 lines
In the current code, a best effort is made to satisfy requests made via the
nr_hugepagesfile. However, requests may be only partially satisfied.Log a message if the code was unsuccessful in fully satisfying a request. This includes both increasing and decreasing the number of persistent huge pages.
Thanks, sounds much better, I will use these to replace the old log.
Quoted 10 lines
Quoted 6 lines
Signed-off-by Baoquan He <[email protected]>--- mm/hugetlb.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)I am not opposed to this patch. However, I believe the best way for a user to determine if their request was successful is to compare the value of
nr_hugepagesto the value which was written.
Agree. While from our customer's request, they told the log can help 'Easily detect and analyse previous reservation failures'.
Quoted 33 lines
Quoted 33 lines
mm/hugetlb.c +8
-1@@ -2661,7 +2661,7 @@ static int adjust_pool_surplus(struct hstate *h, nodemask_t *nodes_allowed,static int set_max_huge_pages (struct hstate *h,unsigned long count,int nid,nodemask_t *nodes_allowed) { -unsigned long min_count, ret; +unsigned long min_count, ret, old_max;NODEMASK_ALLOC (nodemask_t, node_alloc_noretry,GFP_KERNEL );/* @@ -2723,6 +2723,7 @@ static int set_max_huge_pages(struct hstate *h, unsigned long count, int nid,* pool might be one hugepage larger than it needs to be, but * within all the constraints specified by the sysctls. */ + old_max =persistent_huge_pages (h);while (h->surplus_huge_pages && count >persistent_huge_pages (h)) {if (!adjust_pool_surplus (h, nodes_allowed,-1 ))break ;@@ -2779,6 +2780,16 @@ static int set_max_huge_pages(struct hstate *h, unsigned long count, int nid, } out: h->max_huge_pages =persistent_huge_pages (h); +if (count != h->max_huge_pages) { +char buf[32 ]; + +string_get_size (huge_page_size (h),1 ,STRING_UNITS_2 , buf,32 ); + pr_warn("HugeTLB: %s %lu of page size %s failed. Only %s %lu hugepages.\n", + count > old_max ? "increasing" : "decreasing",
Quoted 8 lines
mm/hugetlb.c static int set_max_huge_pages(struct hstate *h, unsigned long count, int nid, · line 2793
Quoted 5 lines
mm/hugetlb.c +4
-0+abs (count - old_max), buf, + count > old_max ?"increased" :"decreased" , + abs(old_max - h->max_huge_pages)); + }spin_unlock (&hugetlb_lock);I would prefer if we drop the lock before logging the message. That would involve grabbing the value of
h->max_huge_pagesbefore dropping the lock.
Sure, will change. We should try to release the lock's burden. Thanks.
Re: [PATCH 5/5] mm/hugetl.c: warn out if expected count of huge pages adjustment is not achieved
In reply to Mike KravetzHi Mike,
Quoted 31 lines · Mike Kravetz
Quoted 31 lines
mm/hugetlb.c +8
-1@@ -2661,7 +2661,7 @@ static int adjust_pool_surplus(struct hstate *h, nodemask_t *nodes_allowed,static int set_max_huge_pages (struct hstate *h,unsigned long count,int nid,nodemask_t *nodes_allowed) { -unsigned long min_count, ret; +unsigned long min_count, ret, old_max;NODEMASK_ALLOC (nodemask_t, node_alloc_noretry,GFP_KERNEL );/* @@ -2723,6 +2723,7 @@ static int set_max_huge_pages(struct hstate *h, unsigned long count, int nid,* pool might be one hugepage larger than it needs to be, but * within all the constraints specified by the sysctls. */ + old_max =persistent_huge_pages (h);while (h->surplus_huge_pages && count >persistent_huge_pages (h)) {if (!adjust_pool_surplus (h, nodes_allowed,-1 ))break ;@@ -2779,6 +2780,16 @@ static int set_max_huge_pages(struct hstate *h, unsigned long count, int nid, } out: h->max_huge_pages =persistent_huge_pages (h); +if (count != h->max_huge_pages) { +char buf[32 ]; + +string_get_size (huge_page_size (h),1 ,STRING_UNITS_2 , buf,32 ); + pr_warn("HugeTLB: %s %lu of page size %s failed. Only %s %lu hugepages.\n", + count > old_max ? "increasing" : "decreasing",
Quoted 8 lines
mm/hugetlb.c static int set_max_huge_pages(struct hstate *h, unsigned long count, int nid, · line 2793
Quoted 5 lines
mm/hugetlb.c +4
-0+abs (count - old_max), buf, + count > old_max ?"increased" :"decreased" , + abs(old_max - h->max_huge_pages)); + }spin_unlock (&hugetlb_lock);I would prefer if we drop the lock before logging the message. That would involve grabbing the value of
h->max_huge_pagesbefore dropping the lock.
Do you think the below change is OK to you to move the message logging after lock dropping? If yes, I will repost with updated patches.
mm/hugetlb.c +6 -4
@@ -2661,7 +2661,7 @@ static int adjust_pool_surplus(struct hstate *h, nodemask_t *nodes_allowed,static int set_max_huge_pages (struct hstate *h,unsigned long count,int nid,nodemask_t *nodes_allowed) { -unsigned long min_count, ret, old_max; +unsigned long min_count, ret, old_max, new_max;NODEMASK_ALLOC (nodemask_t, node_alloc_noretry,GFP_KERNEL );/*
@@ -2780,7 +2780,10 @@ static int set_max_huge_pages(struct hstate *h, unsigned long count, int nid, } out: h->max_huge_pages =persistent_huge_pages (h); -if (count != h->max_huge_pages) { + new_max = h->max_huge_pages; +spin_unlock (&hugetlb_lock); + +if (count != new_max) {char buf[32 ];string_get_size (huge_page_size (h),1 ,STRING_UNITS_2 , buf,32 );
@@ -2788,9 +2791,8 @@ static int set_max_huge_pages(struct hstate *h, unsigned long count, int nid, count > old_max ?"increasing" :"decreasing" ,abs (count - old_max), buf, count > old_max ?"increased" :"decreased" , - abs(old_max - h->max_huge_pages)); + abs(old_max - new_max)); } -spin_unlock (&hugetlb_lock);NODEMASK_FREE (node_alloc_noretry);
Re: [PATCH 5/5] mm/hugetl.c: warn out if expected count of huge pages adjustment is not achieved
Quoted 45 lines · Baoquan He
Quoted 6 lines · Mike Kravetz
Quoted 6 lines
(quoted diff) +6
-0+ if (count != h->max_huge_pages) { + char buf[32]; + + string_get_size(huge_page_size(h), 1, STRING_UNITS_2, buf, 32); + pr_warn("HugeTLB: %s %lu of page size %s failed. Only %s %lu hugepages.\n", + count > old_max ? "increasing" : "decreasing",Quoted 8 lines
Review
Quoted 5 lines
(quoted diff) +4
-0+ abs(count - old_max), buf, + count > old_max ? "increased" : "decreased", + abs(old_max - h->max_huge_pages)); + } spin_unlock(&hugetlb_lock);I would prefer if we drop the lock before logging the message. That would involve grabbing the value of
h->max_huge_pagesbefore dropping the lock.Do you think the below change is OK to you to move the message logging after lock dropping? If yes, I will repost with updated patches.
mm/hugetlb.c +5
-2@@ -2661,7 +2661,7 @@ static int adjust_pool_surplus(struct hstate *h, nodemask_t *nodes_allowed,static int set_max_huge_pages (struct hstate *h,unsigned long count,int nid,nodemask_t *nodes_allowed) { -unsigned long min_count, ret, old_max; +unsigned long min_count, ret, old_max, new_max;NODEMASK_ALLOC (nodemask_t, node_alloc_noretry,GFP_KERNEL );/* @@ -2780,7 +2780,10 @@ static int set_max_huge_pages(struct hstate *h, unsigned long count, int nid, } out: h->max_huge_pages =persistent_huge_pages (h); -if (count != h->max_huge_pages) { + new_max = h->max_huge_pages; +spin_unlock (&hugetlb_lock); + +if (count != new_max) {char buf[32 ];string_get_size (huge_page_size (h),1 ,STRING_UNITS_2 , buf,32 );
mm/hugetlb.c static int set_max_huge_pages(struct hstate *h, unsigned long count, int nid, · line 2797
Quoted 9 lines
mm/hugetlb.c +1
-2@@ -2788,9 +2791,8 @@ static int set_max_huge_pages(struct hstate *h, unsigned long count, int nid, count > old_max ?"increasing" :"decreasing" ,abs (count - old_max), buf, count > old_max ?"increased" :"decreased" , - abs(old_max - h->max_huge_pages)); + abs(old_max - new_max)); } -spin_unlock (&hugetlb_lock);NODEMASK_FREE (node_alloc_noretry);
Yes, that looks better. Thank you.
Signature · 1 line
Mike Kravetz