On 3/16/22 1:17 PM, Claudio Fontana wrote:
> On 3/14/22 6:48 PM, Daniel P. Berrangé wrote:
>> On Mon, Mar 14, 2022 at 06:38:31PM +0100, Claudio Fontana wrote:
>>> On 3/14/22 6:17 PM, Daniel P. Berrangé wrote:
>>>> On Sat, Mar 12, 2022 at 05:30:01PM +0100, Claudio Fontana wrote:
>>>>> the first user is the qemu driver,
>>>>>
>>>>> virsh save/resume would slow to a crawl with a default pipe size (64k).
>>>>>
>>>>> This improves the situation by 400%.
>>>>>
>>>>> Going through io_helper still seems to incur in some penalty (~15%-ish)
>>>>> compared with direct qemu migration to a nc socket to a file.
>>>>>
>>>>> Signed-off-by: Claudio Fontana <cfont...@suse.de>
>>>>> ---
>>>>> src/qemu/qemu_driver.c | 6 +++---
>>>>> src/qemu/qemu_saveimage.c | 11 ++++++-----
>>>>> src/util/virfile.c | 12 ++++++++++++
>>>>> src/util/virfile.h | 1 +
>>>>> 4 files changed, 22 insertions(+), 8 deletions(-)
>>>>>
>>>>> Hello, I initially thought this to be a qemu performance issue,
>>>>> so you can find the discussion about this in qemu-devel:
>>>>>
>>>>> "Re: bad virsh save /dev/null performance (600 MiB/s max)"
>>>>>
>>>>> https://lists.gnu.org/archive/html/qemu-devel/2022-03/msg03142.html
>>>>>
>>>>> RFC since need to validate idea, and it is only lightly tested:
>>>>>
>>>>> save - about 400% benefit in throughput, getting around 20 Gbps to
>>>>> /dev/null,
>>>>> and around 13 Gbps to a ramdisk.
>>>>> By comparison, direct qemu migration to a nc socket is around 24Gbps.
>>>>>
>>>>> restore - not tested, _should_ also benefit in the "bypass_cache" case
>>>>> coredump - not tested, _should_ also benefit like for save
>>>>>
>>>>> Thanks for your comments and review,
>>>>>
>>>>> Claudio
>>>>>
>>>>>
>>>>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>>>>> index c1b3bd8536..be248c1e92 100644
>>>>> --- a/src/qemu/qemu_driver.c
>>>>> +++ b/src/qemu/qemu_driver.c
>>>>> @@ -3044,7 +3044,7 @@ doCoreDump(virQEMUDriver *driver,
>>>>> virFileWrapperFd *wrapperFd = NULL;
>>>>> int directFlag = 0;
>>>>> bool needUnlink = false;
>>>>> - unsigned int flags = VIR_FILE_WRAPPER_NON_BLOCKING;
>>>>> + unsigned int wrapperFlags = VIR_FILE_WRAPPER_NON_BLOCKING |
>>>>> VIR_FILE_WRAPPER_BIG_PIPE;
>>>>> const char *memory_dump_format = NULL;
>>>>> g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
>>>>> g_autoptr(virCommand) compressor = NULL;
>>>>> @@ -3059,7 +3059,7 @@ doCoreDump(virQEMUDriver *driver,
>>>>>
>>>>> /* Create an empty file with appropriate ownership. */
>>>>> if (dump_flags & VIR_DUMP_BYPASS_CACHE) {
>>>>> - flags |= VIR_FILE_WRAPPER_BYPASS_CACHE;
>>>>> + wrapperFlags |= VIR_FILE_WRAPPER_BYPASS_CACHE;
>>>>> directFlag = virFileDirectFdFlag();
>>>>> if (directFlag < 0) {
>>>>> virReportError(VIR_ERR_OPERATION_FAILED, "%s",
>>>>> @@ -3072,7 +3072,7 @@ doCoreDump(virQEMUDriver *driver,
>>>>> &needUnlink)) < 0)
>>>>> goto cleanup;
>>>>>
>>>>> - if (!(wrapperFd = virFileWrapperFdNew(&fd, path, flags)))
>>>>> + if (!(wrapperFd = virFileWrapperFdNew(&fd, path, wrapperFlags)))
>>>>> goto cleanup;
>>>>>
>>>>> if (dump_flags & VIR_DUMP_MEMORY_ONLY) {
>>>>> diff --git a/src/qemu/qemu_saveimage.c b/src/qemu/qemu_saveimage.c
>>>>> index c0139041eb..1b522a1542 100644
>>>>> --- a/src/qemu/qemu_saveimage.c
>>>>> +++ b/src/qemu/qemu_saveimage.c
>>>>> @@ -267,7 +267,7 @@ qemuSaveImageCreate(virQEMUDriver *driver,
>>>>> int fd = -1;
>>>>> int directFlag = 0;
>>>>> virFileWrapperFd *wrapperFd = NULL;
>>>>> - unsigned int wrapperFlags = VIR_FILE_WRAPPER_NON_BLOCKING;
>>>>> + unsigned int wrapperFlags = VIR_FILE_WRAPPER_NON_BLOCKING |
>>>>> VIR_FILE_WRAPPER_BIG_PIPE;
>>>>>
>>>>> /* Obtain the file handle. */
>>>>> if ((flags & VIR_DOMAIN_SAVE_BYPASS_CACHE)) {
>>>>> @@ -463,10 +463,11 @@ qemuSaveImageOpen(virQEMUDriver *driver,
>>>>> if ((fd = qemuDomainOpenFile(cfg, NULL, path, oflags, NULL)) < 0)
>>>>> return -1;
>>>>>
>>>>> - if (bypass_cache &&
>>>>> - !(*wrapperFd = virFileWrapperFdNew(&fd, path,
>>>>> -
>>>>> VIR_FILE_WRAPPER_BYPASS_CACHE)))
>>>>> - return -1;
>>>>> + if (bypass_cache) {
>>>>> + unsigned int wrapperFlags = VIR_FILE_WRAPPER_BYPASS_CACHE |
>>>>> VIR_FILE_WRAPPER_BIG_PIPE;
>>>>> + if (!(*wrapperFd = virFileWrapperFdNew(&fd, path, wrapperFlags)))
>>>>> + return -1;
>>>>> + }
>>>>>
>>>>> data = g_new0(virQEMUSaveData, 1);
>>>>>
>>>>> diff --git a/src/util/virfile.c b/src/util/virfile.c
>>>>> index a04f888e06..fdacd17890 100644
>>>>> --- a/src/util/virfile.c
>>>>> +++ b/src/util/virfile.c
>>>>> @@ -282,6 +282,18 @@ virFileWrapperFdNew(int *fd, const char *name,
>>>>> unsigned int flags)
>>>>>
>>>>> ret->cmd = virCommandNewArgList(iohelper_path, name, NULL);
>>>>>
>>>>> + if (flags & VIR_FILE_WRAPPER_BIG_PIPE) {
>>>>> + /*
>>>>> + * virsh save/resume would slow to a crawl with a default pipe
>>>>> size (usually 64k).
>>>>> + * This improves the situation by 400%, although going through
>>>>> io_helper still incurs
>>>>> + * in a performance penalty compared with a direct qemu
>>>>> migration to a socket.
>>>>> + */
>>>>> + int pipe_sz, rv = virFileReadValueInt(&pipe_sz,
>>>>> "/proc/sys/fs/pipe-max-size");
>>>>
>>>> This is fine as an experiment but I don't think it is that safe
>>>> to use in the real world. There could be a variety of reasons why
>>>> an admin can enlarge this value, and we shouldn't assume the max
>>>> size is sensible for libvirt/QEMU to use.
>>>>
>>>> I very much suspect there are diminishing returns here in terms
>>>> of buffer sizes.
>>>>
>>>> 64k is obvious too small, but 1 MB, may be sufficiently large
>>>> that the bottleneck is then elsewhere in our code. IOW, If the
>>>> pipe max size is 100 MB, we shouldn't blindly use it. Can you
>>>> do a few tests with varying sizes to see where a sensible
>>>> tradeoff falls ?
>>>
>>>
>>> Hi Daniel,
>>>
>>> this is a very good point. Actually I see very diminishing returns after
>>> the default pipe-max-size (1MB).
>>>
>>> The idea was that beyond allowing larger size, the admin could have set a
>>> _smaller_ pipe-max-size,
>>> so we want to use that in that case, otherwise an attempt to use 1MB would
>>> result in EPERM, if the process does not have CAP_SYS_RESOURCE or
>>> CAP_SYS_ADMIN.
>>> I am not sure if used with Kubevirt, for example, CAP_SYS_RESOURCE or
>>> CAP_SYS_ADMIN would be available...?
>>>
>>> So maybe one idea could be to use the minimum between
>>> /proc/sys/fs/pipe-max-size and for example 1MB, but will do more testing to
>>> see where the actual break point is.
>>
>> That's reasonable.
>>
>
> Just as an update: still running tests with various combinations, and larger
> VMs (to RAM, to slow disk, and now to nvme).
>
> For now no clear winner yet. There seems to be a significant benefit already
> going from 1MB (my previous default) to 2MB,
> but anything more than 16MB seems to not improve anything at all.
>
> But I just need to do more testing, more runs.
>
> Thanks,
>
> Claudio
>
Current results show these experimental averages maximum throughput migrating
to /dev/null per each FdWrapper Pipe Size (as per QEMU QMP "query-migrate",
tests repeated 5 times for each).
VM Size is 60G, most of the memory effectively touched before migration,
through user application allocating and touching all memory with pseudorandom
data.
64K: 5200 Mbps (current situation)
128K: 5800 Mbps
256K: 20900 Mbps
512K: 21600 Mbps
1M: 22800 Mbps
2M: 22800 Mbps
4M: 22400 Mbps
8M: 22500 Mbps
16M: 22800 Mbps
32M: 22900 Mbps
64M: 22900 Mbps
128M: 22800 Mbps
This above is the throughput out of patched libvirt with multiple Pipe Sizes
for the FDWrapper.
As for the theoretical limit for the libvirt architecture,
I ran a qemu migration directly issuing the appropriate QMP commands, setting
the same migration parameters as per libvirt, and then migrating to a socket
netcatted to /dev/null via
{"execute": "migrate", "arguments": { "uri", "unix:///tmp/netcat.sock" } } :
QMP: 37000 Mbps
---
So although the Pipe size improves things (in particular the large jump is for
the 256K size, although 1M seems a very good value),
there is still a second bottleneck in there somewhere that accounts for a loss
of ~14200 Mbps in throughput.
Thanks,
Claudio
