| Message ID | 20230911080233.1305942-1-liwang@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,1/2] lib: add support for kinds of hpsize reservation | expand |
Hi Cyril,
[Please hold off on merging this patch]
The hesitating part of this method (from myself) is the new field
'hp->hpsize'.
It seems not wise to leave it to users to fill the gigantic page size
manually,
as some arches support different huge/gigantic page sizes:
x86_64 and x86: 2MB and 1GB.
PowerPC: ranging from 64KB to 16GB.
ARM64: 2MB and 1GB.
IA-64 (Itanium): from 4KB to 256MB.
we probably need a intelengent way to detect and reserve whatever
hugepage or gitantic-page that all acmplish that in ltp-library or setup().
Then people don't need to fill any byte which avoiding typo or
something wrong.
What I can think of the improved way is to extend the hugepage policy
with "_G" subfix to specified the gigantic pages.
Is this sounds better? What do you think?
Something drafted base on my patch V2:
--- a/include/tst_hugepage.h
+++ b/include/tst_hugepage.h
@@ -20,14 +20,15 @@ extern char *nr_opt; /* -s num Set the number of the
been allocated hugepages
extern char *Hopt; /* -H /.. Location of hugetlbfs, i.e. -H
/var/hugetlbfs */
enum tst_hp_policy {
- TST_REQUEST,
- TST_NEEDS,
+ TST_REQUEST_H = 0x0,
+ TST_REQUEST_G = 0x1,
+ TST_NEEDS_H = 0x2,
+ TST_NEEDS_G = 0x4,
};
struct tst_hugepage {
const unsigned long number;
enum tst_hp_policy policy;
- const unsigned long hpsize;
};
/*
@@ -35,6 +36,11 @@ struct tst_hugepage {
*/
size_t tst_get_hugepage_size(void);
+/*
+ * Get the gigantic hugepage size. Returns 0 if hugepages are not
supported.
+ */
+size_t tst_get_gigantic_size(void);
+
/*
* Try the best to request a specified number of huge pages from system,
* it will store the reserved hpage number in tst_hugepages.
diff --git a/lib/tst_hugepage.c b/lib/tst_hugepage.c
index f4b18bbbf..568884fbb 100644
--- a/lib/tst_hugepage.c
+++ b/lib/tst_hugepage.c
@@ -21,6 +21,30 @@ size_t tst_get_hugepage_size(void)
return SAFE_READ_MEMINFO("Hugepagesize:") * 1024;
}
+/* Check if hugetlb page is gigantic */
+static inline int is_hugetlb_gigantic(unsigned long hpage_size)
+{
+ return (hpage_size / getpagesize()) >> 11;
+}
+
+size_t tst_get_gigantic_size(void)
+{
+ DIR *dir;
+ struct dirent *ent;
+ unsigned long g_hpage_size;
+
+ dir = SAFE_OPENDIR(PATH_HUGEPAGES);
+ while ((ent = SAFE_READDIR(dir))) {
+ if ((sscanf(ent->d_name, "hugepages-%lukB", &g_hpage_size)
== 1) &&
+ is_hugetlb_gigantic(g_hpage_size * 1024)) {
+ break;
+ }
+ }
+
+ SAFE_CLOSEDIR(dir);
+ return g_hpage_size * 1024;
+}
+
unsigned long tst_reserve_hugepages(struct tst_hugepage *hp)
{
unsigned long val, max_hpages, hpsize;
@@ -43,10 +67,10 @@ unsigned long tst_reserve_hugepages(struct tst_hugepage
*hp)
else
tst_hugepages = hp->number;
- if (hp->hpsize)
- hpsize = hp->hpsize;
+ if (hp->policy & (TST_NEEDS_G | TST_REQUEST_G))
+ hpsize = tst_get_gigantic_size() / 1024;
else
- hpsize = SAFE_READ_MEMINFO(MEMINFO_HPAGE_SIZE);
+ hpsize = tst_get_hugepage_size() / 1024;
sprintf(hugepage_path,
PATH_HUGEPAGES"/hugepages-%lukB/nr_hugepages", hpsize);
if (access(hugepage_path, F_OK)) {
Hello, Li Wang <liwang@redhat.com> writes: > Hi Cyril, > > [Please hold off on merging this patch] > > The hesitating part of this method (from myself) is the new field > 'hp->hpsize'. > It seems not wise to leave it to users to fill the gigantic page size > manually, > as some arches support different huge/gigantic page sizes: Yes, good idea. > > x86_64 and x86: 2MB and 1GB. > PowerPC: ranging from 64KB to 16GB. > ARM64: 2MB and 1GB. > IA-64 (Itanium): from 4KB to 256MB. > > we probably need a intelengent way to detect and reserve whatever > hugepage or gitantic-page that all acmplish that in ltp-library or setup(). > Then people don't need to fill any byte which avoiding typo or > something wrong. It seems like a special flag is needed in mmap if you want to allocate a gigantic page other than 1GB? > > What I can think of the improved way is to extend the hugepage policy > with "_G" subfix to specified the gigantic pages. > > Is this sounds better? What do you think? > > Something drafted base on my patch V2: > > --- a/include/tst_hugepage.h > +++ b/include/tst_hugepage.h > @@ -20,14 +20,15 @@ extern char *nr_opt; /* -s num Set the number of the > been allocated hugepages > extern char *Hopt; /* -H /.. Location of hugetlbfs, i.e. -H > /var/hugetlbfs */ > > enum tst_hp_policy { > - TST_REQUEST, > - TST_NEEDS, > + TST_REQUEST_H = 0x0, > + TST_REQUEST_G = 0x1, > + TST_NEEDS_H = 0x2, > + TST_NEEDS_G = 0x4, > }; > > struct tst_hugepage { > const unsigned long number; > enum tst_hp_policy policy; > - const unsigned long hpsize; > }; Why not keep hpsize and add enum tst_hp_size { TST_HUGE, TST_GIGANTIC }? In theory more sizes can be added. > > /* > @@ -35,6 +36,11 @@ struct tst_hugepage { > */ > size_t tst_get_hugepage_size(void); > > +/* > + * Get the gigantic hugepage size. Returns 0 if hugepages are not > supported. > + */ > +size_t tst_get_gigantic_size(void); > + > /* > * Try the best to request a specified number of huge pages from system, > * it will store the reserved hpage number in tst_hugepages. > diff --git a/lib/tst_hugepage.c b/lib/tst_hugepage.c > index f4b18bbbf..568884fbb 100644 > --- a/lib/tst_hugepage.c > +++ b/lib/tst_hugepage.c > @@ -21,6 +21,30 @@ size_t tst_get_hugepage_size(void) > return SAFE_READ_MEMINFO("Hugepagesize:") * 1024; > } > > +/* Check if hugetlb page is gigantic */ > +static inline int is_hugetlb_gigantic(unsigned long hpage_size) > +{ > + return (hpage_size / getpagesize()) >> 11; > +} What is 11? If it is the order or shift of hugepages then that is not constant (see below). > + > +size_t tst_get_gigantic_size(void) > +{ > + DIR *dir; > + struct dirent *ent; > + unsigned long g_hpage_size; > + > + dir = SAFE_OPENDIR(PATH_HUGEPAGES); > + while ((ent = SAFE_READDIR(dir))) { > + if ((sscanf(ent->d_name, "hugepages-%lukB", &g_hpage_size) > == 1) && > + is_hugetlb_gigantic(g_hpage_size * 1024)) { > + break; > + } > + } I guess in theory more gigantic page sizes could be added. I'm not sure what size we should pick, but we don't want it to be random because it would make debugging more difficult. So could we search for the smallest size (hugepagesize) and second smallest (smallest gigantic page)? > + > + SAFE_CLOSEDIR(dir); > + return g_hpage_size * 1024; > +} > + > unsigned long tst_reserve_hugepages(struct tst_hugepage *hp) > { > unsigned long val, max_hpages, hpsize; > @@ -43,10 +67,10 @@ unsigned long tst_reserve_hugepages(struct tst_hugepage > *hp) > else > tst_hugepages = hp->number; > > - if (hp->hpsize) > - hpsize = hp->hpsize; > + if (hp->policy & (TST_NEEDS_G | TST_REQUEST_G)) > + hpsize = tst_get_gigantic_size() / 1024; > else > - hpsize = SAFE_READ_MEMINFO(MEMINFO_HPAGE_SIZE); > + hpsize = tst_get_hugepage_size() / 1024; > > sprintf(hugepage_path, > PATH_HUGEPAGES"/hugepages-%lukB/nr_hugepages", hpsize); > if (access(hugepage_path, F_OK)) { > > > > > -- > Regards, > Li Wang
On Wed, Oct 18, 2023 at 6:09 PM Richard Palethorpe <rpalethorpe@suse.de> wrote: > Hello, > > Li Wang <liwang@redhat.com> writes: > > > Hi Cyril, > > > > [Please hold off on merging this patch] > > > > The hesitating part of this method (from myself) is the new field > > 'hp->hpsize'. > > It seems not wise to leave it to users to fill the gigantic page size > > manually, > > as some arches support different huge/gigantic page sizes: > > Yes, good idea. > > > > > x86_64 and x86: 2MB and 1GB. > > PowerPC: ranging from 64KB to 16GB. > > ARM64: 2MB and 1GB. > > IA-64 (Itanium): from 4KB to 256MB. > > > > we probably need a intelengent way to detect and reserve whatever > > hugepage or gitantic-page that all acmplish that in ltp-library or > setup(). > > Then people don't need to fill any byte which avoiding typo or > > something wrong. > > It seems like a special flag is needed in mmap if you want to allocate a > gigantic page other than 1GB? > Right, MAP_HUGE_2MB or MAP_HUGE_1GB used in conjunction with MAP_HUGETLB to select alternative hugetlb page sizes. But no matter which one to use, we should reserve enough memory size for them in "HugePages_Free:". That's what the ".hugepages" are doing in LTP library. > > > > > What I can think of the improved way is to extend the hugepage policy > > with "_G" subfix to specified the gigantic pages. > > > > Is this sounds better? What do you think? > > > > Something drafted base on my patch V2: > > > > --- a/include/tst_hugepage.h > > +++ b/include/tst_hugepage.h > > @@ -20,14 +20,15 @@ extern char *nr_opt; /* -s num Set the number of > the > > been allocated hugepages > > extern char *Hopt; /* -H /.. Location of hugetlbfs, i.e. -H > > /var/hugetlbfs */ > > > > enum tst_hp_policy { > > - TST_REQUEST, > > - TST_NEEDS, > > + TST_REQUEST_H = 0x0, > > + TST_REQUEST_G = 0x1, > > + TST_NEEDS_H = 0x2, > > + TST_NEEDS_G = 0x4, > > }; > > > > struct tst_hugepage { > > const unsigned long number; > > enum tst_hp_policy policy; > > - const unsigned long hpsize; > > }; > > Why not keep hpsize and add enum tst_hp_size { TST_HUGE, TST_GIGANTIC }? > Good advice! I think maybe the 'enum tst_hp_type' is better? This sounds like the proper type of hugepages we choose for the test. enum tst_hp_policy { TST_REQUEST, TST_NEEDS, }; enum tst_hp_type { TST_HUGEPAGE, TST_GIGANTIC, }; struct tst_hugepage { const unsigned long number; enum tst_hp_policy policy; enum tst_hp_type hptype; }; ... static struct tst_test test = { .needs_root = 1, ... .hugepages = {2, TST_NEEDS, TST_GIGANTIC}, }; > > In theory more sizes can be added. > > > > > /* > > @@ -35,6 +36,11 @@ struct tst_hugepage { > > */ > > size_t tst_get_hugepage_size(void); > > > > +/* > > + * Get the gigantic hugepage size. Returns 0 if hugepages are not > > supported. > > + */ > > +size_t tst_get_gigantic_size(void); > > + > > /* > > * Try the best to request a specified number of huge pages from system, > > * it will store the reserved hpage number in tst_hugepages. > > diff --git a/lib/tst_hugepage.c b/lib/tst_hugepage.c > > index f4b18bbbf..568884fbb 100644 > > --- a/lib/tst_hugepage.c > > +++ b/lib/tst_hugepage.c > > @@ -21,6 +21,30 @@ size_t tst_get_hugepage_size(void) > > return SAFE_READ_MEMINFO("Hugepagesize:") * 1024; > > } > > > > +/* Check if hugetlb page is gigantic */ > > +static inline int is_hugetlb_gigantic(unsigned long hpage_size) > > +{ > > + return (hpage_size / getpagesize()) >> 11; > > +} > > What is 11? If it is the order or shift of hugepages then that is not > constant (see below). > This function is extracted from mem/hugetlb/lib/hugetlb.h. I think that 11 means, the gigantic-pgsize is not less than 2048 general pgsize. But you're right it can't cover all situations. > > > + > > +size_t tst_get_gigantic_size(void) > > +{ > > + DIR *dir; > > + struct dirent *ent; > > + unsigned long g_hpage_size; > > + > > + dir = SAFE_OPENDIR(PATH_HUGEPAGES); > > + while ((ent = SAFE_READDIR(dir))) { > > + if ((sscanf(ent->d_name, "hugepages-%lukB", > &g_hpage_size) > > == 1) && > > + is_hugetlb_gigantic(g_hpage_size * 1024)) { > > + break; > > + } > > + } > > I guess in theory more gigantic page sizes could be added. I'm not sure > what size we should pick, but we don't want it to be random because it > would make debugging more difficult. > > So could we search for the smallest size (hugepagesize) and second > smallest (smallest gigantic page)? > Yes, we could have a try on this way. To intelligently judge and pick the proper size for gigantic or huge page, which should work for most mainstream platforms(and TCONF on no-supported systems). > > > + > > + SAFE_CLOSEDIR(dir); > > + return g_hpage_size * 1024; > > +} > > + > > unsigned long tst_reserve_hugepages(struct tst_hugepage *hp) > > { > > unsigned long val, max_hpages, hpsize; > > @@ -43,10 +67,10 @@ unsigned long tst_reserve_hugepages(struct > tst_hugepage > > *hp) > > else > > tst_hugepages = hp->number; > > > > - if (hp->hpsize) > > - hpsize = hp->hpsize; > > + if (hp->policy & (TST_NEEDS_G | TST_REQUEST_G)) > > + hpsize = tst_get_gigantic_size() / 1024; > > else > > - hpsize = SAFE_READ_MEMINFO(MEMINFO_HPAGE_SIZE); > > + hpsize = tst_get_hugepage_size() / 1024; > > > > sprintf(hugepage_path, > > PATH_HUGEPAGES"/hugepages-%lukB/nr_hugepages", hpsize); > > if (access(hugepage_path, F_OK)) { > > > > > > > > > > -- > > Regards, > > Li Wang > > > -- > Thank you, > Richard. > >
diff --git a/doc/c-test-api.txt b/doc/c-test-api.txt index f692909e2..ec728cf8a 100644 --- a/doc/c-test-api.txt +++ b/doc/c-test-api.txt @@ -2034,9 +2034,15 @@ For full documentation see the comments in 'include/tst_fuzzy_sync.h'. ~~~~~~~~~~~~~~~~~~~~~~~~ Many of the LTP tests need to use hugepage in their testing, this allows the -test can reserve hugepages from system via '.hugepages = {xx, TST_REQUEST}'. +test can reserve specify size of hugepages from system via: + '.hugepages = {xx, TST_REQUEST, yy}'. -We achieved two policies for reserving hugepages: +xx: This is used to set how many pages we wanted. + +yy: This is used to specify the size of the hugepage. + (If not set it will use the default hugepage size) + +Two policies for reserving hugepages: TST_REQUEST: It will try the best to reserve available huge pages and return the number @@ -2103,6 +2109,34 @@ struct tst_test test = { }; ------------------------------------------------------------------------------- +or, + +[source,c] +------------------------------------------------------------------------------- +#include "tst_test.h" +#define GIGANTIC_PGSZ 1024 * 1024 + +static void run(void) +{ + ... +} + +static void setup(void) +{ + /* + * Specify hpsize reserved automatically in the library + * $ echo 2 > /sys/kernel/mm//hugepages/hugepages-1048576kB/nr_hugepages + * Do check if 2 hpages are reserved correctly in there + */ +} + +struct tst_test test = { + .test_all = run, + .hugepages = {2, TST_NEEDS, GIGANTIC_PGSZ}, + ... +}; +------------------------------------------------------------------------------- + 1.35 Checking for required commands ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ diff --git a/include/tst_hugepage.h b/include/tst_hugepage.h index 46327c79a..e96bfbe53 100644 --- a/include/tst_hugepage.h +++ b/include/tst_hugepage.h @@ -27,6 +27,7 @@ enum tst_hp_policy { struct tst_hugepage { const unsigned long number; enum tst_hp_policy policy; + const unsigned long hpsize; }; /* diff --git a/lib/tst_hugepage.c b/lib/tst_hugepage.c index d2e70a955..f4b18bbbf 100644 --- a/lib/tst_hugepage.c +++ b/lib/tst_hugepage.c @@ -5,6 +5,7 @@ #define TST_NO_DEFAULT_MAIN +#include <stdio.h> #include "tst_test.h" #include "tst_hugepage.h" @@ -22,9 +23,10 @@ size_t tst_get_hugepage_size(void) unsigned long tst_reserve_hugepages(struct tst_hugepage *hp) { - unsigned long val, max_hpages; + unsigned long val, max_hpages, hpsize; + char hugepage_path[PATH_MAX]; struct tst_path_val pvl = { - .path = PATH_NR_HPAGES, + .path = hugepage_path, .val = NULL, .flags = TST_SR_SKIP_MISSING | TST_SR_TCONF_RO }; @@ -41,6 +43,19 @@ unsigned long tst_reserve_hugepages(struct tst_hugepage *hp) else tst_hugepages = hp->number; + if (hp->hpsize) + hpsize = hp->hpsize; + else + hpsize = SAFE_READ_MEMINFO(MEMINFO_HPAGE_SIZE); + + sprintf(hugepage_path, PATH_HUGEPAGES"/hugepages-%lukB/nr_hugepages", hpsize); + if (access(hugepage_path, F_OK)) { + if (hp->policy == TST_NEEDS) + tst_brk(TCONF, "Hugepage size %lu not supported", hpsize); + tst_hugepages = 0; + goto out; + } + if (hp->number == TST_NO_HUGEPAGES) { tst_hugepages = 0; goto set_hugepages; @@ -53,7 +68,7 @@ unsigned long tst_reserve_hugepages(struct tst_hugepage *hp) goto set_hugepages; } - max_hpages = SAFE_READ_MEMINFO("MemFree:") / SAFE_READ_MEMINFO("Hugepagesize:"); + max_hpages = SAFE_READ_MEMINFO("MemFree:") / hpsize; if (tst_hugepages > max_hpages) { tst_res(TINFO, "Requested number(%lu) of hugepages is too large, " "limiting to 80%% of the max hugepage count %lu", @@ -66,22 +81,26 @@ unsigned long tst_reserve_hugepages(struct tst_hugepage *hp) set_hugepages: tst_sys_conf_save(&pvl); - SAFE_FILE_PRINTF(PATH_NR_HPAGES, "%lu", tst_hugepages); - SAFE_FILE_SCANF(PATH_NR_HPAGES, "%lu", &val); + + SAFE_FILE_PRINTF(hugepage_path, "%lu", tst_hugepages); + SAFE_FILE_SCANF(hugepage_path, "%lu", &val); + if (val != tst_hugepages) tst_brk(TCONF, "nr_hugepages = %lu, but expect %lu. " "Not enough hugepages for testing.", val, tst_hugepages); if (hp->policy == TST_NEEDS) { - unsigned long free_hpages = SAFE_READ_MEMINFO("HugePages_Free:"); + unsigned long free_hpages; + sprintf(hugepage_path, PATH_HUGEPAGES"/hugepages-%lukB/free_hugepages", hpsize); + SAFE_FILE_SCANF(hugepage_path, "%lu", &free_hpages); if (hp->number > free_hpages) tst_brk(TCONF, "free_hpages = %lu, but expect %lu. " "Not enough hugepages for testing.", free_hpages, hp->number); } - tst_res(TINFO, "%lu hugepage(s) reserved", tst_hugepages); + tst_res(TINFO, "%lu (%luMB) hugepage(s) reserved", tst_hugepages, hpsize/1024); out: return tst_hugepages; }
Typically when we make use of huge page via LTP library, .hugepages choose the default hugepage size, but this can not satisfy all scenarios. So this patch introduces applying a specified hpsize huge page for user. There is nothing that needs to change for the existing test cases which already using .hugepages, it only needs to fill one more field in the structure of .hugepages if a different hpsize is required. e.g. static struct tst_test test = { .needs_root = 1, ... .hugepages = {2, TST_NEEDS, 1024 * 1024}, }; Signed-off-by: Li Wang <liwang@redhat.com> --- Notes: V1 --> V2 * check if the specific hugepage size exist or not * guarantee the HugePages_Free-$(size)kB as expected * print the reserved hugepage size doc/c-test-api.txt | 38 ++++++++++++++++++++++++++++++++++++-- include/tst_hugepage.h | 1 + lib/tst_hugepage.c | 33 ++++++++++++++++++++++++++------- 3 files changed, 63 insertions(+), 9 deletions(-)