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

[Andreas Färber]

Am 26.07.2013 16:37, schrieb Paolo Bonzini:
> 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:
>>>>>> 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.
>>>
>>> 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.

The difference is that a user can understand drive-add, whereas
set-mem-policy in this use case is really hard to grasp as a prereq. If
it were
  memory-add id=foo,size=2G
  device-add dimm,slot=0,mem=foo
that may be different, but that is unhandy implementation-wise because
we assume initial RAM to be in one chunk and want to partition it into
guest NUMA nodes IIUC. Or has that changed?

I don't care if we name the device DIMM or MemoryBar, point is it should
represent the physical thing one plugs into a physical board, to match
what admins do with physical servers. And that physical bar has a size,
as Igor points out. It should not just represent some virtual
hot-plugged policy thing.

The drive is separate from the device because we can exchange the CD
media while leaving the device connected to ATA/AHCI/SCSI (and it allows
us to keep file, cache, etc. properties in a central place). Admins buy
servers/boards and memory bars, they won't solder chips onto it nor
change the NUMA configuration of the board while running, so we should
consider it immutable once created. If we unplug physical DIMMs, the
data can be considered lost (ignoring deep cooling tricks etc.), so no
point to transfer memory data from one DIMM to another.

Would we seriously want to exchange the memory a DIMM is backed by while
leaving the DIMM plugged? Rather the entity that owns the DIMM slots (as
opposed to DIMMs) has the guest NUMA policy configured for the
unpopulated slots. The slot has a maximum size and the DIMM has a size
less or equal to slot's maximum size.

I just wonder whether we need a DimmBus at all? Because if we get the
slot specified as in your examples then we could just set some dimm[n]
link<DIMM> on realize (question is where). We had a discussion once
about a missing callback when a link property is set to a new value, not
sure if that has been resolved meantime?
Can't think of a situation where we would have multiple DimmBus'es, only
some cases where it's on a SoC or SoM and not directly on the mainboard,
i.e. not /machine/dimm[n].
Code seems to be copying ICC bus, but ICC bus was added because APIC
needed a hot-pluggable bus to replace SysBus.

Hadn't Anthony and Ping Fang factored out a memory controller on the
i440fx? Patch 9 seems to add the bus directly to the i440fx PCI device.

Patch 13 seems to use a Notifier for PIIX4 ACPI rather than having the
bus handle hotplug itself and bus / memory controller communicating with
ACPI as necessary (layering violation). For the CPU we initially had no
bus at all to handle such event (we still don't outside x86).

What I am not finding in this series is a translation from -m to initial
non-hotplugged DIMMs?

Some random other remarks on the series:
* s/"dimmbus"/"dimm-bus"/g
* s/klass/oc/g -- there were loud protests against k* (class reserved)
* gtk-doc markup for parent fields is missing in header
* s/qdev/parent_obj/, s/qbus/parent_obj/ -- don't invite to use them
* s/dc/dbc/g -- dc is for DeviceClass
* DimmBusClass::register_memory() is never overwritten? In that case it
could well be just a static function called from realizefn - only
disctinction I can think of is how to allocate MemoryRegion. A
caller-settable DimmBus::slot_plugged() for i440fx to notify piix4 would
seem to solve the issue of the Notifier elegantly. PCIBus has such hooks.

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg