mm/hugetlb: Small cleanup and improvement

Just like its neighbour is_hugetlb_entry_migration() has done.

Signed-off-by Baoquan He <[email protected]>

Reviewed-by Mike Kravetz <[email protected]>

Reviewed-by David Hildenbrand <[email protected]>

---
 mm/hugetlb.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

mm/hugetlb.c +2 -2

index f24acb3af741..3569e731e66b 100644
@@ -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;

The 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.

Signed-off-by Baoquan He <[email protected]>

Reviewed-by Mike Kravetz <[email protected]>

Reviewed-by David Hildenbrand <[email protected]>

---
 mm/hugetlb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

mm/hugetlb.c +1 -1

index 3569e731e66b..c14837854392 100644
@@ -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;

Quoted 3 lines · Baoquan He

The 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.

They are no stricter as such but implicitly contains non_swap_entry() in itself. If a swap entry tests positive for either is_[migration|hwpoison]_entry(), then its swap_type() is among SWP_MIGRATION_READ, SWP_MIGRATION_WRITE and SWP_HWPOISON. All these types >= MAX_SWAPFILES, exactly what is asserted with non_swap_entry().

Quoted 29 lines

Hence remove the unnecessary non_swap_entry() in is_hugetlb_entry_migration() and is_hugetlb_entry_hwpoisoned() to simplify code.

Signed-off-by Baoquan He <[email protected]>

Reviewed-by Mike Kravetz <[email protected]>

Reviewed-by David Hildenbrand <[email protected]>

---
 mm/hugetlb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

mm/hugetlb.c +1 -2

index 3569e731e66b..c14837854392 100644
@@ -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))

mm/hugetlb.c static bool is_hugetlb_entry_hwpoisoned(pte_t pte) · line 3767

Quoted 5 lines

mm/hugetlb.c +1 -0

+	if (is_hwpoison_entry(swp))
 		return true;
 	else
 		return false;

It would be better if the commit message contains details about the existing redundant check. But either way.

Quoted 41 lines · Anshuman Khandual

Quoted 3 lines · Baoquan He

The 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.

They are no stricter as such but implicitly contains non_swap_entry() in itself. If a swap entry tests positive for either is_[migration|hwpoison]_entry(), then its swap_type() is among SWP_MIGRATION_READ, SWP_MIGRATION_WRITE and SWP_HWPOISON. All these types >= MAX_SWAPFILES, exactly what is asserted with non_swap_entry().

Quoted 29 lines

Hence remove the unnecessary non_swap_entry() in is_hugetlb_entry_migration() and is_hugetlb_entry_hwpoisoned() to simplify code.

Signed-off-by Baoquan He <[email protected]>

Reviewed-by Mike Kravetz <[email protected]>

Reviewed-by David Hildenbrand <[email protected]>

---
 mm/hugetlb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

mm/hugetlb.c +1 -2

index 3569e731e66b..c14837854392 100644
@@ -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))

Quoted 8 lines

mm/hugetlb.c static bool is_hugetlb_entry_hwpoisoned(pte_t pte) · line 3767

Quoted 5 lines

mm/hugetlb.c +1 -0

+	if (is_hwpoison_entry(swp))
 		return true;
 	else
 		return false;

It would be better if the commit message contains details about the existing redundant check. But either way.

Thanks for your advice. Do you think updating the log as below is OK?

~~~~~~~~
If a swap entry tests positive for either is_[migration|hwpoison]_entry(), then its swap_type() is among SWP_MIGRATION_READ, SWP_MIGRATION_WRITE and SWP_HWPOISON. All these types >= MAX_SWAPFILES, exactly what is asserted with non_swap_entry().

So the checking non_swap_entry() in is_hugetlb_entry_migration() and is_hugetlb_entry_hwpoisoned() is redundant.

Let's remove it to optimize code.
~~~~~~~~

Change 'pecify' to 'Specify'.

Signed-off-by Baoquan He <[email protected]>

Reviewed-by Mike Kravetz <[email protected]>

Reviewed-by David Hildenbrand <[email protected]>

---
 Documentation/admin-guide/mm/hugetlbpage.rst | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Documentation/admin-guide/mm/hugetlbpage.rst +0 -0

index 015a5f7d7854..f7b1c7462991 100644

Quoted 15 lines · Baoquan He

Change 'pecify' to 'Specify'.

Signed-off-by Baoquan He <[email protected]>

Reviewed-by Mike Kravetz <[email protected]>

Reviewed-by David Hildenbrand <[email protected]>

---
 Documentation/admin-guide/mm/hugetlbpage.rst | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Documentation/admin-guide/mm/hugetlbpage.rst +0 -0

index 015a5f7d7854..f7b1c7462991 100644

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

This does not apply on 5.8-rc6 and the original typo seems to be missing there as well. This section was introduced recently with following commit.

282f4214384e ("hugetlbfs: clean up command line processing")

A customer complained that no message is logged wh en 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 8 lines · Baoquan He

