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

[Igor Mammedov <imammedo@redhat.com>]

On Thu, 7 Jun 2018 16:00:54 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 07.06.2018 15:44, Igor Mammedov wrote:
> > On Mon, 4 Jun 2018 13:27:01 +0200
> > David Hildenbrand <david@redhat.com> wrote:
> >   
> >> On 31.05.2018 16:13, Igor Mammedov wrote:  
> >>> On Wed, 30 May 2018 16:13:32 +0200
> >>> David Hildenbrand <david@redhat.com> wrote:
> >>>     
> >>>> On 30.05.2018 15:08, Igor Mammedov wrote:    
> >>>>> On Thu, 17 May 2018 10:15:17 +0200
> >>>>> David Hildenbrand <david@redhat.com> 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.
maybe or maybe not, depending on if pmem endups as memory device or
separate controller. And even with duplication, machine code would
be easy to follow just down one explicit call chain.

> 
> >   
> >> 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.
> >>  
> >   
> 
>