Re: [Qemu-devel] [PATCH 10/16] dimm: add busy slot check and slot auto-allocation

[Markus Armbruster]

Paolo Bonzini <address@hidden> writes:

> Il 26/07/2013 14:51, Igor Mammedov ha scritto:
>> On Fri, 26 Jul 2013 11:26:16 +0200
>> Paolo Bonzini <address@hidden> wrote:
>> 
>>> Il 26/07/2013 09:38, Igor Mammedov ha scritto:
>>>> Perhaps denying memory add and suggesting node migration to a node with
>>>> more memory would be right approach, otherwise user is bound to be hit by
>>>> cross node penalty.
>>>
>>> Or better, the user can first change the policy from "bind" to
>>> "preferred", and then hotplug.
>>>
>>>>>> I agree that specifying the policy on every hotplug complicates
>>>>>> management and may be overkill.  But then, most guests are not NUMA at
>>>>>> all and you would hardly perceive the difference, you would just have to
>>>>>> separate
>>>>>>
>>>>>>     set-mem-policy 0 size=2G
>>>>>>     device_add dimm,slot=0
>>>> I'm confused, size is inherent property of generic DimmDevice and policies
>>>> are NUMA specific of node.
>>>
>>> No, the size is not a property of the DimmDevice, it is a property of
>>> the host object that backs the DimmDevice.  This is like the size is not
>>> a property of a disk, but rather of the image that backs it.

If you excuse my pedantry: size *is* a property of the block device
model (guest part, a.k.a. frontend).  We just choose to derive it from
the image size (host part, a.k.a. backend) instead of making it
separately configurable.

If we wanted to make it separately configurable (overriding the implicit
image size), I'd certainly argue to make it a device model property.

>>> Now, there may be a good reason to remove the host/guest distinction in
>>> the case of memory, but I'm still not sure this is the case.
>> I don't like split model in this case because it's complicates interface
>> and confuses user. On bare-metal when user adds DIMM, it definitely has
>> size property. So when user adds DIMM device like:
>> 
>>      set-mem-policy 0 size=2G,somehostprop=y
>>      device_add dimm,slot=0
>> 
>> it's not very clear/intuitive to what 'size' relates to. On contrary:
>> 
>>      set-mem-policy 0 somehostprop=y
>>      device_add dimm,slot=0,size=2G
>> 
>> clearly says that we are adding 2Gb DIMM and allocator of memory that
>> bakes up DIMM, takes care about host specific details isolating them
>> from DIMM device model (which resembles baremetal one as much as possible).
>
> How is this different from
>
>     drive-add id=foo,aio=native
>     device-add virtio-block,drive=foo,file=/vm/foo.img
>
> We clearly do not do that, we put the file in the drive-add.

Yes, because the image file is a backend property, not a frontend
property.

>>> In the case of NUMA policies, I talked to Andrea Arcangeli and indeed
>>> his suggestion was to have a single policy per guest node (typically
>>> bind or preferred if guest node size <= host node size, interleave if
>>> guest node size > host node size).
>> with current implementation we could inherit DimmBusNumaAware from DimmBus,
>> and replace allocator to do that, not touching DIMM device model at all.
>
> The guest bus should surely not be aware of NUMA policies in the host.
>
> Remember here the guest may have just one node, but you want it bound to
> one particular node of the host.
>
>>> However, there are other properties of the memory object (e.g. hugetlbfs
>>> path) that could be customized for every slot.
>> 
>> Why not keep this mapping in object that allocates memory (DimmBus) and
>> allow it apply them to allocated memory when DIMM device is being added?
>
> It's not DimmBus that allocates memory.  Allocating memory is a host
> action, and even if it is triggered by DimmBus, it is not _done_ by DimmBus.
>
>> It still would be a split model but DimmDevice interface and implementation
>> would stay stable and the same (i.e. device_add dimm,id,size,slot,start,node)
>> whether we use per DIMM host specific policies, NUMA node policies or not 
>> care
>> about them at all.
>
> I'm not sure how.  Clean separation of host vs. guest properties and
> actions is the only thing that makes DimmDevice stable.
>
> In fact, the same separation is present in the real world as well.  The
> DimmDevice is not the physical memory chips, it is just a guest-visible
> representation of it.  Size is a property of the physical memory chips.

Without fully understanding the issues in this specific case: clean
separation of host vs. guest matters.  Old code didn't care, and we
spent a lot of sweat on cleaning it up, dancing to the merry backward
compatibility polka all the way.