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

On Wed, Jul 29, 2026 at 05:52:33AM +0000, [email protected] wrote:
> 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.
> 
AI is now questioning my 90% threshold implementation. To be fair, adding a 
90% threshold under overcommit_memory = 2 might be overkill. But under modes 0 and 1, 
where OOM is to be triggered, I'm not entirely confident that the test can reliably 
capture MemFree > min_free_kbytes before the process gets killed.

What do you guys think? I'd love to hear your feedback on this.

> 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?
No idea why AI think this can not improves responsiveness :(
> 
> 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.
Make sense, but in my local test it show ok.
> 
> > +			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?
I have replied in patchv4.
> 
> 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.
I have replied in patchv4.
> 
> > +					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.
can be improved
> 
> >  * 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