The hugetlb cgroup usage wait loops in charge_reserved_hugetlb.sh were
unbounded and could hang forever if the expected cgroup file value never
appears (e.g. due to bugs, timing issues, or unexpected behavior).

--- Error log ---
  # uname -r
  6.12.0-xxx.el10.aarch64+64k

  # ls /sys/kernel/mm/hugepages/hugepages-*
  hugepages-16777216kB/  hugepages-2048kB/  hugepages-524288kB/

  #./charge_reserved_hugetlb.sh -cgroup-v2
  # -----------------------------------------
  ...
  # nr hugepages = 10
  # writing cgroup limit: 5368709120
  # writing reseravation limit: 5368709120
  ...
  # write_to_hugetlbfs: Error mapping the file: Cannot allocate memory
  # Waiting for hugetlb memory reservation to reach size 2684354560.
  # 0
  # Waiting for hugetlb memory reservation to reach size 2684354560.
  # 0
  # Waiting for hugetlb memory reservation to reach size 2684354560.
  # 0
  # Waiting for hugetlb memory reservation to reach size 2684354560.
  # 0
  # Waiting for hugetlb memory reservation to reach size 2684354560.
  # 0
  # Waiting for hugetlb memory reservation to reach size 2684354560.
  # 0
  ...

Introduce a small helper, wait_for_file_value(), and use it for:
  - waiting for reservation usage to drop to 0,
  - waiting for reservation usage to reach a given size,
  - waiting for fault usage to reach a given size.

This makes the waits consistent and adds a hard timeout (120 tries with
0.5s sleep) so the test fails instead of stalling indefinitely.

Signed-off-by: Li Wang <liwang@redhat.com>
Cc: David Hildenbrand <david@kernel.org>
Cc: Mark Brown <broonie@kernel.org>
Cc: Shuah Khan <shuah@kernel.org>
Cc: Waiman Long <longman@redhat.com>
---
 .../selftests/mm/charge_reserved_hugetlb.sh   | 47 ++++++++++---------
 1 file changed, 26 insertions(+), 21 deletions(-)

