Linux-kselftest Archive on lore.kernel.org
 help / color / Atom feed
From: Mina Almasry <almasrymina@google.com>
To: shuah <shuah@kernel.org>
Cc: "Mike Kravetz" <mike.kravetz@oracle.com>,
	"David Rientjes" <rientjes@google.com>,
	"Shakeel Butt" <shakeelb@google.com>,
	"Greg Thelen" <gthelen@google.com>,
	"Andrew Morton" <akpm@linux-foundation.org>,
	khalid.aziz@oracle.com,
	"open list" <linux-kernel@vger.kernel.org>,
	linux-mm@kvack.org, linux-kselftest@vger.kernel.org,
	cgroups@vger.kernel.org,
	"Aneesh Kumar" <aneesh.kumar@linux.vnet.ibm.com>,
	"Michal Koutný" <mkoutny@suse.com>
Subject: Re: [PATCH v4 8/9] hugetlb_cgroup: Add hugetlb_cgroup reservation tests
Date: Wed, 18 Sep 2019 18:53:05 -0700
Message-ID: <CAHS8izOnJtMFsevb1U0qiBsQsM+UxyO=1F49NKuGrZGwNAz8Yw@mail.gmail.com> (raw)
In-Reply-To: <4240683f-9fa6-daf8-95ee-259667c87ef7@kernel.org>

On Mon, Sep 16, 2019 at 6:52 PM shuah <shuah@kernel.org> wrote:
>
> On 9/10/19 5:31 PM, Mina Almasry wrote:
> > The tests use both shared and private mapped hugetlb memory, and
> > monitors the hugetlb usage counter as well as the hugetlb reservation
> > counter. They test different configurations such as hugetlb memory usage
> > via hugetlbfs, or MAP_HUGETLB, or shmget/shmat, and with and without
> > MAP_POPULATE.
> >
> > Signed-off-by: Mina Almasry <almasrymina@google.com>
> > ---
> >   tools/testing/selftests/vm/.gitignore         |   1 +
> >   tools/testing/selftests/vm/Makefile           |   4 +
> >   .../selftests/vm/charge_reserved_hugetlb.sh   | 440 ++++++++++++++++++
> >   .../selftests/vm/write_hugetlb_memory.sh      |  22 +
> >   .../testing/selftests/vm/write_to_hugetlbfs.c | 252 ++++++++++
> >   5 files changed, 719 insertions(+)
> >   create mode 100755 tools/testing/selftests/vm/charge_reserved_hugetlb.sh
> >   create mode 100644 tools/testing/selftests/vm/write_hugetlb_memory.sh
> >   create mode 100644 tools/testing/selftests/vm/write_to_hugetlbfs.c
> >
> > diff --git a/tools/testing/selftests/vm/.gitignore b/tools/testing/selftests/vm/.gitignore
> > index 31b3c98b6d34d..d3bed9407773c 100644
> > --- a/tools/testing/selftests/vm/.gitignore
> > +++ b/tools/testing/selftests/vm/.gitignore
> > @@ -14,3 +14,4 @@ virtual_address_range
> >   gup_benchmark
> >   va_128TBswitch
> >   map_fixed_noreplace
> > +write_to_hugetlbfs
> > diff --git a/tools/testing/selftests/vm/Makefile b/tools/testing/selftests/vm/Makefile
> > index 9534dc2bc9295..8d37d5409b52c 100644
> > --- a/tools/testing/selftests/vm/Makefile
> > +++ b/tools/testing/selftests/vm/Makefile
> > @@ -18,6 +18,7 @@ TEST_GEN_FILES += transhuge-stress
> >   TEST_GEN_FILES += userfaultfd
> >   TEST_GEN_FILES += va_128TBswitch
> >   TEST_GEN_FILES += virtual_address_range
> > +TEST_GEN_FILES += write_to_hugetlbfs
> >
> >   TEST_PROGS := run_vmtests
> >
> > @@ -29,3 +30,6 @@ include ../lib.mk
> >   $(OUTPUT)/userfaultfd: LDLIBS += -lpthread
> >
> >   $(OUTPUT)/mlock-random-test: LDLIBS += -lcap
> > +
> > +# Why does adding $(OUTPUT)/ like above not apply this flag..?
>
> Can you verify the following and remove this comment, once you figure
> out if you need $(OUTPUT)/
> > +write_to_hugetlbfs: CFLAGS += -static
>
> It should. Did you test "make O=" and "KBUILD_OUTPUT" kselftest
> use-cases?
>

Turns out I don't need -static actually.

