Re: [PATCH v1 29/29] virtio-mem: Big Block Mode (BBM) - safe memory hotunplug

[David Hildenbrand]

On 19.10.20 09:54, Wei Yang wrote:
> On Mon, Oct 12, 2020 at 02:53:23PM +0200, David Hildenbrand wrote:
>> Let's add a safe mechanism to unplug memory, avoiding long/endless loops
>> when trying to offline memory - similar to in SBM.
>>
>> Fake-offline all memory (via alloc_contig_range()) before trying to
>> offline+remove it. Use this mode as default, but allow to enable the other
>> mode explicitly (which could give better memory hotunplug guarantees in
> 
> I don't get the point how unsafe mode would have a better guarantees?

It's primarily only relevant when there is a lot of concurrent action
going on while unplugging memory. Using alloc_contig_range() on
ZONE_MOVABLE can fail more easily than memory offlining.

alloc_contig_range() doesn't try as hard as memory offlining code to
isolate memory. There are known issues with temporary page pinning
(e.g., when a process dies) and the PCP. (mostly discovered via CMA
allocations)

See the TODO I add in patch #14.

[...]
>>
>> +	if (bbm_safe_unplug) {
>> +		/*
>> +		 * Start by fake-offlining all memory. Once we marked the device
>> +		 * block as fake-offline, all newly onlined memory will
>> +		 * automatically be kept fake-offline. Protect from concurrent
>> +		 * onlining/offlining until we have a consistent state.
>> +		 */
>> +		mutex_lock(&vm->hotplug_mutex);
>> +		virtio_mem_bbm_set_bb_state(vm, bb_id,
>> +					    VIRTIO_MEM_BBM_BB_FAKE_OFFLINE);
>> +
> 
> State is set here.
> 
>> +		for (pfn = start_pfn; pfn < end_pfn; pfn += PAGES_PER_SECTION) {
>> +			page = pfn_to_online_page(pfn);
>> +			if (!page)
>> +				continue;
>> +
>> +			rc = virtio_mem_fake_offline(pfn, PAGES_PER_SECTION);
>> +			if (rc) {
>> +				end_pfn = pfn;
>> +				goto rollback_safe_unplug;
>> +			}
>> +		}
>> +		mutex_unlock(&vm->hotplug_mutex);
>> +	}
>> +
>> 	rc = virtio_mem_bbm_offline_and_remove_bb(vm, bb_id);
>> -	if (rc)
>> +	if (rc) {
>> +		if (bbm_safe_unplug) {
>> +			mutex_lock(&vm->hotplug_mutex);
>> +			goto rollback_safe_unplug;
>> +		}
>> 		return rc;
>> +	}
>>
>> 	rc = virtio_mem_bbm_unplug_bb(vm, bb_id);
>> 	if (rc)
> 
> And changed to PLUGGED or UNUSED based on rc.

Right, after offlining+remove succeeded. So no longer added to Linux.

The final state depends on the success of the unplug request towards the
hypervisor.

> 
>> @@ -1987,6 +2069,17 @@ static int virtio_mem_bbm_offline_remove_and_unplug_bb(struct virtio_mem *vm,
>> 		virtio_mem_bbm_set_bb_state(vm, bb_id,
>> 					    VIRTIO_MEM_BBM_BB_UNUSED);
>> 	return rc;
>> +
>> +rollback_safe_unplug:
>> +	for (pfn = start_pfn; pfn < end_pfn; pfn += PAGES_PER_SECTION) {
>> +		page = pfn_to_online_page(pfn);
>> +		if (!page)
>> +			continue;
>> +		virtio_mem_fake_online(pfn, PAGES_PER_SECTION);
>> +	}
>> +	virtio_mem_bbm_set_bb_state(vm, bb_id, VIRTIO_MEM_BBM_BB_ADDED);
> 
> And changed to ADDED if failed.

Right, back to the initial state when entering this function.

> 
>> +	mutex_unlock(&vm->hotplug_mutex);
>> +	return rc;
>> }
> 
> So in which case, the bbm state is FAKE_OFFLINE during
> virtio_mem_bbm_notify_going_offline() and
> virtio_mem_bbm_notify_cancel_offline() ?

Exactly, so we can do our magic with fake-offline pages and our
virtio_mem_bbm_offline_and_remove_bb() can actually succeed.


-- 
Thanks,

David / dhildenb