[LTP] [RFC PATCH 1/2] lib: add support for kinds of hpsize reservation
Li Wang
liwang@redhat.com
Thu Sep 7 14:37:33 CEST 2023
Hi Cyril,
On Thu, Sep 7, 2023 at 5:25 PM Cyril Hrubis <chrubis@suse.cz> wrote:
> Hi!
> > 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, 1048576},
> > };
> >
> > Signed-off-by: Li Wang <liwang@redhat.com>
> > ---
> > doc/c-test-api.txt | 37 +++++++++++++++++++++++++++++++++++--
> > include/tst_hugepage.h | 1 +
> > lib/tst_hugepage.c | 23 +++++++++++++++++------
> > 3 files changed, 53 insertions(+), 8 deletions(-)
> >
> > diff --git a/doc/c-test-api.txt b/doc/c-test-api.txt
> > index dcb6e1ba8..45610474c 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,33 @@ struct tst_test test = {
> > };
> >
> -------------------------------------------------------------------------------
> >
> > +or,
> > +
> > +[source,c]
> >
> +-------------------------------------------------------------------------------
> > +#include "tst_test.h"
> > +
> > +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, 1048576},
> > + ...
> > +};
> >
> +-------------------------------------------------------------------------------
> > +
> > 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 728a8c3ec..b6706d412 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,
>
> This pointer (hugepage_path) is on stack and is passed to a function
> that stores the structure into a list and then uses it to restore the
> value at the end of the testrun, which will probably crash the test
> since there will be random leftovers on the stack.
>
But the function tst_sys_conf_save_str allocates a new memory
area for saving the structure info, the original is not used for
appending to the list, isn't it?
strncpy() inside that function helps avoid the problem you pointed.
>
> The array has to be static to have unlimited lifetime.
>
> > .val = NULL,
> > .flags = TST_SR_SKIP_MISSING | TST_SR_TCONF_RO
> > };
> >
> > @@ -41,6 +43,13 @@ 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);
>
> Do we check anywhere here if the path actually exists? There is no
> guarantee that specific hugepage size will exist on the system, so I
> suppose that we have to do:
>
+1 good advice!
I previously thought to use .arch to guarantee that a specific path exists
but it obviously a stupid way in patch2/2.
> 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;
> > @@ -52,7 +61,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",
> > @@ -65,14 +74,16 @@ 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) {
> > + if ((hp->policy == TST_NEEDS) && (!hp->hpsize)) {
>
> This branch shouldn't be disabled for TST_NEEDS case, shouldn't there be
> HugePages_Free-$(size)kB ?
>
No, this is necessary.
Unless the kernel booting with parameter 'default_hugepagesz=1G' otherwise
there won't be any info about gigantic pages in /proc/meminfo, because Linux
only set the default hugepage size(2MB for x86_64) to
HugePages_Free-$(size)kB.
>
> > unsigned long free_hpages =
> SAFE_READ_MEMINFO("HugePages_Free:");
> > if (hp->number > free_hpages)
> > tst_brk(TCONF, "free_hpages = %lu, but expect %lu.
> "
> > --
> > 2.40.0
> >
> >
> > --
> > Mailing list info: https://lists.linux.it/listinfo/ltp
>
> --
> Cyril Hrubis
> chrubis@suse.cz
>
>
--
Regards,
Li Wang
More information about the ltp
mailing list