On Mon, Jun 16, 2025 at 07:02:08PM -0700, Nicolin Chen wrote:
> On Mon, Jun 16, 2025 at 01:25:01PM -0300, Jason Gunthorpe wrote:
> > On Sun, Jun 15, 2025 at 10:02:03PM -0700, Nicolin Chen wrote:
> > > FIXTURE_TEARDOWN(iommufd_dirty_tracking)
> > > {
> > > - munmap(self->buffer, variant->buffer_size);
> > > - munmap(self->bitmap, DIV_ROUND_UP(self->bitmap_size, BITS_PER_BYTE));
> > > + unsigned long size = variant->buffer_size;
> > > +
> > > + if (variant->hugepages)
> > > + size = __ALIGN_KERNEL(variant->buffer_size, HUGEPAGE_SIZE);
> > > + munmap(self->buffer, size);
> > > + free(self->buffer);
> > > + free(self->bitmap);
> > > teardown_iommufd(self->fd, _metadata);
> >
> > munmap followed by free isn't right..
>
> You are right. I re-checked with Copilot. It says the same thing.
> I think the whole posix_memalign() + mmap() confuses me..
>
> Yet, should the bitmap pair with free() since it's allocated by a
> posix_memalign() call?
>
> > This code is using the glibc allocator to get a bunch of pages mmap'd
> > to an aligned location then replacing the pages with MAP_SHARED and
> > maybe HAP_HUGETLB versions.
>
> And I studied some use cases from Copilot. It says that, to use
> the combination of posix_memalign+mmap, we should do:
> aligned_ptr = posix_memalign(pagesize, pagesize);
> unmap(aligned_ptr, pagesize);
> mapped = mmap(aligned_ptr, pagesize, PROT_READ | PROT_WRITE,
> MAP_PRIVATE | MAP_ANONYMOUS | MAP_FIXED, -1, 0);
> munmap(mapped, pagesize);
> // No free() after munmap().
> ---breakdown---
> Before `posix_memalign()`:
> [ heap memory unused ]
>
> After `posix_memalign()`:
> [ posix_memalign() memory ] ← managed by malloc/free
> ↑ aligned_ptr
>
> After `munmap(aligned_ptr)`:
> [ unmapped memory ] ← allocator no longer owns it
Incorrect. The allocator has no idea about the munmap and munmap
doesn't disturb any of the allocator tracking structures.
> After `mmap(aligned_ptr, ..., MAP_FIXED)`:
> [ anonymous mmap region ] ← fully remapped, under your control
> ↑ mapped
> ---end---
No, this is wrong.
> It points out that the heap bookkeeping will be silently clobbered
> without the munmap() in-between (like we are doing):
Nope, doesn't work like that.
> ---breakdown---
> After `posix_memalign()`:
> [ posix_memalign() memory ] ← malloc thinks it owns this
>
> Then `mmap(aligned_ptr, ..., MAP_FIXED)`:
> [ anonymous mmap region ] ← malloc still thinks it owns this (!)
> ↑ mapped
> ---end---
Yes, this is correct and what we are doing here. The allocator always
owns it and we are just replacing the memory with a different mmap.
> It also gives a simpler solution for a memory that is not huge
> page backed but huge page aligned (our !variant->hugepage case):
> ---code---
> void *ptr;
> size_t alignment = 2 * 1024 * 1024; // or whatever HUGEPAGE_SIZE was
> size_t size = variant->buffer_size;
>
> // Step 1: Use posix_memalign to get an aligned pointer
> if (posix_memalign(&ptr, alignment, size) != 0) {
> perror("posix_memalign");
> return -1;
> }
Also no, the main point of this is to inject MAP_SHARED which
posix_memalign cannot not do.
> Also, for a huge page case, there is no need of posix_memalign():
> "Hugepages are not part of the standard heap, so allocator functions
> like posix_memalign() or malloc() don't help and can even get in the
> way."
> Instead, it suggests a cleaner version without posix_memalign():
> ---code---
> void *addr = mmap(NULL, variant->buffer_size, PROT_READ | PROT_WRITE,
> MAP_SHARED | MAP_ANONYMOUS | MAP_HUGETLB | MAP_POPULATE,
> -1, 0);
> if (addr == MAP_FAILED) { perror("mmap"); return -1; }
> ---end---
Yes, we could do this only for MAP_HUGETLB, but it doesn't help the
normal case with MAP_SHARED.
So I would leave it alone, use the version I showed.
Jason
