Re: [libvirt] [PATCH v2 1/3] qemu: use a bigger unplug timeout for PPC64 guests

On 10/9/19 5:15 PM, Cole Robinson wrote: > On 9/11/19 5:05 PM, Daniel Henrique Barboza wrote: >> For some architectures and setups, device removal can take >> longer than the default 5 seconds. This results in commands >> such as 'virsh setvcpus' to fire timeout messages even if >> the operation were successful in the guest, confusing the >> user. >> >> This patch sets a new 10 seconds unplug timeout for PPC64 >> guests. All other archs will keep the default 5 seconds >> timeout. >> >> Instead of putting 'if PPC64' conditionals inside qemu_hotplug.c >> to set the new timeout value, a new QEMU driver attribute >> 'unplugTimeout' was added. The timeout value is set during >> qemuStateInitialize only once. All qemu_hotplug.c functions >> that uses the timeout have easy access to a qemu_driver object, >> thus the change to use unplugTimeout is straightforward. >> >> The now unused 'qemuDomainRemoveDeviceWaitTime' global can't >> be simply erased from qemu_hotplug.c though. Next patch will >> remove it properly. >> > > Sorry for the wrong review delay. I see this implements danpb's > suggestion from the previous thread. The implementation seems a little > odd to me though because it is differentiating on host arch, but this > is about guest arch right? And probably an arbitrary number of > options, like I imagine TCG would want a longer timeout too (though > that's not anything you need to deal with) I think it's sensible to say that a TCG guest would always required a greater unplug timeout than a hardware accelerated one. However, never thought about considering TCG guests in this patch series though. A shame. So, considering that TCG guest exists, we can parametrize this unplug timeout calculation by considering guest (not host) architecture and if it's a TCG or a KVM guest. Speaking about the problem I'm trying to fix with ppc64 and what we already have, we can roll out this unplug timeout logic like: - pseries guest on KVM: 10 seconds - everyone else on KVM: 5 seconds (untouched, like it is today) > For TCG guests, perhaps double the KVM timeout? This would need experimentation, but double the timeout seems ok at first glance. Then we can do: - TCG pseries guest: 10 * 2 = 20 seconds - TCG with every other arch guest = 5 * 2 = 10 seconds This TCG calculation can be left alone for now as well - I can create the API considering that TCG guest exists, but do not infer the timeout for it. Either way works for me.

> > So I think this should be a function that lives in qemu_hotplug.c and > acts on a DomainDef at least. The test suite will have to mock that in > a qemuhotplugmock.c file, tests/qemucpumock.c is a good example to > follow. Just to check if we're on the same page, by 'I think this should be a function' you mean the calculation of qemuDomainRemoveDeviceWaitTime that I just mentioned above, right?