Re: [LTP] [PATCH v3 1/3] lib: add support for kinds of hpsize reservation

[Petr Vorel <pvorel@suse.cz>]

Hi Li,

> 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 types of hugepage 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 type (GIGANTIC or HUGE) is required.

> e.g.

>     static struct tst_test test = {
> 	.needs_root = 1,
> 	...
> 	.hugepages = {2, TST_NEEDS, TST_GIGANTIC},
>     };

+1

Algorithm looks good to me.
It would be nice to add a new test or to modify at least one of the library
tests to use some of the new functionality.

Below only very minor formatting notes.

Reviewed-by: Petr Vorel <pvorel@suse.cz>

> Signed-off-by: Li Wang <liwang@redhat.com>
> ---
>  doc/C-Test-API.asciidoc | 42 +++++++++++++++++++++++++--
>  include/tst_hugepage.h  | 11 +++++++
>  lib/tst_hugepage.c      | 63 ++++++++++++++++++++++++++++++++++++-----
>  3 files changed, 107 insertions(+), 9 deletions(-)

> diff --git a/doc/C-Test-API.asciidoc b/doc/C-Test-API.asciidoc
> index dab811564..82a1866d3 100644
> --- a/doc/C-Test-API.asciidoc
> +++ b/doc/C-Test-API.asciidoc
> @@ -2034,9 +2034,13 @@ 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, TST_HUGE}'  or,
> +  '.hugepages = {xx, TST_NEEDS,   TST_GIGANTIC}'.
very nit: this formats in wiki as inline. If you meant to have the previous following
showing also as a list, it would have to be:

test can reserve specify size of hugepages from system via:

* '.hugepages = {xx, TST_REQUEST, TST_HUGE}'  or,
* '.hugepages = {xx, TST_NEEDS,   TST_GIGANTIC}'.

very nit: maybe N instead of XX?

> -We achieved two policies for reserving hugepages:
> +xx: This is used to set how many pages we wanted.
> +
> +Two policies for reserving hugepage:

>  TST_REQUEST:
>    It will try the best to reserve available huge pages and return the number
> @@ -2049,6 +2053,17 @@ TST_NEEDS:
>    use these specified numbers correctly. Otherwise, test exits with TCONF if
>    the attempt to reserve hugepages fails or reserves less than requested.

> +Two types of the reserved hugepage (optional field):
> +
> +TST_HUGE:
> +  It means target for reserve the default hugepage size (e.g. 2MB on x86_64).
> +  And, if nothing fills in this field LTP also chooses the default hugepage
> +  size to reserve. i.e.
> +  '.hugepages = {xx, TST_REQUEST}' == '.hugepages = {xx, TST_REQUEST, TST_HUGE}'
> +
> +TST_GIGANTIC:
> +  It means target for reserve the largest hugepage size (e.g. 1GB on x86_64)
very nit: missing '.' at the end.

> +
...
> +or,
> +
> +[source,c]
> +-------------------------------------------------------------------------------
> +#include "tst_test.h"
> +
> +static void run(void)
> +{
> +	...
> +}
> +
> +struct tst_test test = {
> +	.test_all = run,
> +	/*
> +	 * Specify gigantic page sizes 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.
Maybe my bad English, but this looked to me at first as imperative, e.g.:
"Hey, test author, do check the value!" Maybe something like "Library checks
whether 2 hpages is actually set.".

> +	 */
> +	.hugepages = {2, TST_NEEDS, TST_GIGANTIC},
> +	...
> +};
> +-------------------------------------------------------------------------------
> +
>  1.35 Checking for required commands
>  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

> diff --git a/include/tst_hugepage.h b/include/tst_hugepage.h
> index 46327c79a..725b4ddaf 100644
> --- a/include/tst_hugepage.h
> +++ b/include/tst_hugepage.h
> @@ -24,9 +24,15 @@ enum tst_hp_policy {
>  	TST_NEEDS,
>  };

> +enum tst_hp_type {
> +	TST_HUGE,
> +	TST_GIGANTIC,
> +};
> +
>  struct tst_hugepage {
>  	const unsigned long number;
>  	enum  tst_hp_policy policy;
> +	enum  tst_hp_type   hptype;
>  };

>  /*
> @@ -34,6 +40,11 @@ struct tst_hugepage {
>   */
>  size_t tst_get_hugepage_size(void);

> +/*
> + * Get the largest hugepage (gigantic) size. Returns 0 if hugepages are not supported.
> + */
> +size_t tst_get_gigantic_size(void);
..
> +++ 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"

> @@ -20,11 +21,35 @@ size_t tst_get_hugepage_size(void)
>  	return SAFE_READ_MEMINFO("Hugepagesize:") * 1024;
>  }

> +size_t tst_get_gigantic_size(void)
> +{
> +	DIR *dir;
> +	struct dirent *ent;
> +	unsigned long max, g_pgsz;
> +
> +	max = tst_get_hugepage_size() / 1024;
> +
> +	/*
> +	 * Scanning the largest hugepage sisze, for example aarch64 configuration:
nit: s/sisze/size/

> +	 * hugepages-1048576kB hugepages-32768kB hugepages-2048kB hugepages-64kB
> +	 */
> +	dir = SAFE_OPENDIR(PATH_HUGEPAGES);
> +	while ((ent = SAFE_READDIR(dir)) != NULL) {
> +		if (sscanf(ent->d_name, "hugepages-%lukB", &g_pgsz)
> +				&& (g_pgsz > max))
> +			max = g_pgsz;
> +	}
> +
> +	SAFE_CLOSEDIR(dir);
> +	return max * 1024;
> +}
...

The rest LGTM.

Kind regards,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp