On 24.02.20 23:26, Peter Xu wrote:
> On Fri, Feb 21, 2020 at 05:42:01PM +0100, David Hildenbrand wrote:
>
> [...]
>
>> @@ -3160,7 +3160,13 @@ static int ram_load_postcopy(QEMUFile *f)
>> break;
>> }
>>
>> - if (!offset_in_ramblock(block, addr)) {
>> + /*
>> + * Relying on used_length is racy and can result in false
>> positives.
>> + * We might place pages beyond used_length in case RAM was
>> shrunk
>> + * while in postcopy, which is fine - trying to place via
>> + * UFFDIO_COPY/UFFDIO_ZEROPAGE will never segfault.
>> + */
>> + if (!block->host || addr >= block->postcopy_length) {
>
> I'm thinking whether we can even avoid the -ENOENT failure of
> UFFDIO_COPY. With the postcopy_length you introduced, I think it's
> the case when addr >= used_length && addr < postcopy_length, right?
> Can we skip those?
1. Recall that any check against used_length is completely racy. So no,
it's not that easy. There is no trusting on used_length at all. It
should never be access from asynchronous postcopy code.
2. There is one theoretical case with resizable allocations: Assume you
first shrink and then grow again. You would have some addr < used_length
where you cannot (and don't want to) place.
Note: Before discovering the nice -ENOENT handling, I had a second
variable postcopy_place_length stored in RAM blocks that would be
- Initialized to postcopy_length
- Synchronized by a mutex
- Changed inside the resize callback on any resizes to
-- postcopy_place_length = min(postcopy_place_length, newsize)
But TBH, I find using -ENOENT much more elegant. It was designed to
handle mmap changes like this.
--
Thanks,
David / dhildenb