A customer complained that no message is logged wh en 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.

Quoted 3 lines

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.

But is kernel expected to warn for all such situations where the user requested resources could not be allocated completely ? Otherwise, it does not make sense to add an warning for just one such situation.

Quoted 15 lines · Anshuman Khandual

Quoted 11 lines · Baoquan He

A customer complained that no message is logged wh en 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 3 lines

But is kernel expected to warn for all such situations where the user requested resources could not be allocated completely ? Otherwise, it does not make sense to add an warning for just one such situation.

It's not for just one such situation, we have already had one to warn out in mm/hugetlb.c, please check hugetlb_hstate_alloc_pages().

As Mike said, in one time of persistent huge page number setting, comparing the old value with the new vlaue is good enough for customer to get the information. However, if customer want to detect and analyze previous setting failure, logging message will be helpful. So I think logging the failure or partial success makes sense.

Quoted 16 lines · Baoquan He

Quoted 15 lines · Anshuman Khandual

Quoted 11 lines · Baoquan He

A customer complained that no message is logged wh en 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 6 lines

Quoted 3 lines

But is kernel expected to warn for all such situations where the user requested resources could not be allocated completely ? Otherwise, it does not make sense to add an warning for just one such situation.

It's not for just one such situation, we have already had one to warn out in mm/hugetlb.c, please check hugetlb_hstate_alloc_pages().

Those are a little different in that they are warnings based on kernel command line parameters.

Quoted 5 lines

As Mike said, in one time of persistent huge page number setting, comparing the old value with the new vlaue is good enough for customer to get the information. However, if customer want to detect and analyze previous setting failure, logging message will be helpful. So I think logging the failure or partial success makes sense.

I can understand the argument against adding a new warning for this. You could even argue that this condition has existed since the time hugetlb was added to the kernel which was long ago. And, nobody has complained enough to add a warning. I have even heard of a sysadmin practice of asking for a ridiculously large amount of hugetlb pages just so that the kernel will allocate as many as possible. They do not 'expect' to get the ridiculous amount they asked for. In such cases, this will be a new warning in their log.

As mentioned in a previous e-mail, when one sets nr_hugepages by writing to the sysfs or proc file, one needs to read the file to determine if the number of requested pages were actually allocated. Anyone who does not do this is just asking for trouble. Yet, I imagine that it may happen.

To be honest, I do not see this log message as something that would be helpful to end users. Rather, I could see this as being useful to support people. Support always asks for system logs and this could point out a possible issue with hugetlb usage.

I do not feel strongly one way or another about adding the warning. Since it is fairly trivial and could help diagnose issues I am in favor of adding it. If people feel strongly that it should not be added, I am open to those arguments.

On 7/23/20 2:11 AM, Baoquan He wrote:

Quoted 6 lines

Quoted 3 lines

But is kernel expected to warn for all such situations where the user requested resources could not be allocated completely ? Otherwise, it does not make sense to add an warning for just one such situation.

It's not for just one such situation, we have already had one to warn out in mm/hugetlb.c, please check hugetlb_hstate_alloc_pages().

Those are a little different in that they are warnings based on kernel command line parameters.

Quoted 5 lines

As Mike said, in one time of persistent huge page number setting, comparing the old value with the new vlaue is good enough for customer to get the information. However, if customer want to detect and analyze previous setting failure, logging message will be helpful. So I think logging the failure or partial success makes sense.

I can understand the argument against adding a new warning for this. You could even argue that this condition has existed since the time hugetlb was added to the kernel which was long ago. And, nobody has complained enough to add a warning. I have even heard of a sysadmin practice of asking for a ridiculously large amount of hugetlb pages just so that the kernel will allocate as many as possible. They do not 'expect' to get the ridiculous amount they asked for. In such cases, this will be a new warning in their log.

As mentioned in a previous e-mail, when one sets nr_hugepages by writing to the sysfs or proc file, one needs to read the file to determine if the number of requested pages were actually allocated. Anyone who does not do this is just asking for trouble. Yet, I imagine that it may happen.

To be honest, I do not see this log message as something that would be helpful to end users. Rather, I could see this as being useful to support people. Support always asks for system logs and this could point out a possible issue with hugetlb usage.

I do not feel strongly one way or another about adding the warning. Since it is fairly trivial and could help diagnose issues I am in favor of adding it. If people feel strongly that it should not be added, I am open to those arguments.

Hi Mike,

Quoted 1 line · Mike Kravetz

On 7/23/20 2:11 AM, Baoquan He wrote:

...

Quoted 35 lines

Quoted 6 lines

Quoted 3 lines

But is kernel expected to warn for all such situations where the user requested resources could not be allocated completely ? Otherwise, it does not make sense to add an warning for just one such situation.

It's not for just one such situation, we have already had one to warn out in mm/hugetlb.c, please check hugetlb_hstate_alloc_pages().

Those are a little different in that they are warnings based on kernel command line parameters.

Quoted 5 lines

