Re: hugemmap: Migrate alloc-instantiate-race test from libhugetlbfs

On 19/08/26 1:57 am, [email protected] wrote:
> Hi Samir,
>
> On August 18, 2026, Samir Mulani wrote:
>> hugemmap: Migrate alloc-instantiate-race test from libhugetlbfs
>> Migrate the alloc-instantiate-race.c test from libhugetlbfs [1] to LTP
>> as hugemmap36.
> Could this be corrected to hugemmap42, which is the test added by this
> patch?
Yes, fixed the commit message. It now correctly says hugemmap42. updated 
in patch v7
>> +hugemmap42 hugemmap42
> Could a second runtest entry exercise "-m private"? Without an option,
> setup() defaults to MAP_SHARED, so the new pthread path is not run by the
> hugetlb suite.
Yes, added a second entry in runtest/hugetlb: updated in patch v7
>> +	err = sched_setaffinity(getpid(), mask_size, cpuset);
> Could this pass 0 as the pid? sched_setaffinity() applies the mask to the
> thread ID supplied in pid. In the MAP_PRIVATE path, getpid() identifies the
> thread-group leader for both pthreads, so both racers change the main
> thread's affinity and remain unpinned.
Yes, fixed. Changed getpid() to 0.
With getpid(), both pthreads return the same thread-group ID, so both 
were pinning the main thread instead of themselves. Using 0 means "apply 
to the calling thread", which is correct for both the fork and pthread 
cases.
>> +	p_sync = SAFE_MMAP(NULL, (totpages - 1) * hpage_size,
>> +			   PROT_READ | PROT_WRITE, MAP_SHARED, fd_sync, 0);
>> +
>> +	run_race(race_type);
> Could each hugepage in p_sync be written before run_race()? mmap() without
> MAP_POPULATE does not fault these pages in. Consequently all free hugepages
> remain available to the racers instead of only the final page, and the
> allocation race is not exercised. The source test explicitly touches each
> page for this reason.
Yes, fixed. Added a loop that writes one byte to each sync page before 
calling run_race():

for (unsigned long i = 0; i < totpages - 1; i++) {
     volatile char *cp = (volatile char *)p_sync + i * hpage_size;
     *cp = 0;
}
Without this, mmap() does not actually take hugepages from the pool (no 
MAP_POPULATE), so all pages were still free when the race started and 
the race condition was never triggered. This was the most important fix.
>> +	if (p_sync != MAP_FAILED) {
>> +		unsigned long totpages = SAFE_READ_MEMINFO(MEMINFO_HPAGE_FREE);
>> +
>> +		SAFE_MUNMAP(p_sync, totpages * tst_get_hugepage_size());
>> +	}
> Could the exact length passed to mmap() be saved and reused here? The
> current free-page count is not the mapping length. On an abort with the
> current code it is one page larger, and munmap() may remove an adjacent
> mapping. After the pages are faulted in, a pre-existing hugepage pool can
> instead make it smaller and leave part of p_sync mapped.
Yes, fixed. Added a global sync_mapping_size, set it at mmap() time
and reused it in both run_test() and cleanup(). This avoids using
SAFE_READ_MEMINFO() in cleanup() which can return a different value
than what was actually mapped
>> +		{NULL, NULL, NULL}
> Could this use the standard empty sentinel "{}"? check-hugemmap42 reports
> LTP-005 for this options array.
Yes, fixed. Changed {NULL, NULL, NULL} to {}.
>
> 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

v7 patch will follow shortly.

Regards,
Samir

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