diff --git a/tools/testing/selftests/mm/charge_reserved_hugetlb.sh b/tools/testing/selftests/mm/charge_reserved_hugetlb.sh
index e1fe16bcbbe8..249a5776c074 100755
--- a/tools/testing/selftests/mm/charge_reserved_hugetlb.sh
+++ b/tools/testing/selftests/mm/charge_reserved_hugetlb.sh
@@ -100,7 +100,7 @@ function setup_cgroup() {
   echo writing cgroup limit: "$cgroup_limit"
   echo "$cgroup_limit" >$cgroup_path/$name/hugetlb.${MB}MB.$fault_limit_file
 
-  echo writing reseravation limit: "$reservation_limit"
+  echo writing reservation limit: "$reservation_limit"
   echo "$reservation_limit" > \
     $cgroup_path/$name/hugetlb.${MB}MB.$reservation_limit_file
 
@@ -112,41 +112,46 @@ function setup_cgroup() {
   fi
 }
 
+function wait_for_file_value() {
+  local path="$1"
+  local expect="$2"
+  local max_tries="120"
+
+  local i cur
+  for ((i=1; i<=max_tries; i++)); do
+    cur="$(cat "$path")"
+    if [[ "$cur" == "$expect" ]]; then
+      return 0
+    fi
+    echo "Waiting for $path to become '$expect' (current: '$cur') (try $i/$max_tries)"
+    sleep 0.5
+  done
+
+  echo "ERROR: timeout waiting for $path to become '$expect' (last: '$cur')"
+  return 1
+}
+
 function wait_for_hugetlb_memory_to_get_depleted() {
   local cgroup="$1"
   local path="$cgroup_path/$cgroup/hugetlb.${MB}MB.$reservation_usage_file"
-  # Wait for hugetlbfs memory to get depleted.
-  while [ $(cat $path) != 0 ]; do
-    echo Waiting for hugetlb memory to get depleted.
-    cat $path
-    sleep 0.5
-  done
+
+  wait_for_file_value "$path" "0"
 }
 
 function wait_for_hugetlb_memory_to_get_reserved() {
   local cgroup="$1"
   local size="$2"
-
   local path="$cgroup_path/$cgroup/hugetlb.${MB}MB.$reservation_usage_file"
-  # Wait for hugetlbfs memory to get written.
-  while [ $(cat $path) != $size ]; do
-    echo Waiting for hugetlb memory reservation to reach size $size.
-    cat $path
-    sleep 0.5
-  done
+
+  wait_for_file_value "$path" "$size"
 }
 
 function wait_for_hugetlb_memory_to_get_written() {
   local cgroup="$1"
   local size="$2"
-
   local path="$cgroup_path/$cgroup/hugetlb.${MB}MB.$fault_usage_file"
-  # Wait for hugetlbfs memory to get written.
-  while [ $(cat $path) != $size ]; do
-    echo Waiting for hugetlb memory to reach size $size.
-    cat $path
-    sleep 0.5
-  done
+
+  wait_for_file_value "$path" "$size"
 }
 
 function write_hugetlbfs_and_get_usage() {
-- 
2.49.0

On 12/21/25 09:58, Li Wang wrote:
> The hugetlb cgroup usage wait loops in charge_reserved_hugetlb.sh were
> unbounded and could hang forever if the expected cgroup file value never
> appears (e.g. due to bugs, timing issues, or unexpected behavior).

Did you actually hit that in practice? Just wondering.

> 
> --- Error log ---
>    # uname -r
>    6.12.0-xxx.el10.aarch64+64k
> 
>    # ls /sys/kernel/mm/hugepages/hugepages-*
>    hugepages-16777216kB/  hugepages-2048kB/  hugepages-524288kB/
> 
>    #./charge_reserved_hugetlb.sh -cgroup-v2
>    # -----------------------------------------
>    ...
>    # nr hugepages = 10
>    # writing cgroup limit: 5368709120
>    # writing reseravation limit: 5368709120
>    ...
>    # write_to_hugetlbfs: Error mapping the file: Cannot allocate memory
>    # Waiting for hugetlb memory reservation to reach size 2684354560.
>    # 0
>    # Waiting for hugetlb memory reservation to reach size 2684354560.
>    # 0
>    # Waiting for hugetlb memory reservation to reach size 2684354560.
>    # 0
>    # Waiting for hugetlb memory reservation to reach size 2684354560.
>    # 0
>    # Waiting for hugetlb memory reservation to reach size 2684354560.
>    # 0
>    # Waiting for hugetlb memory reservation to reach size 2684354560.
>    # 0
>    ...
> 
> Introduce a small helper, wait_for_file_value(), and use it for:
>    - waiting for reservation usage to drop to 0,
>    - waiting for reservation usage to reach a given size,
>    - waiting for fault usage to reach a given size.
> 
> This makes the waits consistent and adds a hard timeout (120 tries with
> 0.5s sleep) so the test fails instead of stalling indefinitely.
> 
> Signed-off-by: Li Wang <liwang@redhat.com>
> Cc: David Hildenbrand <david@kernel.org>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Shuah Khan <shuah@kernel.org>
> Cc: Waiman Long <longman@redhat.com>
> ---
>   .../selftests/mm/charge_reserved_hugetlb.sh   | 47 ++++++++++---------
>   1 file changed, 26 insertions(+), 21 deletions(-)
> 
> diff --git a/tools/testing/selftests/mm/charge_reserved_hugetlb.sh b/tools/testing/selftests/mm/charge_reserved_hugetlb.sh
> index e1fe16bcbbe8..249a5776c074 100755
> --- a/tools/testing/selftests/mm/charge_reserved_hugetlb.sh
> +++ b/tools/testing/selftests/mm/charge_reserved_hugetlb.sh
> @@ -100,7 +100,7 @@ function setup_cgroup() {
>     echo writing cgroup limit: "$cgroup_limit"
>     echo "$cgroup_limit" >$cgroup_path/$name/hugetlb.${MB}MB.$fault_limit_file
>   
> -  echo writing reseravation limit: "$reservation_limit"
> +  echo writing reservation limit: "$reservation_limit"
>     echo "$reservation_limit" > \
>       $cgroup_path/$name/hugetlb.${MB}MB.$reservation_limit_file
>   
> @@ -112,41 +112,46 @@ function setup_cgroup() {
>     fi
>   }
>   
> +function wait_for_file_value() {
> +  local path="$1"
> +  local expect="$2"
> +  local max_tries="120"
> +
> +  local i cur

I would just move "cur" into the loop; I don't see a reason to print it 
on the error path when you printed the value on the last "Waiting" line?

	local cur="$(cat "$path")"

Also, not sure if you really need the "local i" here.

What if the path does not exist, do we want to catch that earlier and 
bail out instead of letting "cat" fail here?

> +  for ((i=1; i<=max_tries; i++)); do
> +    cur="$(cat "$path")"
> +    if [[ "$cur" == "$expect" ]]; then
> +      return 0
> +    fi
> +    echo "Waiting for $path to become '$expect' (current: '$cur') (try $i/$max_tries)"
> +    sleep 0.5

Any reason we don't go for the more intuitive "wait 1s" - max 60s wait?

> +  done


Nothing else jumped at me.

-- 
Cheers

David

David Hildenbrand (Red Hat) <david@kernel.org> wrote:

> On 12/21/25 09:58, Li Wang wrote:
> > The hugetlb cgroup usage wait loops in charge_reserved_hugetlb.sh were
> > unbounded and could hang forever if the expected cgroup file value never
> > appears (e.g. due to bugs, timing issues, or unexpected behavior).
>
> Did you actually hit that in practice? Just wondering.

Yes.

On an aarch64 64k setup with 512MB hugepages, the test failed earlier
(hugetlbfs got mounted with an effective size of 0 due to size=256M), so
write_to_hugetlbfs couldn’t allocate the expected pages. After that, the
script’s wait loops never observed the target value, so they spun forever.

Detail see below logs.

>
> >
> > --- Error log ---
> >    # uname -r
> >    6.12.0-xxx.el10.aarch64+64k
> >
> >    # ls /sys/kernel/mm/hugepages/hugepages-*
> >    hugepages-16777216kB/  hugepages-2048kB/  hugepages-524288kB/
> >
> >    #./charge_reserved_hugetlb.sh -cgroup-v2
> >    # -----------------------------------------
> >    ...
> >    # nr hugepages = 10
> >    # writing cgroup limit: 5368709120
> >    # writing reseravation limit: 5368709120
> >    ...
> >    # write_to_hugetlbfs: Error mapping the file: Cannot allocate memory
> >    # Waiting for hugetlb memory reservation to reach size 2684354560.
> >    # 0
> >    # Waiting for hugetlb memory reservation to reach size 2684354560.
> >    # 0
> >    # Waiting for hugetlb memory reservation to reach size 2684354560.
> >    # 0
> >    # Waiting for hugetlb memory reservation to reach size 2684354560.
> >    # 0
> >    # Waiting for hugetlb memory reservation to reach size 2684354560.
> >    # 0
> >    # Waiting for hugetlb memory reservation to reach size 2684354560.
> >    # 0
> >    ...
> >
> > Introduce a small helper, wait_for_file_value(), and use it for:
> >    - waiting for reservation usage to drop to 0,
> >    - waiting for reservation usage to reach a given size,
> >    - waiting for fault usage to reach a given size.
> >
> > This makes the waits consistent and adds a hard timeout (120 tries with
> > 0.5s sleep) so the test fails instead of stalling indefinitely.
> >
> > Signed-off-by: Li Wang <liwang@redhat.com>
> > Cc: David Hildenbrand <david@kernel.org>
> > Cc: Mark Brown <broonie@kernel.org>
> > Cc: Shuah Khan <shuah@kernel.org>
> > Cc: Waiman Long <longman@redhat.com>
> > ---
> >   .../selftests/mm/charge_reserved_hugetlb.sh   | 47 ++++++++++---------
> >   1 file changed, 26 insertions(+), 21 deletions(-)
> >
> > diff --git a/tools/testing/selftests/mm/charge_reserved_hugetlb.sh b/tools/testing/selftests/mm/charge_reserved_hugetlb.sh
> > index e1fe16bcbbe8..249a5776c074 100755
> > --- a/tools/testing/selftests/mm/charge_reserved_hugetlb.sh
> > +++ b/tools/testing/selftests/mm/charge_reserved_hugetlb.sh
> > @@ -100,7 +100,7 @@ function setup_cgroup() {
> >     echo writing cgroup limit: "$cgroup_limit"
> >     echo "$cgroup_limit" >$cgroup_path/$name/hugetlb.${MB}MB.$fault_limit_file
> >
> > -  echo writing reseravation limit: "$reservation_limit"
> > +  echo writing reservation limit: "$reservation_limit"
> >     echo "$reservation_limit" > \
> >       $cgroup_path/$name/hugetlb.${MB}MB.$reservation_limit_file
> >
> > @@ -112,41 +112,46 @@ function setup_cgroup() {
> >     fi
> >   }
> >
> > +function wait_for_file_value() {
> > +  local path="$1"
> > +  local expect="$2"
> > +  local max_tries="120"
> > +
> > +  local i cur
>
> I would just move "cur" into the loop; I don't see a reason to print it
> on the error path when you printed the value on the last "Waiting" line?
>
>         local cur="$(cat "$path")"

+1

>
> Also, not sure if you really need the "local i" here.
>
> What if the path does not exist, do we want to catch that earlier and
> bail out instead of letting "cat" fail here?

Yes, we can add a file check before the "cat" loop.

>
> > +  for ((i=1; i<=max_tries; i++)); do
> > +    cur="$(cat "$path")"
> > +    if [[ "$cur" == "$expect" ]]; then
> > +      return 0
> > +    fi
> > +    echo "Waiting for $path to become '$expect' (current: '$cur') (try $i/$max_tries)"
> > +    sleep 0.5
>
> Any reason we don't go for the more intuitive "wait 1s" - max 60s wait?

Sure, the total loop time are same.

-- 
Regards,
Li Wang

On 12/21/25 10:35, Li Wang wrote:
> David Hildenbrand (Red Hat) <david@kernel.org> wrote:
> 
>> On 12/21/25 09:58, Li Wang wrote:
>>> The hugetlb cgroup usage wait loops in charge_reserved_hugetlb.sh were
>>> unbounded and could hang forever if the expected cgroup file value never
>>> appears (e.g. due to bugs, timing issues, or unexpected behavior).
>>
>> Did you actually hit that in practice? Just wondering.
> 
> Yes.
> 
> On an aarch64 64k setup with 512MB hugepages, the test failed earlier
> (hugetlbfs got mounted with an effective size of 0 due to size=256M), so
> write_to_hugetlbfs couldn’t allocate the expected pages. After that, the
> script’s wait loops never observed the target value, so they spun forever.

Okay, so essentially what you fix in patch #3, correct?

It might make sense to reorder #2 and #3, and likely current #3 should 
get a Fixes: tag.

Then you can just briefly describe here that this was previously hit due 
to other tests issues. Although I wonder how much value this patch here 
as after #3 is in. But it looks like a cleanup and the timeout of 60s 
sounds reasonable.

I know the reservation of hugetlb folios can take a rather long time in 
some environments, though.

-- 
Cheers

David

On Sun, Dec 21, 2025 at 5:52 PM David Hildenbrand (Red Hat)
<david@kernel.org> wrote:
>
> On 12/21/25 10:35, Li Wang wrote:
> > David Hildenbrand (Red Hat) <david@kernel.org> wrote:
> >
> >> On 12/21/25 09:58, Li Wang wrote:
> >>> The hugetlb cgroup usage wait loops in charge_reserved_hugetlb.sh were
> >>> unbounded and could hang forever if the expected cgroup file value never
> >>> appears (e.g. due to bugs, timing issues, or unexpected behavior).
> >>
> >> Did you actually hit that in practice? Just wondering.
> >
> > Yes.
> >
> > On an aarch64 64k setup with 512MB hugepages, the test failed earlier
> > (hugetlbfs got mounted with an effective size of 0 due to size=256M), so
> > write_to_hugetlbfs couldn’t allocate the expected pages. After that, the
> > script’s wait loops never observed the target value, so they spun forever.
>
> Okay, so essentially what you fix in patch #3, correct?
>
> It might make sense to reorder #2 and #3, and likely current #3 should
> get a Fixes: tag.

+1

> Then you can just briefly describe here that this was previously hit due
> to other tests issues. Although I wonder how much value this patch here
> as after #3 is in. But it looks like a cleanup and the timeout of 60s
> sounds reasonable.

The reason is that I improved the infinite loop before debugging the #3 issue.
But your suggestion makes sense, and I will reorder patch #2 and #3.

-- 
Regards,
Li Wang