As Mike said, in one time of persistent huge page number setting, comparing the old value with the new vlaue is good enough for customer to get the information. However, if customer want to detect and analyze previous setting failure, logging message will be helpful. So I think logging the failure or partial success makes sense.

I can understand the argument against adding a new warning for this. You could even argue that this condition has existed since the time hugetlb was added to the kernel which was long ago. And, nobody has complained enough to add a warning. I have even heard of a sysadmin practice of asking for a ridiculously large amount of hugetlb pages just so that the kernel will allocate as many as possible. They do not 'expect' to get the ridiculous amount they asked for. In such cases, this will be a new warning in their log.

As mentioned in a previous e-mail, when one sets nr_hugepages by writing to the sysfs or proc file, one needs to read the file to determine if the number of requested pages were actually allocated. Anyone who does not do this is just asking for trouble. Yet, I imagine that it may happen.

To be honest, I do not see this log message as something that would be helpful to end users. Rather, I could see this as being useful to support people. Support always asks for system logs and this could point out a possible issue with hugetlb usage.

Quoted 4 lines

I do not feel strongly one way or another about adding the warning. Since it is fairly trivial and could help diagnose issues I am in favor of adding it. If people feel strongly that it should not be added, I am open to those arguments.

Ping!

It's been a while, seems no objection to log the message. Do you consider accepting this patch or offering an Ack?

Thanks
Baoquan

Cc Michal

Quoted 47 lines · Baoquan He

Hi Mike,

Quoted 1 line · Mike Kravetz

On 7/23/20 2:11 AM, Baoquan He wrote:

...

Quoted 35 lines

Quoted 6 lines

Quoted 3 lines

But is kernel expected to warn for all such situations where the user requested resources could not be allocated completely ? Otherwise, it does not make sense to add an warning for just one such situation.

It's not for just one such situation, we have already had one to warn out in mm/hugetlb.c, please check hugetlb_hstate_alloc_pages().

Those are a little different in that they are warnings based on kernel command line parameters.

Quoted 5 lines

As Mike said, in one time of persistent huge page number setting, comparing the old value with the new vlaue is good enough for customer to get the information. However, if customer want to detect and analyze previous setting failure, logging message will be helpful. So I think logging the failure or partial success makes sense.

I can understand the argument against adding a new warning for this. You could even argue that this condition has existed since the time hugetlb was added to the kernel which was long ago. And, nobody has complained enough to add a warning. I have even heard of a sysadmin practice of asking for a ridiculously large amount of hugetlb pages just so that the kernel will allocate as many as possible. They do not 'expect' to get the ridiculous amount they asked for. In such cases, this will be a new warning in their log.

As mentioned in a previous e-mail, when one sets nr_hugepages by writing to the sysfs or proc file, one needs to read the file to determine if the number of requested pages were actually allocated. Anyone who does not do this is just asking for trouble. Yet, I imagine that it may happen.

To be honest, I do not see this log message as something that would be helpful to end users. Rather, I could see this as being useful to support people. Support always asks for system logs and this could point out a possible issue with hugetlb usage.

Quoted 4 lines

I do not feel strongly one way or another about adding the warning. Since it is fairly trivial and could help diagnose issues I am in favor of adding it. If people feel strongly that it should not be added, I am open to those arguments.

Ping!

Quoted 5 lines

It's been a while, seems no objection to log the message. Do you consider accepting this patch or offering an Ack?

Thanks
Baoquan

Adding Michal as he has had opinions about hugetlbfs log messages in the past.

My opinion is that the warning is too late to add at this stage. It would have been much better if the user interface has provided a reasonable feedback on how much the request was sucessful. But this is not the case (except for few error cases) and we have to live with the interface where the caller has to read the value after writing to it. Lame but a reality.

I have heard about people making an opportunistic attempt to grab as many hugetlb pages as possible and they do expect the failure and scale the request size down. I do not think those would appreciate warnings.

That being said I would rather keep the existing behavior even though it is suboptimal. It is just trivial to add the check in the userspace without risking complains by other users. Besides the warning is not really telling us much more than a subsequent read anyway. You are not going to learn why the allocation has failed because that one is done (intentionaly) as __GFP_NOWARN.

v1 is here:
https://lore.kernel.org/linux-mm/[email protected]/

Patch 1~3 are small clean up.

Patch 4 is adding warning message when the number of persistent huge pages is not changed to the exact value written to the sysfs or proc nr_hugepages file.

v1->v2:
Drop the old patch 1/5 in v1 post, which was thought as typo, while actually another kind of abbreviation.

Updated patch log of patch 4 which is rephrased by Mike. And move the added message logging code after the hugetlb_lock dropping, this is suggested by Mike.

Baoquan He (4):
mm/hugetlb.c: make is_hugetlb_entry_hwpoisoned return bool mm/hugetlb.c: Remove the unnecessary non_swap_entry() doc/vm: fix typo 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                                 | 27 +++++++++++++++-----
 2 files changed, 21 insertions(+), 8 deletions(-)