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

[David Hildenbrand]

On 06.02.20 14:22, David Hildenbrand wrote:
> On 06.02.20 13:08, Richard Henderson wrote:
>> On 2/3/20 6:31 PM, David Hildenbrand wrote:
>>> +void *qemu_ram_mmap_resize(void *ptr, int fd, size_t old_size, size_t 
>>> new_size,
>>> +                           bool shared, bool is_pmem)
>>>  {
>>>      const size_t pagesize = mmap_pagesize(fd);
>>>  
>>>      /* 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);
>>> +
>>> +    /* 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);
>>> +    }
>>> +    return ptr;
>>> +}
>>
>> What does the return value indicate?
>> Is it just for != MAP_FAILED?
> 
> It indicates if resizing succeeded. In a previous version I returned an
> int via
> 
> ptr == MAP_FAILED ? -errno : 0;
> 
> 
> Populating will usually only fail because we're out of memory.
> 
> Populating and reserving *might* fail if we are out of VMAs in the
> kernel. VMA merging will make sure that the number of VMAs will not
> explode (usually 2-3 VMAs for one resizable region: populated VMA +
> Reserved VMA + Guard page VMA). But once we would be close to the VMA
> limit, it could happen - but it's highly unlikely.
> 
>> Assuming an assert isn't viable, are we better off with a boolean return?  
>> With
>> an Error **ptr?
> 
> either that or an int. What do you prefer?

I'll go with a bool for now. "return ptr != MAP_FAILED;"

The actual error will usually be -ENOMEM, so there is no real benefit in
returning it.

-- 
Thanks,

David / dhildenb