Re: [LTP] min_free_kbytes: Handle transient memory drops in check_monitor

[linuxtestproject.agent@gmail.com]

Hi Wei,

On Wed, 29 Jul 2026, Wei Gao wrote:
> min_free_kbytes: Handle transient memory drops in check_monitor

> Implement a 2-second grace period with high-accuracy 10ms fixed polling
> in check_monitor() to allow the kernel time to reclaim memory.
>
> Introduce a 10% tolerance (90% threshold) for the MemFree check.

These are two independent changes, and the second one is much more
invasive than the first. Could they be split into separate patches?

The idle poll interval change and the diagnostics are two more logical
changes in the same commit. Splitting them would let the tolerance be
discussed (or reverted) on its own.

> +		threshold = tune * 9 / 10;

> +			if (memfree < threshold) {
> +				tst_res(TFAIL, "MemFree %lu kB < 90%% of min_free_kbytes %lu kB (MinSeen: %lu%%) after 2s",
> +					memfree, tune, (min_memfree * 100 / tune));
> +				violated = 1;
> +			} else if (memfree < tune) {
> +				tst_res(TINFO, "MemFree (%lu kB) stayed within 10%% tolerance (min %lu%%) after ~2s",
> +					memfree, (min_memfree * 100 / tune));

With this, a kernel that steadily keeps MemFree at, say, 0.95 *
min_free_kbytes forever never fails: the loop runs its full 2s, the
value is above threshold, only TINFO is printed and the test ends with
TPASS.

Isn't that the exact regression this test exists to catch? The kernel
contract is MemFree >= min_free_kbytes, and the tolerance turns a
permanent violation into an informational message.

The 2s grace period already covers the transient case described in the
commit message. What does the tolerance add on top of it that the grace
period does not?

If the concern is the slow recovery tail, would extending the grace
period (or retrying with a longer budget) be preferable to lowering the
watermark the test enforces?

> -		sleep(2);
> +		usleep(100000);

The commit message says this improves "responsiveness". What does the
monitor gain from it?

Sampling 10x more often finds more transient dips, which is what the
grace period and the tolerance in this same patch are trying to
suppress. The two changes seem to pull in opposite directions.

It also makes the child read /proc/meminfo and the min_free_kbytes
sysctl 20 times per second while the parent is deliberately driving the
machine into memory pressure, so the monitor adds load to what it is
measuring.

> +			for (i = 10; i <= 2000; i += 10) {
> +				usleep(10000);
> +				memfree = SAFE_READ_MEMINFO("MemFree:");

The loop does not look at "end". min_free_kbytes_test() sends SIGUSR1
right after test_tune() returns, so if the signal lands while this loop
is running the child keeps sampling for up to 2s more and can report
TFAIL for a sample taken after the workload has already finished.

Would adding "end" to the loop condition avoid reporting on the
post-test tail?

Also, c-tests rule 4 asks for exponential-backoff polling rather than a
fixed usleep() interval, and TST_RETRY_FN_EXP_BACKOFF() in
include/tst_common.h implements it. Fixed 10ms is what makes the
"recovered after %d ms" number meaningful, so this may be a deliberate
trade-off worth stating in a comment.

> +					memfree, tune, (min_memfree * 100 / tune));

min_memfree * 100 overflows a 32-bit unsigned long above roughly 41 GB
of free memory. The test still carries a TST_ABI32 branch, so 32-bit is
in scope. 100 * (min_memfree / tune) or a 64-bit intermediate would
avoid it.

>  * Since the tune is not too large or too little, which will
>  * lead to the system hang, the following cases are tested
>  * on all ``overcommit_memory`` policy, at the same time, compare
>  * the current free memory with the tunable value repeatedly.

The high-level description still says the free memory is compared with
the tunable value. After this patch that is no longer what happens.
Could this block be updated to mention the grace period and, if it
stays, the tolerance?

Verdict - Needs revision

Pre-existing issues, not introduced by this patch:

- "volatile int end;" is a non-static global; it could be static.
- eatup_mem() mmaps until failure and never munmaps. The child exits
  immediately after, so this is not a real leak.
- checkpatch reports pre-existing warnings on lines 36, 104 and 126.

---
Note:

The agent can sometimes produce false positives although often its
findings are genuine. If you find issues with the review, please
comment this email or ignore the suggestions.

Regards,
LTP AI Reviewer

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