[LTP] [PATCH] hugepages: ensure CONFIG_COMPACTION is defined

[LTP] [PATCH] hugepages: ensure CONFIG_COMPACTION is defined

[Andrea Cervesato]

From: Andrea Cervesato <andrea.cervesato@suse.com>

Add check to ensure CONFIG_COMPACTION=y when we need to use hugepages
inside tests. The kernel configuration is used to enable usage of
`/proc/sys/vm/compact_memory` inside the LTP core library.

Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com>
---
The issue was raised by https://github.com/linux-test-project/ltp/pull/1335
that is trying to solve a problem with cachestat03.
---
 lib/tst_hugepage.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/lib/tst_hugepage.c b/lib/tst_hugepage.c
index 40ecfa162a236a8674628f8ffe7a8fd2ccd52712..8fba466b0ebb04d43542d686d6bb9173f1a14cb8 100644
--- a/lib/tst_hugepage.c
+++ b/lib/tst_hugepage.c
@@ -46,6 +46,11 @@ unsigned long tst_reserve_hugepages(struct tst_hugepage *hp)
 		goto set_hugepages;
 	}
 
+	if (access(PATH_VM_COMPACT_MEMORY, F_OK)) {
+		if (hp->policy == TST_NEEDS || hp->policy == TST_REQUEST)
+			tst_brk(TCONF, "CONFIG_COMPACTION not enabled in kernel");
+	}
+
 	SAFE_FILE_PRINTF(PATH_VM_DROP_CACHES, "3");
 	SAFE_FILE_PRINTF(PATH_VM_COMPACT_MEMORY, "1");
 	if (hp->policy == TST_NEEDS) {

---
base-commit: d6fe8d8e7a58c83b6a192fb27cd2ecf5249182f7
change-id: 20260717-fix_tst_test_hugepages-fb3c88e9716c

Best regards,
-- 
Andrea Cervesato <andrea.cervesato@suse.com>


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

Re: [LTP] [PATCH] hugepages: ensure CONFIG_COMPACTION is defined

[Andrea Cervesato via ltp]

I was not 100% sure to use kconfig or change the message, so I opted
for an hybrid approach (CONFIG_COMPACTION implies the missing file),
let me know if I should update the code or this is already ok.

Regards,
--
Andrea Cervesato
SUSE QE Automation Engineer Linux
andrea.cervesato@suse.com

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

Re: [LTP] [PATCH] hugepages: ensure CONFIG_COMPACTION is defined

[Li Wang]

Hi Andrea,

> From: Andrea Cervesato <andrea.cervesato@suse.com>

> --- a/lib/tst_hugepage.c
> +++ b/lib/tst_hugepage.c
> @@ -46,6 +46,11 @@ unsigned long tst_reserve_hugepages(struct tst_hugepage *hp)
>  		goto set_hugepages;
>  	}
>  
> +	if (access(PATH_VM_COMPACT_MEMORY, F_OK)) {
> +		if (hp->policy == TST_NEEDS || hp->policy == TST_REQUEST)
> +			tst_brk(TCONF, "CONFIG_COMPACTION not enabled in kernel");
> +	}
> +

Thanks for the patch. I think the goal is reasonable, but I'd
push back on the core assumption: compact_memory is not actually
required to reserve hugepages.

Writing 1 to /proc/sys/vm/compact_memory only triggers proactive
memory compaction. It's a best-effort hint that makes a hugepage
reservation more likely to succeed by defragmenting memory
beforehand, but the reservation itself does not depend on it.

The kernel can (and often will) still satisfy the request without
any explicit compaction step. So treating a missing compact_memory
(i.e. CONFIG_COMPACTION=n) as a hard TCONF is too strict: it skips
tests that could otherwise run perfectly fine.

Given that, I don't think we should tst_brk(TCONF) here at all.
My suggetion is not to use SAFE_* macros:

--- a/lib/tst_hugepage.c
+++ b/lib/tst_hugepage.c
@@ -47,7 +47,7 @@ unsigned long tst_reserve_hugepages(struct tst_hugepage *hp)
        }
 
        SAFE_FILE_PRINTF(PATH_VM_DROP_CACHES, "3");
