Re: [PATCH v5 3/4] selftest/mm: test enable_soft_offline behaviors

[Jiaqi Yan <jiaqiyan@google.com>]

On Tue, Jun 25, 2024 at 6:54 PM Miaohe Lin <linmiaohe@huawei.com> wrote:
>
> On 2024/6/26 7:57, Jiaqi Yan wrote:
> > On Tue, Jun 25, 2024 at 12:05 AM Miaohe Lin <linmiaohe@huawei.com> wrote:
> >>
> >> On 2024/6/25 0:33, Jiaqi Yan wrote:
> >>> Add regression and new tests when hugepage has correctable memory
> >> ...
> >>> diff --git a/tools/testing/selftests/mm/hugetlb-soft-offline.c b/tools/testing/selftests/mm/hugetlb-soft-offline.c
> >>> new file mode 100644
> >>> index 000000000000..16fe52f972e2
> >>> --- /dev/null
> >>> +++ b/tools/testing/selftests/mm/hugetlb-soft-offline.c
> >>> @@ -0,0 +1,227 @@
> >>> +// SPDX-License-Identifier: GPL-2.0
> >>> +/*
> >>> + * Test soft offline behavior for HugeTLB pages:
> >>> + * - if enable_soft_offline = 0, hugepages should stay intact and soft
> >>> + *   offlining failed with EINVAL.
> >>
> >> s/failed with EINVAL/failed with EOPNOTSUPP/g
> >
> > To be fixed in v6.
> >
> >>
> >>> + * - if enable_soft_offline = 1, a hugepage should be dissolved and
> >>> + *   nr_hugepages/free_hugepages should be reduced by 1.
> >>> + *
> >>> + * Before running, make sure more than 2 hugepages of default_hugepagesz
> >>> + * are allocated. For example, if /proc/meminfo/Hugepagesize is 2048kB:
> >>> + *   echo 8 > /sys/kernel/mm/hugepages/hugepages-2048kB/nr_hugepages
> >>> + */
> >>> +
> >> ...
> >>> +static void test_soft_offline_common(int enable_soft_offline)
> >>> +{
> >>> +     int fd;
> >>> +     int expect_errno = enable_soft_offline ? 0 : EOPNOTSUPP;
> >>> +     struct statfs file_stat;
> >>> +     unsigned long hugepagesize_kb = 0;
> >>> +     unsigned long nr_hugepages_before = 0;
> >>> +     unsigned long nr_hugepages_after = 0;
> >>> +     int ret;
> >>> +
> >>> +     ksft_print_msg("Test soft-offline when enabled_soft_offline=%d\n",
> >>> +                    enable_soft_offline);
> >>> +
> >>> +     fd = create_hugetlbfs_file(&file_stat);
> >>> +     if (fd < 0) {
> >>> +             ksft_exit_fail_msg("Failed to create hugetlbfs file\n");
> >>> +             return;
> >>> +     }
> >>> +
> >>> +     hugepagesize_kb = file_stat.f_bsize / 1024;
> >>> +     ksft_print_msg("Hugepagesize is %ldkB\n", hugepagesize_kb);
> >>> +
> >>> +     if (set_enable_soft_offline(enable_soft_offline)) {
> >>> +             ksft_exit_fail_msg("Failed to set enable_soft_offline\n");
> >>
> >> Call destroy_hugetlbfs_file() in error path?
> >
> > As the counterpart of destroy_hugetlbfs_file, I think the test only
> > needs to close(fd). Will add it in v6.
> >
> >>
> >>> +             return;
> >>> +     }
> >>> +
> >>> +     if (read_nr_hugepages(hugepagesize_kb, &nr_hugepages_before) != 0) {
> >>> +             ksft_exit_fail_msg("Failed to read nr_hugepages\n");
> >>> +             return;
> >>> +     }
> >>> +
> >>> +     ksft_print_msg("Before MADV_SOFT_OFFLINE nr_hugepages=%ld\n",
> >>> +                    nr_hugepages_before);
> >>> +
> >>> +     ret = do_soft_offline(fd, 2 * file_stat.f_bsize, expect_errno);
> >>> +
> >>> +     if (read_nr_hugepages(hugepagesize_kb, &nr_hugepages_after) != 0) {
> >>> +             ksft_exit_fail_msg("Failed to read nr_hugepages\n");
> >>> +             return;
> >>> +     }
> >>> +
> >>> +     ksft_print_msg("After MADV_SOFT_OFFLINE nr_hugepages=%ld\n",
> >>> +             nr_hugepages_after);
> >>> +
> >>> +     if (enable_soft_offline) {
> >>> +             if (nr_hugepages_before != nr_hugepages_after + 1) {
> >>> +                     ksft_test_result_fail("MADV_SOFT_OFFLINE should reduced 1 hugepage\n");
> >>> +                     return;
> >>> +             }
> >>> +     } else {
> >>> +             if (nr_hugepages_before != nr_hugepages_after) {
> >>> +                     ksft_test_result_fail("MADV_SOFT_OFFLINE reduced %lu hugepages\n",
> >>> +                             nr_hugepages_before - nr_hugepages_after);
> >>> +                     return;
> >>> +             }
> >>> +     }
> >>> +
> >>> +     ksft_test_result(ret == 0,
> >>> +                      "Test soft-offline when enabled_soft_offline=%d\n",
> >>> +                      enable_soft_offline);
> >>
> >> Call destroy_hugetlbfs_file() when test finished ?
> >
> > Test can just close(fd) once nr_hugepages_after is read.
>
> I'm sorry but I can't find the code to call close(fd) after nr_hugepages_after is read.

Sorry, I meant in v6 I will add close(fd) after nr_hugepages_after.

> IMO create_hugetlbfs_file() would fail to create a new hugetlb file later if close(fd)
> is not called when testing previous enable_soft_offline = 1 testcase. Because a hugetlb
> file with same name is already there. But I might miss something.

Yes, this is an absolutely valid concern, and should be address in v6
by adding close(fd) after nr_hugepages_after.

>
> Thanks.
> .
>