> > diff --git a/tools/testing/selftests/vm/charge_reserved_hugetlb.sh b/tools/testing/selftests/vm/charge_reserved_hugetlb.sh
> > new file mode 100755
> > index 0000000000000..09e90e8f6fab4
> > --- /dev/null
> > +++ b/tools/testing/selftests/vm/charge_reserved_hugetlb.sh
> > @@ -0,0 +1,440 @@
> > +#!/bin/sh
> > +# SPDX-License-Identifier: GPL-2.0
> > +
> > +set -e
> > +
> > +cgroup_path=/dev/cgroup/memory
> > +if [[ ! -e $cgroup_path ]]; then
> > +      mkdir -p $cgroup_path
> > +      mount -t cgroup -o hugetlb,memory cgroup $cgroup_path
> > +fi
> > +
>
> Does this test need root access? If yes, please add root check
> and skip the test when a non-root runs the test.
>
> > +cleanup () {
> > +     echo $$ > $cgroup_path/tasks
> > +
> > +     set +e
> > +     if [[ "$(pgrep write_to_hugetlbfs)" != "" ]]; then
> > +           kill -2 write_to_hugetlbfs
> > +           # Wait for hugetlbfs memory to get depleted.
> > +           sleep 0.5
>
> This time looks arbitrary. How can you be sure it gets depleted?
> Is there another way to check for it.
>
> > +     fi
> > +     set -e
> > +
> > +     if [[ -e /mnt/huge ]]; then
> > +           rm -rf /mnt/huge/*
> > +           umount /mnt/huge || echo error
> > +           rmdir /mnt/huge
> > +     fi
> > +     if [[ -e $cgroup_path/hugetlb_cgroup_test ]]; then
> > +           rmdir $cgroup_path/hugetlb_cgroup_test
> > +     fi
> > +     if [[ -e $cgroup_path/hugetlb_cgroup_test1 ]]; then
> > +           rmdir $cgroup_path/hugetlb_cgroup_test1
> > +     fi
> > +     if [[ -e $cgroup_path/hugetlb_cgroup_test2 ]]; then
> > +           rmdir $cgroup_path/hugetlb_cgroup_test2
> > +     fi
> > +     echo 0 > /proc/sys/vm/nr_hugepages
> > +     echo CLEANUP DONE
> > +}
> > +
> > +cleanup
> > +
> > +function expect_equal() {
> > +      local expected="$1"
> > +      local actual="$2"
> > +      local error="$3"
> > +
> > +      if [[ "$expected" != "$actual" ]]; then
> > +         echo "expected ($expected) != actual ($actual): $3"
> > +         cleanup
> > +         exit 1
> > +      fi
> > +}
> > +
> > +function setup_cgroup() {
> > +      local name="$1"
> > +      local cgroup_limit="$2"
> > +      local reservation_limit="$3"
> > +
> > +      mkdir $cgroup_path/$name
> > +
> > +      echo writing cgroup limit: "$cgroup_limit"
> > +      echo "$cgroup_limit" > $cgroup_path/$name/hugetlb.2MB.limit_in_bytes
> > +
> > +      echo writing reseravation limit: "$reservation_limit"
> > +      echo "$reservation_limit" > \
> > +         $cgroup_path/$name/hugetlb.2MB.reservation_limit_in_bytes
> > +      echo 0 > $cgroup_path/$name/cpuset.cpus
> > +      echo 0 > $cgroup_path/$name/cpuset.mems
> > +}
> > +
> > +function write_hugetlbfs_and_get_usage() {
> > +      local cgroup="$1"
> > +      local size="$2"
> > +      local populate="$3"
> > +      local write="$4"
> > +      local path="$5"
> > +      local method="$6"
> > +      local private="$7"
> > +      local expect_failure="$8"
> > +
> > +      # Function return values.
> > +      reservation_failed=0
> > +      oom_killed=0
> > +      hugetlb_difference=0
> > +      reserved_difference=0
> > +
> > +      local hugetlb_usage=$cgroup_path/$cgroup/hugetlb.2MB.usage_in_bytes
> > +      local reserved_usage=$cgroup_path/$cgroup/hugetlb.2MB.reservation_usage_in_bytes
> > +
> > +      local hugetlb_before=$(cat $hugetlb_usage)
> > +      local reserved_before=$(cat $reserved_usage)
> > +
> > +      echo
> > +      echo Starting:
> > +      echo hugetlb_usage="$hugetlb_before"
> > +      echo reserved_usage="$reserved_before"
> > +      echo expect_failure is "$expect_failure"
> > +
> > +      set +e
> > +      if [[ "$method" == "1" ]] || [[ "$method" == 2 ]] || \
> > +         [[ "$private" == "-r" ]] && [[ "$expect_failure" != 1 ]]; then
> > +         bash write_hugetlb_memory.sh "$size" "$populate" "$write" \
> > +               "$cgroup"  "$path" "$method" "$private" "-l" &
> > +
> > +         local write_result=$?
> > +         # This sleep is to make sure that the script above has had enough
> > +         # time to do its thing, since it runs in the background. This may
> > +         # cause races...
> > +         sleep 0.5
>
> I am not happy with these arbitrary sleep times, especially coupled with
> the comment about races above. :)
>
> > +         echo write_result is $write_result
> > +      else
> > +         bash write_hugetlb_memory.sh "$size" "$populate" "$write" \
> > +               "$cgroup"  "$path" "$method" "$private"
> > +         local write_result=$?
> > +      fi
> > +      set -e
> > +
> > +      if [[ "$write_result" == 1 ]]; then
> > +         reservation_failed=1
> > +      fi
> > +
> > +      # On linus/master, the above process gets SIGBUS'd on oomkill, with
> > +      # return code 135. On earlier kernels, it gets actual oomkill, with return
> > +      # code 137, so just check for both conditions incase we're testing against
>
> in case
>
> > +      # an earlier kernel.
> > +      if [[ "$write_result" == 135 ]] || [[ "$write_result" == 137 ]]; then
>
> Please add defines for these return values.
>

There is comment that explains this line. Not enough clarity?

> > +         oom_killed=1
> > +      fi
> > +
> > +      local hugetlb_after=$(cat $hugetlb_usage)
> > +      local reserved_after=$(cat $reserved_usage)
> > +
> > +      echo After write:
> > +      echo hugetlb_usage="$hugetlb_after"
> > +      echo reserved_usage="$reserved_after"
> > +
> > +      hugetlb_difference=$(($hugetlb_after - $hugetlb_before))
> > +      reserved_difference=$(($reserved_after - $reserved_before))
> > +}
> > +
> > +function cleanup_hugetlb_memory() {
> > +      set +e
> > +      if [[ "$(pgrep write_to_hugetlbfs)" != "" ]]; then
> > +         echo kiling write_to_hugetlbfs
> > +         killall -2 write_to_hugetlbfs
> > +         # Wait for hugetlbfs memory to get depleted.
> > +         sleep 0.5
>
> Sleep time? Rationale for this number?
>
> > +      fi
> > +      set -e
> > +
> > +      if [[ -e /mnt/huge ]]; then
> > +         rm -rf /mnt/huge/*
> > +           umount /mnt/huge
> > +           rmdir /mnt/huge
> > +      fi
> > +}
> > +
> > +function run_test() {
> > +      local size="$1"
> > +      local populate="$2"
> > +      local write="$3"
> > +      local cgroup_limit="$4"
> > +      local reservation_limit="$5"
> > +      local nr_hugepages="$6"
> > +      local method="$7"
> > +      local private="$8"
> > +      local expect_failure="$9"
> > +
> > +      # Function return values.
> > +      hugetlb_difference=0
> > +      reserved_difference=0
> > +      reservation_failed=0
> > +      oom_killed=0
> > +
> > +      echo nr hugepages = "$nr_hugepages"
> > +      echo "$nr_hugepages" > /proc/sys/vm/nr_hugepages
> > +
> > +      setup_cgroup "hugetlb_cgroup_test" "$cgroup_limit" "$reservation_limit"
> > +
> > +      mkdir -p /mnt/huge
> > +      mount -t hugetlbfs \
> > +         -o pagesize=2M,size=256M none /mnt/huge
> > +
> > +      write_hugetlbfs_and_get_usage "hugetlb_cgroup_test" "$size" "$populate" \
> > +         "$write" "/mnt/huge/test" "$method" "$private" "$expect_failure"
> > +
> > +      cleanup_hugetlb_memory
> > +
> > +      local final_hugetlb=$(cat $cgroup_path/hugetlb_cgroup_test/hugetlb.2MB.usage_in_bytes)
> > +      local final_reservation=$(cat $cgroup_path/hugetlb_cgroup_test/hugetlb.2MB.reservation_usage_in_bytes)
> > +
> > +      expect_equal "0" "$final_hugetlb" "final hugetlb is not zero"
> > +      expect_equal "0" "$final_reservation" "final reservation is not zero"
> > +}
> > +
> > +function run_multiple_cgroup_test() {
> > +      local size1="$1"
> > +      local populate1="$2"
> > +      local write1="$3"
> > +      local cgroup_limit1="$4"
> > +      local reservation_limit1="$5"
> > +
> > +      local size2="$6"
> > +      local populate2="$7"
> > +      local write2="$8"
> > +      local cgroup_limit2="$9"
> > +      local reservation_limit2="${10}"
> > +
> > +      local nr_hugepages="${11}"
> > +      local method="${12}"
> > +      local private="${13}"
> > +      local expect_failure="${14}"
> > +
> > +      # Function return values.
> > +      hugetlb_difference1=0
> > +      reserved_difference1=0
> > +      reservation_failed1=0
> > +      oom_killed1=0
> > +
> > +      hugetlb_difference2=0
> > +      reserved_difference2=0
> > +      reservation_failed2=0
> > +      oom_killed2=0
> > +
> > +
> > +      echo nr hugepages = "$nr_hugepages"
> > +      echo "$nr_hugepages" > /proc/sys/vm/nr_hugepages
> > +
> > +      setup_cgroup "hugetlb_cgroup_test1" "$cgroup_limit1" "$reservation_limit1"
> > +      setup_cgroup "hugetlb_cgroup_test2" "$cgroup_limit2" "$reservation_limit2"
> > +
> > +      mkdir -p /mnt/huge
> > +      mount -t hugetlbfs \
> > +         -o pagesize=2M,size=256M none /mnt/huge
> > +
> > +      write_hugetlbfs_and_get_usage "hugetlb_cgroup_test1" "$size1" \
> > +         "$populate1" "$write1" "/mnt/huge/test1" "$method" "$private" \
> > +         "$expect_failure"
> > +
> > +      hugetlb_difference1=$hugetlb_difference
> > +      reserved_difference1=$reserved_difference
> > +      reservation_failed1=$reservation_failed
> > +      oom_killed1=$oom_killed
> > +
> > +      local cgroup1_hugetlb_usage=$cgroup_path/hugetlb_cgroup_test1/hugetlb.2MB.usage_in_bytes
> > +      local cgroup1_reservation_usage=$cgroup_path/hugetlb_cgroup_test1/hugetlb.2MB.reservation_usage_in_bytes
> > +      local cgroup2_hugetlb_usage=$cgroup_path/hugetlb_cgroup_test2/hugetlb.2MB.usage_in_bytes
> > +      local cgroup2_reservation_usage=$cgroup_path/hugetlb_cgroup_test2/hugetlb.2MB.reservation_usage_in_bytes
> > +
> > +      local usage_before_second_write=$(cat $cgroup1_hugetlb_usage)
> > +      local reservation_usage_before_second_write=$(cat \
> > +         $cgroup1_reservation_usage)
> > +
> > +      write_hugetlbfs_and_get_usage "hugetlb_cgroup_test2" "$size2" \
> > +         "$populate2" "$write2" "/mnt/huge/test2" "$method" "$private" \
> > +         "$expect_failure"
> > +
> > +      hugetlb_difference2=$hugetlb_difference
> > +      reserved_difference2=$reserved_difference
> > +      reservation_failed2=$reservation_failed
> > +      oom_killed2=$oom_killed
> > +
> > +      expect_equal "$usage_before_second_write" \
> > +         "$(cat $cgroup1_hugetlb_usage)" "Usage changed."
> > +      expect_equal "$reservation_usage_before_second_write" \
> > +         "$(cat $cgroup1_reservation_usage)" "Reservation usage changed."
> > +
> > +      cleanup_hugetlb_memory
> > +
> > +      local final_hugetlb=$(cat $cgroup1_hugetlb_usage)
> > +      local final_reservation=$(cat $cgroup1_reservation_usage)
> > +
> > +      expect_equal "0" "$final_hugetlb" \
> > +         "hugetlbt_cgroup_test1 final hugetlb is not zero"
> > +      expect_equal "0" "$final_reservation" \
> > +         "hugetlbt_cgroup_test1 final reservation is not zero"
> > +
> > +      local final_hugetlb=$(cat $cgroup2_hugetlb_usage)
> > +      local final_reservation=$(cat $cgroup2_reservation_usage)
> > +
> > +      expect_equal "0" "$final_hugetlb" \
> > +         "hugetlb_cgroup_test2 final hugetlb is not zero"
> > +      expect_equal "0" "$final_reservation" \
> > +         "hugetlb_cgroup_test2 final reservation is not zero"
> > +}
> > +
> > +for private in "" "-r" ; do
> > +for populate in  "" "-o"; do
> > +for method in 0 1 2; do
> > +
> > +# Skip mmap(MAP_HUGETLB | MAP_SHARED). Doesn't seem to be supported.
> > +if [[ "$method" == 1 ]] && [[ "$private" == "" ]]; then
> > +      continue
> > +fi
> > +
> > +# Skip populated shmem tests. Doesn't seem to be supported.
> > +if [[ "$method" == 2"" ]] && [[ "$populate" == "-o" ]]; then
> > +      continue
> > +fi
> > +
> > +cleanup
> > +echo
> > +echo
> > +echo
> > +echo Test normal case.
> > +echo private=$private, populate=$populate, method=$method
> > +run_test $((10 * 1024 * 1024)) "$populate" "" $((20 * 1024 * 1024)) \
> > +      $((20 * 1024 * 1024)) 10 "$method" "$private" "0"
> > +
> > +echo Memory charged to hugtlb=$hugetlb_difference
> > +echo Memory charged to reservation=$reserved_difference
> > +
> > +if [[ "$populate" == "-o" ]]; then
> > +      expect_equal "$((10 * 1024 * 1024))" "$hugetlb_difference" \
> > +         "Reserved memory charged to hugetlb cgroup."
> > +else
> > +      expect_equal "0" "$hugetlb_difference" \
> > +         "Reserved memory charged to hugetlb cgroup."
> > +fi
> > +
> > +expect_equal "$((10 * 1024 * 1024))" "$reserved_difference" \
> > +      "Reserved memory not charged to reservation usage."
> > +echo 'PASS'
> > +
> > +cleanup
> > +echo
> > +echo
> > +echo
> > +echo Test normal case with write.
> > +echo private=$private, populate=$populate, method=$method
> > +run_test $((10 * 1024 * 1024)) "$populate" '-w' $((20 * 1024 * 1024)) \
> > +      $((20 * 1024 * 1024)) 10 "$method" "$private" "0"
> > +
> > +echo Memory charged to hugtlb=$hugetlb_difference
> > +echo Memory charged to reservation=$reserved_difference
> > +
> > +expect_equal "$((10 * 1024 * 1024))" "$hugetlb_difference" \
> > +      "Reserved memory charged to hugetlb cgroup."
> > +expect_equal "$((10 * 1024 * 1024))" "$reserved_difference" \
> > +      "Reserved memory not charged to reservation usage."
> > +echo 'PASS'
> > +
> > +
> > +cleanup
> > +echo
> > +echo
> > +echo
> > +echo Test more than reservation case.
> > +echo private=$private, populate=$populate, method=$method
> > +run_test "$((10 * 1024 * 1024))" "$populate" '' "$((20 * 1024 * 1024))" \
> > +      "$((5 * 1024 * 1024))" "10" "$method" "$private" "1"
> > +
> > +expect_equal "1" "$reservation_failed" "Reservation succeeded."
> > +echo 'PASS'
> > +
> > +cleanup
> > +
> > +echo
> > +echo
> > +echo
> > +echo Test more than cgroup limit case.
> > +echo private=$private, populate=$populate, method=$method
> > +
> > +# Not sure if shm memory can be cleaned up when the process gets sigbus'd.
> > +if [[ "$method" != 2 ]]; then
> > +      run_test $((10 * 1024 * 1024)) "$populate" "-w" $((5 * 1024 * 1024)) \
> > +         $((20 * 1024 * 1024)) 10 "$method" "$private" "1"
> > +
> > +      expect_equal "1" "$oom_killed" "Not oom killed."
> > +fi
> > +echo 'PASS'
> > +
> > +cleanup
> > +
> > +echo
> > +echo
> > +echo
> > +echo Test normal case, multiple cgroups.
> > +echo private=$private, populate=$populate, method=$method
> > +run_multiple_cgroup_test "$((6 * 1024 * 1024))" "$populate" "" \
> > +      "$((20 * 1024 * 1024))" "$((20 * 1024 * 1024))" "$((10 * 1024 * 1024))" \
> > +      "$populate" "" "$((20 * 1024 * 1024))" "$((20 * 1024 * 1024))" "10" \
> > +      "$method" "$private" "0"
> > +
> > +echo Memory charged to hugtlb1=$hugetlb_difference1
> > +echo Memory charged to reservation1=$reserved_difference1
> > +echo Memory charged to hugtlb2=$hugetlb_difference2
> > +echo Memory charged to reservation2=$reserved_difference2
> > +
> > +expect_equal "$((6 * 1024 * 1024))" "$reserved_difference1" \
> > +      "Incorrect reservations charged to cgroup 1."
> > +expect_equal "$((10 * 1024 * 1024))" "$reserved_difference2" \
> > +      "Incorrect reservation charged to cgroup 2."
> > +if [[ "$populate" == "-o" ]]; then
> > +      expect_equal "$((6 * 1024 * 1024))" "$hugetlb_difference1" \
> > +         "Incorrect hugetlb charged to cgroup 1."
> > +      expect_equal "$((10 * 1024 * 1024))" "$hugetlb_difference2" \
> > +         "Incorrect hugetlb charged to cgroup 2."
> > +else
> > +      expect_equal "0" "$hugetlb_difference1" \
> > +         "Incorrect hugetlb charged to cgroup 1."
> > +      expect_equal "0" "$hugetlb_difference2" \
> > +         "Incorrect hugetlb charged to cgroup 2."
> > +fi
> > +echo 'PASS'
> > +
> > +cleanup
> > +echo
> > +echo
> > +echo
> > +echo Test normal case with write, multiple cgroups.
> > +echo private=$private, populate=$populate, method=$method
> > +run_multiple_cgroup_test "$((6 * 1024 * 1024))" "$populate" "-w" \
> > +      "$((20 * 1024 * 1024))" "$((20 * 1024 * 1024))" "$((10 * 1024 * 1024))" \
> > +      "$populate" "-w" "$((20 * 1024 * 1024))" "$((20 * 1024 * 1024))" "10" \
> > +      "$method" "$private" "0"
> > +
> > +echo Memory charged to hugtlb1=$hugetlb_difference1
> > +echo Memory charged to reservation1=$reserved_difference1
> > +echo Memory charged to hugtlb2=$hugetlb_difference2
> > +echo Memory charged to reservation2=$reserved_difference2
> > +
> > +expect_equal "$((6 * 1024 * 1024))" "$hugetlb_difference1" \
> > +      "Incorrect hugetlb charged to cgroup 1."
> > +expect_equal "$((6 * 1024 * 1024))" "$reserved_difference1" \
> > +      "Incorrect reservation charged to cgroup 1."
> > +expect_equal "$((10 * 1024 * 1024))" "$hugetlb_difference2" \
> > +      "Incorrect hugetlb charged to cgroup 2."
> > +expect_equal "$((10 * 1024 * 1024))" "$reserved_difference2" \
> > +      "Incorrected reservation charged to cgroup 2."
> > +
> > +echo 'PASS'
> > +
> > +done # private
> > +done # populate
> > +done # method
> > +
> > +umount $cgroup_path
> > +rmdir $cgroup_path
> > diff --git a/tools/testing/selftests/vm/write_hugetlb_memory.sh b/tools/testing/selftests/vm/write_hugetlb_memory.sh
> > new file mode 100644
> > index 0000000000000..08f5fa5527cfd
> > --- /dev/null
> > +++ b/tools/testing/selftests/vm/write_hugetlb_memory.sh
> > @@ -0,0 +1,22 @@
> > +#!/bin/sh
> > +# SPDX-License-Identifier: GPL-2.0
> > +
> > +set -e
> > +
> > +size=$1
> > +populate=$2
> > +write=$3
> > +cgroup=$4
> > +path=$5
> > +method=$6
> > +private=$7
> > +want_sleep=$8
> > +
> > +echo "Putting task in cgroup '$cgroup'"
> > +echo $$ > /dev/cgroup/memory/"$cgroup"/tasks
> > +
> > +echo "Method is $method"
> > +
> > +set +e
> > +./write_to_hugetlbfs -p "$path" -s "$size" "$write" "$populate" -m "$method" \
> > +      "$private" "$want_sleep"
> > diff --git a/tools/testing/selftests/vm/write_to_hugetlbfs.c b/tools/testing/selftests/vm/write_to_hugetlbfs.c
> > new file mode 100644
> > index 0000000000000..f02a897427a97
> > --- /dev/null
> > +++ b/tools/testing/selftests/vm/write_to_hugetlbfs.c
> > @@ -0,0 +1,252 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * This program reserves and uses hugetlb memory, supporting a bunch of
> > + * scenorios needed by the charged_reserved_hugetlb.sh test.
>
> Spelling?
>
> > + */
> > +
> > +#include <err.h>
> > +#include <errno.h>
> > +#include <signal.h>
> > +#include <stdio.h>
> > +#include <stdlib.h>
> > +#include <string.h>
> > +#include <unistd.h>
> > +#include <fcntl.h>
> > +#include <sys/types.h>
> > +#include <sys/shm.h>
> > +#include <sys/stat.h>
> > +#include <sys/mman.h>
> > +
> > +/* Global definitions. */
> > +enum method {
> > +     HUGETLBFS,
> > +     MMAP_MAP_HUGETLB,
> > +     SHM,
> > +     MAX_METHOD
> > +};
> > +
> > +
> > +/* Global variables. */
> > +static const char *self;
> > +static char *shmaddr;
> > +static int shmid;
> > +
> > +/*
> > + * Show usage and exit.
> > + */
> > +static void exit_usage(void)
> > +{
> > +
> > +     printf("Usage: %s -p <path to hugetlbfs file> -s <size to map> "
> > +             "[-m <0=hugetlbfs | 1=mmap(MAP_HUGETLB)>] [-l] [-r] "
> > +             "[-o] [-w]\n", self);
> > +     exit(EXIT_FAILURE);
> > +}
> > +
> > +void sig_handler(int signo)
> > +{
> > +     printf("Received %d.\n", signo);
> > +     if (signo == SIGINT) {
> > +             printf("Deleting the memory\n");
> > +             if (shmdt((const void *)shmaddr) != 0) {
> > +                     perror("Detach failure");
> > +                     shmctl(shmid, IPC_RMID, NULL);
> > +                     exit(4);
>
> Is this a skip error code? Please note that kselftest framework
> interprets this as a skipped test when returb value is 4.
>
This is not a kselftest framework binary. It's a tool to be called
from charge_reserved_hugetlb.sh The exit value is just to make all the
exits in this file return different codes so I can debug easier where
the tool exited from. They don't actually mean anything. Is that OK?

> > +             }
> > +
> > +             shmctl(shmid, IPC_RMID, NULL);
> > +             printf("Done deleting the memory\n");
> > +     }
> > +     exit(2);
>
> What about this one? What does exit code 2 mean?
>
> > +}
> > +
> > +int main(int argc, char **argv)
> > +{
> > +     int fd = 0;
> > +     int key = 0;
> > +     int *ptr = NULL;
> > +     int c = 0;
> > +     int size = 0;
> > +     char path[256] = "";
> > +     enum method method = MAX_METHOD;
> > +     int want_sleep = 0, private = 0;
> > +     int populate = 0;
> > +     int write = 0;
> > +
> > +     unsigned long i;
> > +
> > +
> > +     if (signal(SIGINT, sig_handler) == SIG_ERR)
> > +             err(1, "\ncan't catch SIGINT\n");
> > +
> > +     /* Parse command-line arguments. */
> > +     setvbuf(stdout, NULL, _IONBF, 0);
> > +     self = argv[0];
> > +
> > +     while ((c = getopt(argc, argv, "s:p:m:owlr")) != -1) {
> > +             switch (c) {
> > +             case 's':
> > +                     size = atoi(optarg);
> > +                     break;
> > +             case 'p':
> > +                     strncpy(path, optarg, sizeof(path));
> > +                     break;
> > +             case 'm':
> > +                     if (atoi(optarg) >= MAX_METHOD) {
> > +                             errno = EINVAL;
> > +                             perror("Invalid -m.");
> > +                             exit_usage();
> > +                     }
> > +                     method = atoi(optarg);
> > +                     break;
> > +             case 'o':
> > +                     populate = 1;
> > +                     break;
> > +             case 'w':
> > +                     write = 1;
> > +                     break;
> > +             case 'l':
> > +                     want_sleep = 1;
> > +                     break;
> > +             case 'r':
> > +                     private = 1;
> > +                     break;
> > +             default:
> > +                     errno = EINVAL;
> > +                     perror("Invalid arg");
> > +                     exit_usage();
> > +             }
> > +     }
> > +
> > +     if (strncmp(path, "", sizeof(path)) != 0) {
> > +             printf("Writing to this path: %s\n", path);
> > +     } else {
> > +             errno = EINVAL;
> > +             perror("path not found");
> > +             exit_usage();
> > +     }
> > +
> > +     if (size != 0) {
> > +             printf("Writing this size: %d\n", size);
> > +     } else {
> > +             errno = EINVAL;
> > +             perror("size not found");
> > +             exit_usage();
> > +     }
> > +
> > +     if (!populate)
> > +             printf("Not populating.\n");
> > +     else
> > +             printf("Populating.\n");
> > +
> > +     if (!write)
> > +             printf("Not writing to memory.\n");
> > +
> > +     if (method == MAX_METHOD) {
> > +             errno = EINVAL;
> > +             perror("-m Invalid");
> > +             exit_usage();
> > +     } else
> > +             printf("Using method=%d\n", method);
> > +
> > +     if (!private)
> > +             printf("Shared mapping.\n");
> > +     else
> > +             printf("Private mapping.\n");
> > +
> > +
> > +     switch (method) {
> > +     case HUGETLBFS:
> > +             printf("Allocating using HUGETLBFS.\n");
> > +             fd = open(path, O_CREAT | O_RDWR, 0777);
> > +             if (fd == -1)
> > +                     err(1, "Failed to open file.");
> > +
> > +             ptr = mmap(NULL, size, PROT_READ | PROT_WRITE,
> > +                     (private ? MAP_PRIVATE : MAP_SHARED) | (populate ?
> > +                             MAP_POPULATE : 0), fd, 0);
> > +
> > +             if (ptr == MAP_FAILED) {
> > +                     close(fd);
> > +                     err(1, "Error mapping the file");
> > +             }
> > +             break;
> > +     case MMAP_MAP_HUGETLB:
> > +             printf("Allocating using MAP_HUGETLB.\n");
> > +             ptr = mmap(NULL, size,
> > +             PROT_READ | PROT_WRITE,
> > +             (private ? (MAP_PRIVATE | MAP_ANONYMOUS) : MAP_SHARED) |
> > +             MAP_HUGETLB | (populate ?
> > +                     MAP_POPULATE : 0),
> > +             -1, 0);
> > +
> > +             if (ptr == MAP_FAILED)
> > +                     err(1, "mmap");
> > +
> > +             printf("Returned address is %p\n", ptr);
> > +             break;
> > +     case SHM:
> > +             printf("Allocating using SHM.\n");
> > +             shmid = shmget(key, size, SHM_HUGETLB | IPC_CREAT | SHM_R |
> > +                             SHM_W);
> > +             if (shmid < 0) {
> > +                     shmid = shmget(++key, size, SHM_HUGETLB | IPC_CREAT |
> > +                                     SHM_R | SHM_W);
> > +                     if (shmid < 0)
> > +                             err(1, "shmget");
> > +
> > +             }
> > +             printf("shmid: 0x%x, shmget key:%d\n", shmid, key);
> > +
> > +             shmaddr = shmat(shmid, NULL, 0);
> > +             if (shmaddr == (char *)-1) {
> > +                     perror("Shared memory attach failure");
> > +                     shmctl(shmid, IPC_RMID, NULL);
> > +                     exit(2);
> > +             }
> > +             printf("shmaddr: %p\n", shmaddr);
> > +
> > +             break;
> > +     default:
> > +             errno = EINVAL;
> > +             err(1, "Invalid method.");
> > +     }
> > +
> > +     if (write) {
> > +             printf("Writing to memory.\n");
> > +             if (method != SHM) {
> > +                     memset(ptr, 1, size);
> > +             } else {
> > +                     printf("Starting the writes:\n");
> > +                     for (i = 0; i < size; i++) {
> > +                             shmaddr[i] = (char)(i);
> > +                             if (!(i % (1024 * 1024)))
> > +                                     printf(".");
> > +                     }
> > +                     printf("\n");
> > +
> > +                     printf("Starting the Check...");
> > +                     for (i = 0; i < size; i++)
> > +                             if (shmaddr[i] != (char)i) {
> > +                                     printf("\nIndex %lu mismatched\n", i);
> > +                                     exit(3);
> > +                             }
> > +                     printf("Done.\n");
> > +
> > +
> > +             }
> > +     }
> > +
> > +     if (want_sleep) {
> > +             /* Signal to caller that we're done. */
> > +             printf("DONE\n");
> > +
> > +             /* Hold memory until external kill signal is delivered. */
> > +             while (1)
> > +                     sleep(100);
>
> Please don't add sleeps. This will hold up the kselftest run.

This tool is meant to allocate a bunch of memory and hold it until the
code in ./charge_reserved_hugetlb.sh checks that the usage is
accounted for. charge_reserved_hugetlb.sh runs this tool in the
background, and asks it to hold the memory until it verifies
accounting, then asks it to delete the memory. It should not hold up a
run.

>
> > +     }
> > +
> > +     close(fd);
>
> Is this close() necessary in all cases? Looks like MMAP_MAP_HUGETLB
> is the only case that opens it.
>
> I am not sure if the error legs are correct.
>
> > +
> > +     return 0;
> > +}
> > --
> > 2.23.0.162.g0b9fbb3734-goog
> >
>
> thanks,
> -- Shuah

Thanks for the review, I should be able to address everything else in
the next patchset.

  reply index

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-09-10 23:31 [PATCH v4 0/9] hugetlb_cgroup: Add hugetlb_cgroup reservation limits Mina Almasry
2019-09-10 23:31 ` [PATCH v4 1/9] hugetlb_cgroup: Add hugetlb_cgroup reservation counter Mina Almasry
2019-09-16 23:43   ` shuah
2019-09-10 23:31 ` [PATCH v4 2/9] hugetlb_cgroup: add interface for charge/uncharge hugetlb reservations Mina Almasry
2019-09-17  1:29   ` shuah
2019-09-10 23:31 ` [PATCH v4 3/9] hugetlb_cgroup: add reservation accounting for private mappings Mina Almasry
2019-09-10 23:31 ` [PATCH v4 4/9] hugetlb: region_chg provides only cache entry Mina Almasry
2019-09-16 22:17   ` Mike Kravetz
2019-09-10 23:31 ` [PATCH v4 5/9] hugetlb: remove duplicated code Mina Almasry
2019-09-16 22:25   ` Mike Kravetz
2019-09-10 23:31 ` [PATCH v4 6/9] hugetlb: disable region_add file_region coalescing Mina Almasry
2019-09-16 23:57   ` Mike Kravetz
2019-09-17  0:16     ` Mina Almasry
2019-09-10 23:31 ` [PATCH v4 7/9] hugetlb_cgroup: add accounting for shared mappings Mina Almasry
2019-09-10 23:31 ` [PATCH v4 8/9] hugetlb_cgroup: Add hugetlb_cgroup reservation tests Mina Almasry
2019-09-17  1:52   ` shuah
2019-09-19  1:53     ` Mina Almasry [this message]
2019-09-10 23:31 ` [PATCH v4 9/9] hugetlb_cgroup: Add hugetlb_cgroup reservation docs Mina Almasry
2019-09-17  1:58   ` shuah

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAHS8izOnJtMFsevb1U0qiBsQsM+UxyO=1F49NKuGrZGwNAz8Yw@mail.gmail.com' \
    --to=almasrymina@google.com \
    --cc=akpm@linux-foundation.org \
    --cc=aneesh.kumar@linux.vnet.ibm.com \
    --cc=cgroups@vger.kernel.org \
    --cc=gthelen@google.com \
    --cc=khalid.aziz@oracle.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-kselftest@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=mike.kravetz@oracle.com \
    --cc=mkoutny@suse.com \
    --cc=rientjes@google.com \
    --cc=shakeelb@google.com \
    --cc=shuah@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-kselftest Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-kselftest/0 linux-kselftest/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-kselftest linux-kselftest/ https://lore.kernel.org/linux-kselftest \
		linux-kselftest@vger.kernel.org
	public-inbox-index linux-kselftest

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kselftest


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git