Re: [PATCH v1 09/13] util/mmap-alloc: Implement resizable mmaps

[David Hildenbrand <david@redhat.com>]

>>      /* we can only map whole pages */
>> -    size = QEMU_ALIGN_UP(size, pagesize);
>> +    old_size = QEMU_ALIGN_UP(old_size, pagesize);
>> +    new_size = QEMU_ALIGN_UP(new_size, pagesize);
> 
> Shouldn't we just assert old_size and new_size are aligned with
> pagesize?
> 

Already reworked :)

>> +
>> +    /* we support actually resizable memory regions only on Linux */
>> +    if (old_size < new_size) {
>> +        /* populate the missing piece into the reserved area */
>> +        ptr = mmap_populate(ptr + old_size, new_size - old_size, fd,
>> old_size, +                            shared, is_pmem);
>> +    } else if (old_size > new_size) {
>> +        /* discard this piece, keeping the area reserved (should never
>> fail) */ +        ptr = mmap_reserve(ptr + new_size, old_size - new_size,
>> fd); +    }
> 
> I find the behaviour of this function somewhat confusing.  Perhaps I'm
> missing something and need your help to clarify.  Please bear with me.
> 
> For the case where we want to grow in size, it returns a populated area
> (PROT_READ | PROT_WRITE flags).
> 
> And for the case where we want to shrink in size, it returns a reserved
> area (PROT_NONE flag), requiring the caller to call mmap_populate()
> again to be able to use that memory.
> 
> I believe the behaviour should be consistent regardless if we want to
> grow or shrink in size.  Either return a reserved or an already
> populated area.  Not both.

The return value is only used to differentiate between success (!=
MAP_FAILED) and failure (== MAP_FAILED), nothing else.

For now, I switched to returning a bool instead (resized vs. not
resized), that avoids this inconsistency. See my reply to Richard's comment.

> 
> Would "old_size == new_size" situation be possible?  In this case, ptr
> would be returned without changing protection flags of the mapping.

It could, although in this patch set, it never would :) The result would
be a NOP, which is the right thing to do.

> 
> Shouldn't we also assert that new_size <= max_size?

Then we would have to pass max_size, just for asserting. I'll leave that
to the callers for now.

> 
>> +    return ptr;
>> +}
>> +
>> +void qemu_ram_munmap(int fd, void *ptr, size_t max_size)
>> +{
>> +    const size_t pagesize = mmap_pagesize(fd);
>> +
>> +    /* we can only map whole pages */
>> +    max_size = QEMU_ALIGN_UP(max_size, pagesize);
> 
> Shouldn't we just assert this and leave the alignment to the caller?

Already reworked.

Thanks!

-- 
Thanks,

David / dhildenb