Re: [LTP] [PATCH v2] Hugetlb: Test to detect bug with freeing gigantic hugetlb pages

Tarun Sahu
2023-04-19T11:44:27Z

Hi Cyril,

Thanks for looking at it.
I agree to your comments, sent the v3. Also second commit tag was wrong.
I changed it too.

~Tarun

> Hi!
>> +#define _GNU_SOURCE
>> +#include <dirent.h>
>> +
>> +#include <stdio.h>
>> +
>> +#include "hugetlb.h"
>> +
>> +#define PATH_GIGANTIC_HUGEPAGE "/sys/kernel/mm/hugepages"
>> +#define GIGANTIC_MIN_ORDER 10
>> +
>> +static int org_g_hpages;
>> +static char g_hpage_path[4096];
>> +
>> +static void run_test(void)
>> +{
>> + if (FILE_PRINTF(g_hpage_path, "%d", 1))
>> + tst_brk(TCONF, "Can't update the gigantic hugepages.");
>> + SAFE_FILE_PRINTF(g_hpage_path, "%d", 0);
>> +
>> + if (tst_taint_check())
>> + tst_res(TFAIL, "Freeing Gigantic pages resulted in Bad Page State bug.");
>> + else
>> + tst_res(TPASS, "Successfully freed the gigantic hugepages");
>> +}
>> +
>> +static void setup(void)
>> +{
>> + DIR *dir;
>> + struct dirent *ent;
>> + unsigned long hpage_size;
>> +
>> + dir = SAFE_OPENDIR(PATH_GIGANTIC_HUGEPAGE);
> ^
> This is very minor, but there is
> nothing gigantic about the path, that's
> just sysfs hugepates directory, so I
> suppose that it should be just
> PATH_HUGEPAGE
>
>> + while ((ent = SAFE_READDIR(dir))) {
>> + if (strstr(ent->d_name, "hugepages-") != NULL) {
>
> Isn't the strstr() here reduntant?
>
> I as far as I can tell if the line in sscanf() will not match the call
> will simply return 0.
>
>> + if ((sscanf(ent->d_name, "hugepages-%lukB", &hpage_size) == 1) &&
>> + is_hugetlb_gigantic(hpage_size * 1024)) {
>> + sprintf(g_hpage_path, "%s/%s/%s", PATH_GIGANTIC_HUGEPAGE,
>> + ent->d_name, "nr_hugepages");
>> + break;
>> + }
>> + }
>> + }
>> + if (!g_hpage_path[0])
>> + tst_brk(TCONF, "Gigantic hugepages not supported");
>> +
>> + SAFE_CLOSEDIR(dir);
>> + SAFE_FILE_LINES_SCANF(g_hpage_path, "%d", &org_g_hpages);
>> +}
>> +
>> +static void cleanup(void)
>> +{
>> + if (g_hpage_path[0])
>> + SAFE_FILE_PRINTF(g_hpage_path, "%d", org_g_hpages);
>> +}
>> +
>> +static struct tst_test test = {
>> + .tags = (struct tst_tag[]) {
>> + {"linux-git", "ba9c1201beaa"},
>> + {"linux-git", "7118fc2906e9"},
> ^
> This has appears to be wrong. Shouldn't the
> last digit be 2 instead of 9?
>> + {}
>> + },
>> + .needs_root = 1,
>> + .setup = setup,
>> + .cleanup = cleanup,
>> + .test_all = run_test,
>> + .taint_check = TST_TAINT_B,
>> +};
>> diff --git a/testcases/kernel/mem/hugetlb/lib/hugetlb.h b/testcases/kernel/mem/hugetlb/lib/hugetlb.h
>> index 241dab708..34fe08c24 100644
>> --- a/testcases/kernel/mem/hugetlb/lib/hugetlb.h
>> +++ b/testcases/kernel/mem/hugetlb/lib/hugetlb.h
>> @@ -39,6 +39,12 @@
>> # endif
>> #endif
>>
>> +/* Check if hugetlb page is gigantic */
>> +static inline int is_hugetlb_gigantic(unsigned long hpage_size)
>> +{
>> + return (hpage_size / getpagesize()) >> 11;
>> +}
>> +
>> /*
>> * to get the lower nine permission bits
>> * from shmid_ds.ipc_perm.mode
>> --
>> 2.31.1
>>
>
> --
> Cyril Hrubis
> chrubis@suse.cz
>
> --
> Mailing list info: https://lists.linux.it/listinfo/ltp