Re: [LTP] hugetlb/hugeshmat: Add hugeshmat06 migrated from libhugetlbfs shm-perms

On 20/08/26 4:03 pm, [email protected] wrote:
> Hi Samir,
>
> On Aug 20, 2026, Samir Mulani wrote:
>> hugetlb/hugeshmat: Add hugeshmat06 migrated from libhugetlbfs shm-perms
>> 			hugetlb_after = get_proc_hugetlb_kb(getpid());
>> 			if (hugetlb_after != 0) {
>> 				tst_res(TFAIL,
>> 					"Child %d: HugetlbPages after detach is %ld kB (expected 0), reservation accounting leak",
> Could this use an oracle tied to reservation state instead? In Linux 7.2,
> HugetlbPages reports mm->hugetlb_usage, which is incremented when a huge
> page is mapped and decremented when it is unmapped. The original regression
> leaked hstate->resv_huge_pages, so HugetlbPages can return to zero after
> shmdt() while that reservation remains leaked.
>
> The original test exposed this by making enough read-only attachments to
> exhaust the pool. Here, an eventual shmat() failure would also be handled by
> SAFE_SHMAT() as TBROK rather than reported as a failure of the operation
> under test. Could the original reservation-leak oracle be retained and the
> regression reported as TFAIL?

Agreed. HugetlbPages tracks the hugepage mappings associated with the
process and can return to zero after shmdt() while the underlying
resv_huge_pages reservation remains leaked.

I have retained the original reservation-exhaustion behavior and handle
the relevant shmat() failure explicitly as TFAIL instead of using
SAFE_SHMAT(), which would report it as TBROK.

Updated in v5.
>
>> #define SEGMENT_KEY	(0x82ba15ff)
>> 	shmid = SAFE_SHMGET(SEGMENT_KEY, segment_size,
>> 			    IPC_CREAT | SHM_HUGETLB | 0640);
> Could this segment use IPC_PRIVATE? IPC_CREAT without IPC_EXCL returns an
> accessible pre-existing segment for this key when it is large enough.
> setup() would then overwrite that segment, and cleanup() would remove
> another process's IPC object with IPC_RMID.

Agreed. Replaced SEGMENT_KEY with IPC_PRIVATE. The children inherit
global_shmid directly, so a fixed key is not required. The unused
SEGMENT_KEY definition has also been removed.
>
>> /*\
>>   * [Description]
> Could the deprecated [Description] header be dropped?
Agreed. Removed the deprecated [Description] tag while retaining the
test description, consistent with the convention used by other tests
in this area.
>> static struct tst_test test = {
>> 	.needs_root	 = 1,
> Could the high-level description explain why root is required, namely that
> the framework modifies the system huge-page pool to reserve pages?
I checked the other tests in the hugeshmat and hugefork directories
and none of them document needs_root in the description block. It is
a well understood LTP framework convention so I am keeping it as is.
>> +/hugetlb/hugeshmat/hugeshmat06
> Could this entry be placed in
> testcases/kernel/mem/hugetlb/hugeshmat/.gitignore instead? New test binaries
> belong in the .gitignore in their own directory.
There is no per-directory .gitignore under
testcases/kernel/mem/hugetlb/hugeshmat/. The existing hugeshmat01
through hugeshmat05 binary entries are maintained in
testcases/kernel/mem/.gitignore. I have kept the hugeshmat06 entry
there to remain consistent with the existing hugeshmat tests.
>
> Verdict - Needs revision
>
> ---
> 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
>
Regards,
Samir

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