-       SAFE_FILE_PRINTF(PATH_VM_COMPACT_MEMORY, "1");
+       FILE_PRINTF(PATH_VM_COMPACT_MEMORY, "1");
        if (hp->policy == TST_NEEDS) {
                tst_hugepages += SAFE_READ_MEMINFO("HugePages_Total:");
                goto set_hugepages;


-- 
Regards,
Li Wang

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

Re: [LTP] [PATCH] hugepages: ensure CONFIG_COMPACTION is defined

[Andrea Cervesato via ltp]

Hi Li,

> Writing 1 to /proc/sys/vm/compact_memory only triggers proactive
> memory compaction. It's a best-effort hint that makes a hugepage
> reservation more likely to succeed by defragmenting memory
> beforehand, but the reservation itself does not depend on it.
> 
> The kernel can (and often will) still satisfy the request without
> any explicit compaction step. So treating a missing compact_memory
> (i.e. CONFIG_COMPACTION=n) as a hard TCONF is too strict: it skips
> tests that could otherwise run perfectly fine.
> 
> Given that, I don't think we should tst_brk(TCONF) here at all.
> My suggetion is not to use SAFE_* macros:
> 
> --- a/lib/tst_hugepage.c
> +++ b/lib/tst_hugepage.c
> @@ -47,7 +47,7 @@ unsigned long tst_reserve_hugepages(struct tst_hugepage *hp)
>         }
>  
>         SAFE_FILE_PRINTF(PATH_VM_DROP_CACHES, "3");
> -       SAFE_FILE_PRINTF(PATH_VM_COMPACT_MEMORY, "1");
> +       FILE_PRINTF(PATH_VM_COMPACT_MEMORY, "1");
>         if (hp->policy == TST_NEEDS) {
>                 tst_hugepages += SAFE_READ_MEMINFO("HugePages_Total:");
>                 goto set_hugepages;
> 
> 
> -- 
> Regards,
> Li Wang

thanks for the patch, indeed I was not sure about the approach and I
think this might work. I will wait for other people to send a final
patch fixing this issue. it sounds resonable to me to follow your
approach.

Regards,
--
Andrea Cervesato
SUSE QE Automation Engineer Linux
andrea.cervesato@suse.com

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

Re: [LTP] [PATCH] hugepages: ensure CONFIG_COMPACTION is defined

[Cyril Hrubis]

Hi!
> Given that, I don't think we should tst_brk(TCONF) here at all.
> My suggetion is not to use SAFE_* macros:
> 
> --- a/lib/tst_hugepage.c
> +++ b/lib/tst_hugepage.c
> @@ -47,7 +47,7 @@ unsigned long tst_reserve_hugepages(struct tst_hugepage *hp)
>         }
>  
>         SAFE_FILE_PRINTF(PATH_VM_DROP_CACHES, "3");
> -       SAFE_FILE_PRINTF(PATH_VM_COMPACT_MEMORY, "1");
> +       FILE_PRINTF(PATH_VM_COMPACT_MEMORY, "1");

Looks good to me.

-- 
Cyril Hrubis
chrubis@suse.cz

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

Re: [LTP] [PATCH] hugepages: ensure CONFIG_COMPACTION is defined

[Petr Vorel]

Hi Andrea,

> Add check to ensure CONFIG_COMPACTION=y when we need to use hugepages
> inside tests. The kernel configuration is used to enable usage of
> `/proc/sys/vm/compact_memory` inside the LTP core library.

> The issue was raised by https://github.com/linux-test-project/ltp/pull/1335
> that is trying to solve a problem with cachestat03.
> ---
>  lib/tst_hugepage.c | 5 +++++
....
> +	if (access(PATH_VM_COMPACT_MEMORY, F_OK)) {
> +		if (hp->policy == TST_NEEDS || hp->policy == TST_REQUEST)
> +			tst_brk(TCONF, "CONFIG_COMPACTION not enabled in kernel");

I'd at least add a question mark when it's not based on a real kconfig
inspection. Because there might be different reasons for missing file in the
future.

Kind regards,
Petr

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

Re: [LTP] [PATCH] hugepages: ensure CONFIG_COMPACTION is defined

[Petr Vorel]

> Hi!
> > Given that, I don't think we should tst_brk(TCONF) here at all.
> > My suggetion is not to use SAFE_* macros:

> > --- a/lib/tst_hugepage.c
> > +++ b/lib/tst_hugepage.c
> > @@ -47,7 +47,7 @@ unsigned long tst_reserve_hugepages(struct tst_hugepage *hp)
> >         }

> >         SAFE_FILE_PRINTF(PATH_VM_DROP_CACHES, "3");
> > -       SAFE_FILE_PRINTF(PATH_VM_COMPACT_MEMORY, "1");
> > +       FILE_PRINTF(PATH_VM_COMPACT_MEMORY, "1");

> Looks good to me.

Li, +1 as well. Thanks all to handle this.

Kind regards,
Petr

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