Re: [Qemu-ppc] [PATCH v4 04/14] pc: prepare for multi stage hotplug handlers

[David Hildenbrand]

On 07.06.2018 15:44, Igor Mammedov wrote:
> On Mon, 4 Jun 2018 13:27:01 +0200
> David Hildenbrand <address@hidden> wrote:
> 
>> On 31.05.2018 16:13, Igor Mammedov wrote:
>>> On Wed, 30 May 2018 16:13:32 +0200
>>> David Hildenbrand <address@hidden> wrote:
>>>   
>>>> On 30.05.2018 15:08, Igor Mammedov wrote:  
>>>>> On Thu, 17 May 2018 10:15:17 +0200
>>>>> David Hildenbrand <address@hidden> wrote:
>>>>>     
>>>>>> For multi stage hotplug handlers, we'll have to do some error handling
>>>>>> in some hotplug functions, so let's use a local error variable (except
>>>>>> for unplug requests).    
>>>>> I'd split out introducing local error into separate patch
>>>>> so patch would do a single thing.  
>>
>> I can do that if it makes review easier.
>>
>>>>>     
>>>>>> Also, add code to pass control to the final stage hotplug handler at the
>>>>>> parent bus.    
>>>>> But I don't agree with generic
>>>>>  "} else if ("dev->parent_bus && dev->parent_bus->hotplug_handler) {"
>>>>> forwarding, it's done by 3/14 for generic case and in case of
>>>>> special device that needs bus handler called from machine one,
>>>>> I'd suggest to do forwarding explicitly for that device only
>>>>> like we do with acpi_dev.    
>>>>
>>>> I decided to do it that way because it is generic and results in nicer
>>>> recovery handling (e.g. in case pc_dimm plug fails, we can simply
>>>> rollback all (for now MemoryDevice) previous plug operations).  
>>> rollback should be managed by the caller of pc_dimm plug
>>> directly, so it's not relevant here.
>>>   
>>>> IMHO, the resulting code is easier to read.
>>>>
>>>> From this handling it is clear that
>>>> "if we reach the hotplug handler, and it is not some special device
>>>> plugged by the machine (CPU, PC_DIMM), pass it on to the actual hotplug
>>>> handler if any exists"  
>>> I strongly disagree with that it's easier to deal with.
>>> You are basically duplicating already generalized code
>>> from qdev_get_hotplug_handler() back into boards.
>>>
>>> If a device doesn't have to be handled by machine handler,
>>> than qdev_get_hotplug_handler() must return its bus handler
>>> if any directly. So branch in question that your are copying
>>> is a dead one, pls drop it.  
>>
>> We forward selected (pc_get_hotpug_handler()) devices to the
>> right hotplug handler. Nothing wrong about that. I don't agree
>> with "basically duplicating already generalized code" wrong.
>> We have to forward at some place. Your idea simply places that
>> code at some other place.
>>
>>
>> But I think we have to get the general idea sorted out first.
>>
>> What you have in mind (e.g. plug):
>>
>> if (TYPE_MEMORY_DEVICE) {
>>      memory_device_plug();
>>      if (local_err) {
>>              goto out;
>>      }
>>
>>      if (TYPE_PC_DIMM) {
>>              pc_dimm_plug(hotplug_dev, dev, &local_err);
>>      } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
>>              hotplug_handler_plug(dev->parent_bus->hotplug_handler, dev, 
>> &local_err);
>>      }
>>      if (local_err) {
>>              memory_device_unplug()
>>              goto out;
>>      }
>> } else if (TYPE_CPU)
>> ...
>>
>>
>> What I have in mind (and implemented in this series):
>>
>>
>> if (TYPE_MEMORY_DEVICE) {
>>      memory_device_plug();
>> }
>> /* possibly other interfaces */
>> if (local_err) {
>>      error_handling();
>>      return;
>> }
>>
>> if (TYPE_PC_DIMM) {
>>      pc_dimm_plug(hotplug_dev, dev, &local_err);
>> } ...
>> } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
>>      hotplug_handler_plug(dev->parent_bus->hotplug_handler, dev, &local_err);
>> }
> I don't like this implicit wiring, where reader of this part of code
> has no idea that TYPE_MEMORY_DEVICE might be also bus device that needs
> request forwarding.
> I'd prefer [pre/un]plug handlers be concrete and explicit spaghetti code,
> something like this:
> 
> if (TYPE_PC_DIMM) {
>     pc_dimm_plug()
>     /* do here additional concrete machine specific things */
> } else if (TYPE_VIRTIO_MEM) {
>     virtio_mem_plug() <- do forwarding in there
>     /* and do here additional concrete machine specific things */
> } else if (TYPE_CPU) {
>     cpu_plug()
>     /* do here additional concrete machine specific things */
> }

That will result in a lot of duplicate code - for every machine we
support (dimm/virtio-mem/virtio-pmem/*add more memory devices here*) -
virtio-mem and virtio-pmem could most probably share the code.

> 
>> if (local_err) {
>>      if (object_dynamic_cast(OBJECT(dev), TYPE_MEMORY_DEVICE)) {
>>              memory_device_unplug()
>>      }
>>      /* possibly other interfaces */
>> }
>> ...
>>
>>
>> I claim that my variant is more generic because:
>> - it easily supports multiple interfaces (like MemoryDevice)
>>   per Device that need a hotplug handler call
>> - there is only one call to hotplug_handler_plug() in case we
>>   add similar handling for another device
> As someone said "one more layer of indirection would solve problem".
> But then one would have a clue how it works after a while (including
> author of the feature).
> I don't think we have a problem here and need generalization.
> 
>>
>> Apart from that they do exactly the same thing.
>>
> 


-- 

Thanks,

David / dhildenb