Re: [qemu-s390x] [PATCH v4 12/14] memory-device: factor out pre-plug into hotplug handler

[David Hildenbrand]

On 07.06.2018 17:00, Igor Mammedov wrote:
> On Mon, 4 Jun 2018 13:45:38 +0200
> David Hildenbrand <address@hidden> wrote:
> 
>> On 01.06.2018 13:17, Igor Mammedov wrote:
>>> On Thu, 17 May 2018 10:15:25 +0200
>>> David Hildenbrand <address@hidden> wrote:
>>>   
>>>> Let's move all pre-plug checks we can do without the device being
>>>> realized into the applicable hotplug handler for pc and spapr.
>>>>
>>>> Signed-off-by: David Hildenbrand <address@hidden>
>>>> ---
>>>>  hw/i386/pc.c                   | 11 +++++++
>>>>  hw/mem/memory-device.c         | 72 
>>>> +++++++++++++++++++-----------------------
>>>>  hw/ppc/spapr.c                 | 11 +++++++
>>>>  include/hw/mem/memory-device.h |  2 ++
>>>>  4 files changed, 57 insertions(+), 39 deletions(-)
>>>>
>>>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>>>> index 8bc41ef24b..61f1537e14 100644
>>>> --- a/hw/i386/pc.c
>>>> +++ b/hw/i386/pc.c
>>>> @@ -2010,6 +2010,16 @@ static void 
>>>> pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
>>>>  {
>>>>      Error *local_err = NULL;
>>>>  
>>>> +    /* first stage hotplug handler */
>>>> +    if (object_dynamic_cast(OBJECT(dev), TYPE_MEMORY_DEVICE)) {
>>>> +        memory_device_pre_plug(MACHINE(hotplug_dev), MEMORY_DEVICE(dev),
>>>> +                               &local_err);
>>>> +    }
>>>> +
>>>> +    if (local_err) {
>>>> +        goto out;
>>>> +    }
>>>> +
>>>>      /* final stage hotplug handler */
>>>>      if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
>>>>          pc_cpu_pre_plug(hotplug_dev, dev, &local_err);
>>>> @@ -2017,6 +2027,7 @@ static void 
>>>> pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
>>>>          hotplug_handler_pre_plug(dev->parent_bus->hotplug_handler, dev,
>>>>                                   &local_err);
>>>>      }
>>>> +out:
>>>>      error_propagate(errp, local_err);
>>>>  }
>>>>  
>>>> diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
>>>> index 361d38bfc5..d22c91993f 100644
>>>> --- a/hw/mem/memory-device.c
>>>> +++ b/hw/mem/memory-device.c
>>>> @@ -68,58 +68,26 @@ static int memory_device_used_region_size(Object *obj, 
>>>> void *opaque)
>>>>      return 0;
>>>>  }
>>>>  
>>>> -static void memory_device_check_addable(MachineState *ms, uint64_t size,
>>>> -                                        Error **errp)
>>>> -{
>>>> -    uint64_t used_region_size = 0;
>>>> -
>>>> -    /* we will need a new memory slot for kvm and vhost */
>>>> -    if (kvm_enabled() && !kvm_has_free_slot(ms)) {
>>>> -        error_setg(errp, "hypervisor has no free memory slots left");
>>>> -        return;
>>>> -    }
>>>> -    if (!vhost_has_free_slot()) {
>>>> -        error_setg(errp, "a used vhost backend has no free memory slots 
>>>> left");
>>>> -        return;
>>>> -    }
>>>> -
>>>> -    /* will we exceed the total amount of memory specified */
>>>> -    memory_device_used_region_size(OBJECT(ms), &used_region_size);
>>>> -    if (used_region_size + size > ms->maxram_size - ms->ram_size) {
>>>> -        error_setg(errp, "not enough space, currently 0x%" PRIx64
>>>> -                   " in use of total hot pluggable 0x" RAM_ADDR_FMT,
>>>> -                   used_region_size, ms->maxram_size - ms->ram_size);
>>>> -        return;
>>>> -    }
>>>> -
>>>> -}
>>>> -
>>>>  uint64_t memory_device_get_free_addr(MachineState *ms, const uint64_t 
>>>> *hint,
>>>>                                       uint64_t align, uint64_t size,
>>>>                                       Error **errp)
>>>>  {
>>>>      uint64_t address_space_start, address_space_end;
>>>> +    uint64_t used_region_size = 0;
>>>>      GSList *list = NULL, *item;
>>>>      uint64_t new_addr = 0;
>>>>  
>>>> -    if (!ms->device_memory) {
>>>> -        error_setg(errp, "memory devices (e.g. for memory hotplug) are 
>>>> not "
>>>> -                         "supported by the machine");
>>>> -        return 0;
>>>> -    }
>>>> -
>>>> -    if (!memory_region_size(&ms->device_memory->mr)) {
>>>> -        error_setg(errp, "memory devices (e.g. for memory hotplug) are 
>>>> not "
>>>> -                         "enabled, please specify the maxmem option");
>>>> -        return 0;
>>>> -    }
>>>>      address_space_start = ms->device_memory->base;
>>>>      address_space_end = address_space_start +
>>>>                          memory_region_size(&ms->device_memory->mr);
>>>>      g_assert(address_space_end >= address_space_start);
>>>>  
>>>> -    memory_device_check_addable(ms, size, errp);
>>>> -    if (*errp) {
>>>> +    /* will we exceed the total amount of memory specified */
>>>> +    memory_device_used_region_size(OBJECT(ms), &used_region_size);
>>>> +    if (used_region_size + size > ms->maxram_size - ms->ram_size) {
>>>> +        error_setg(errp, "not enough space, currently 0x%" PRIx64
>>>> +                   " in use of total hot pluggable 0x" RAM_ADDR_FMT,
>>>> +                   used_region_size, ms->maxram_size - ms->ram_size);
>>>>          return 0;
>>>>      }
>>>>  
>>>> @@ -242,6 +210,32 @@ uint64_t get_plugged_memory_size(void)
>>>>      return size;
>>>>  }
>>>>  
>>>> +void memory_device_pre_plug(MachineState *ms, const MemoryDeviceState *md,
>>>> +                            Error **errp)
>>>> +{
>>>> +    if (!ms->device_memory) {
>>>> +        error_setg(errp, "memory devices (e.g. for memory hotplug) are 
>>>> not "
>>>> +                         "supported by the machine");
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    if (!memory_region_size(&ms->device_memory->mr)) {
>>>> +        error_setg(errp, "memory devices (e.g. for memory hotplug) are 
>>>> not "
>>>> +                         "enabled, please specify the maxmem option");
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    /* we will need a new memory slot for kvm and vhost */
>>>> +    if (kvm_enabled() && !kvm_has_free_slot(ms)) {
>>>> +        error_setg(errp, "hypervisor has no free memory slots left");
>>>> +        return;
>>>> +    }
>>>> +    if (!vhost_has_free_slot()) {
>>>> +        error_setg(errp, "a used vhost backend has no free memory slots 
>>>> left");
>>>> +        return;
>>>> +    }  
>>> thanks for extracting preparations steps into the right callback.
>>>
>>> on top of this _preplug() handler should also set being plugged
>>> device properties if they weren't set by user.
>>>  memory_device_get_free_addr() should be here to.
>>>
>>> general rule for _preplug() would be to check and prepare device
>>> for being plugged but not touch anything beside the device (it's _plug 
>>> handler job)  
>>
>> I disagree. Or at least I disagree as part of this patch series because
>> it over-complicates things :)
> "Welcome to rewrite QEMU first before you do your thing" club :)

I feels like I'm doing nothing else all time :)

> 
> That's why I've suggested to split series in several small ones
> tackling concrete goals /1st: unplug cleanups, 2nd: preplug refactoring/
> instead of intermixing loosely related patches.

I'll send the fixes and cleanups fairly soon. That will hopefully reduce
the patch count (it increased already a lot due to your review already).

> 
> It should help to review and merge prerequisite work fast as it doesn't
> related to virtio-mem. The rest of refactoring (which is not much once
> you split out the 2 first series) that's is done directly for virtio-mem
> sake should be posted as part of that series.
> It probably would be biger series but posting them separately doesn't
> make sense either as reviewer would have to jump between them anyways
> to make sensible review.

We'll find a way. As you want to have TYPE_VIRTIO_MEM specific handling
code, this can go into the virtio-mem series.

> 
>> preplug() can do basic checks but I don't think it should be used to
>> change actual properties. And I give you the main reason for this:
>>
>> We have to do quite some amount of unnecessary error handling (please
>> remember the pc_dimm plug code madness before I refactored it - 80%
>> consisted of error handling) if we want to work on device properties
>> before a device is realized. And we even have duplicate checks both in
>> the realize() and the preplug() code then (again, what we had in the
>> pc_dimm plug code - do we have a memdev already or not).
>>
>> Right now, I assume, that all MemoryDevice functions can be safely
>> called after the device has been realized without error handling. This
>> is nice. It e.g. guarantees that we have a memdev assigned. Otherwise,
>> every time we access some MemoryDevice property (e.g. region size), we
>> have to handle possible uninitialized properties (e.g. memdev) -
>> something I don't like.
>>
>> So I want to avoid this by any means. And I don't really see a benefit
>> for this additional error handling that will be necessary: We don't care
>> about the performance in case something went wrong.
>>
> error checks are not really important here.
> Here I care about keeping QOM model work as it supposed to, i.e.
>  object_new() -> set properties -> realize()
> set properties should happen before realize() is called and
> that's preplug callback.
> 
> Currently setting properties is still in plug() handler
> because preplug handler was introduced later and nobody was
> rewriting that code to the extent you do.
> 

/me regretting he started to touch that code

I still don't think performing that change is wort it. As I said, we
will need a lot of duplicate error handling "is memdev set or not" -
while this is checked at one central place: when realizing.

-- 

Thanks,

David / dhildenb