dm-crypt performance regression due to workqueue changes

dm-crypt performance regression due to workqueue changes

[Mikulas Patocka]

Hi

I report that the patch 63c5484e74952f60f5810256bd69814d167b8d22 
("workqueue: Add multiple affinity scopes and interface to select them") 
is causing massive dm-crypt slowdown in virtual machines.

Steps to reproduce:
* Install a system in a virtual machine with 16 virtual CPUs
* Create a scratch file with "dd if=/dev/zero of=Scratch.img bs=1M
  count=2048 oflag=direct" - the file should be on a fast NVMe drive
* Attach the scratch file to the virtual machine as /dev/vdb; cache mode
  should be 'none'
* cryptsetup --force-password luksFormat /dev/vdb
* cryptsetup luksOpen /dev/vdb cr
* fio --direct=1 --bsrange=128k-128k --runtime=40 --numjobs=1
  --ioengine=libaio --iodepth=8 --group_reporting=1
  --filename=/dev/mapper/cr --name=job --rw=read

With 6.5, we get 3600MiB/s; with 6.6 we get 1400MiB/s.

The reason is that virt-manager by default sets up a topology where we 
have 16 sockets, 1 core per socket, 1 thread per core. And that workqueue 
patch avoids moving work items across sockets, so it processes all 
encryption work only on one virtual CPU.

The performance degradation may be fixed with "echo 'system'
>/sys/module/workqueue/parameters/default_affinity_scope" - but it is 
regression anyway, as many users don't know about this option.

How should we fix it? There are several options:
1. revert back to 'numa' affinity
2. revert to 'numa' affinity only if we are in a virtual machine
3. hack dm-crypt to set the 'numa' affinity for the affected workqueues
4. any other solution?

Mikulas

Re: dm-crypt performance regression due to workqueue changes

[Waiman Long]

On 6/29/24 14:15, Mikulas Patocka wrote:
> Hi
>
> I report that the patch 63c5484e74952f60f5810256bd69814d167b8d22
> ("workqueue: Add multiple affinity scopes and interface to select them")
> is causing massive dm-crypt slowdown in virtual machines.
>
> Steps to reproduce:
> * Install a system in a virtual machine with 16 virtual CPUs
> * Create a scratch file with "dd if=/dev/zero of=Scratch.img bs=1M
>    count=2048 oflag=direct" - the file should be on a fast NVMe drive
> * Attach the scratch file to the virtual machine as /dev/vdb; cache mode
>    should be 'none'
> * cryptsetup --force-password luksFormat /dev/vdb
> * cryptsetup luksOpen /dev/vdb cr
> * fio --direct=1 --bsrange=128k-128k --runtime=40 --numjobs=1
>    --ioengine=libaio --iodepth=8 --group_reporting=1
>    --filename=/dev/mapper/cr --name=job --rw=read
>
> With 6.5, we get 3600MiB/s; with 6.6 we get 1400MiB/s.
>
> The reason is that virt-manager by default sets up a topology where we
> have 16 sockets, 1 core per socket, 1 thread per core. And that workqueue
> patch avoids moving work items across sockets, so it processes all
> encryption work only on one virtual CPU.
>
> The performance degradation may be fixed with "echo 'system'
>> /sys/module/workqueue/parameters/default_affinity_scope" - but it is
> regression anyway, as many users don't know about this option.
>
> How should we fix it? There are several options:
> 1. revert back to 'numa' affinity
> 2. revert to 'numa' affinity only if we are in a virtual machine
> 3. hack dm-crypt to set the 'numa' affinity for the affected workqueues
> 4. any other solution?

Another alternative  is to go back to the old "numa" default if the 
kernel is running under a hypervisor since the cpu configuration 
information is likely to be incorrect anyway. The current default of 
"cache" will remain if not under a hypervisor.

Cheers,
Longman

Re: dm-crypt performance regression due to workqueue changes

[Mikulas Patocka]

[-- Attachment #1: Type: text/plain, Size: 2144 bytes --]



On Sat, 29 Jun 2024, Waiman Long wrote:

> On 6/29/24 14:15, Mikulas Patocka wrote:
> > Hi
> >
> > I report that the patch 63c5484e74952f60f5810256bd69814d167b8d22
> > ("workqueue: Add multiple affinity scopes and interface to select them")
> > is causing massive dm-crypt slowdown in virtual machines.
> >
> > Steps to reproduce:
> > * Install a system in a virtual machine with 16 virtual CPUs
> > * Create a scratch file with "dd if=/dev/zero of=Scratch.img bs=1M
> >    count=2048 oflag=direct" - the file should be on a fast NVMe drive
> > * Attach the scratch file to the virtual machine as /dev/vdb; cache mode
> >    should be 'none'
> > * cryptsetup --force-password luksFormat /dev/vdb
> > * cryptsetup luksOpen /dev/vdb cr
> > * fio --direct=1 --bsrange=128k-128k --runtime=40 --numjobs=1
> >    --ioengine=libaio --iodepth=8 --group_reporting=1
> >    --filename=/dev/mapper/cr --name=job --rw=read
> >
> > With 6.5, we get 3600MiB/s; with 6.6 we get 1400MiB/s.
> >
> > The reason is that virt-manager by default sets up a topology where we
> > have 16 sockets, 1 core per socket, 1 thread per core. And that workqueue
> > patch avoids moving work items across sockets, so it processes all
> > encryption work only on one virtual CPU.
> >
> > The performance degradation may be fixed with "echo 'system'
> >> /sys/module/workqueue/parameters/default_affinity_scope" - but it is
> > regression anyway, as many users don't know about this option.
> >
> > How should we fix it? There are several options:
> > 1. revert back to 'numa' affinity
> > 2. revert to 'numa' affinity only if we are in a virtual machine
> > 3. hack dm-crypt to set the 'numa' affinity for the affected workqueues
> > 4. any other solution?
> 
> Another alternative  is to go back to the old "numa" default if the kernel is
> running under a hypervisor since the cpu configuration information is likely
> to be incorrect anyway. The current default of "cache" will remain if not
> under a hypervisor.
> 
> Cheers,
> Longman

Yes. How could we detect that we run under a hypervisor portably? There's 
a flag X86_FEATURE_HYPERVISOR, but it's x86-only.

Mikulas

Re: dm-crypt performance regression due to workqueue changes

[Waiman Long]

On 6/30/24 05:49, Mikulas Patocka wrote:
>
> On Sat, 29 Jun 2024, Waiman Long wrote:
>
>> On 6/29/24 14:15, Mikulas Patocka wrote:
>>> Hi
>>>
>>> I report that the patch 63c5484e74952f60f5810256bd69814d167b8d22
>>> ("workqueue: Add multiple affinity scopes and interface to select them")
>>> is causing massive dm-crypt slowdown in virtual machines.
>>>
>>> Steps to reproduce:
>>> * Install a system in a virtual machine with 16 virtual CPUs
>>> * Create a scratch file with "dd if=/dev/zero of=Scratch.img bs=1M
>>>     count=2048 oflag=direct" - the file should be on a fast NVMe drive
>>> * Attach the scratch file to the virtual machine as /dev/vdb; cache mode
>>>     should be 'none'
>>> * cryptsetup --force-password luksFormat /dev/vdb
>>> * cryptsetup luksOpen /dev/vdb cr
>>> * fio --direct=1 --bsrange=128k-128k --runtime=40 --numjobs=1
>>>     --ioengine=libaio --iodepth=8 --group_reporting=1
>>>     --filename=/dev/mapper/cr --name=job --rw=read
>>>
>>> With 6.5, we get 3600MiB/s; with 6.6 we get 1400MiB/s.
>>>
>>> The reason is that virt-manager by default sets up a topology where we
>>> have 16 sockets, 1 core per socket, 1 thread per core. And that workqueue
>>> patch avoids moving work items across sockets, so it processes all
>>> encryption work only on one virtual CPU.
>>>
>>> The performance degradation may be fixed with "echo 'system'
>>>> /sys/module/workqueue/parameters/default_affinity_scope" - but it is
>>> regression anyway, as many users don't know about this option.
>>>
>>> How should we fix it? There are several options:
>>> 1. revert back to 'numa' affinity
>>> 2. revert to 'numa' affinity only if we are in a virtual machine
>>> 3. hack dm-crypt to set the 'numa' affinity for the affected workqueues
>>> 4. any other solution?
>> Another alternative  is to go back to the old "numa" default if the kernel is
>> running under a hypervisor since the cpu configuration information is likely
>> to be incorrect anyway. The current default of "cache" will remain if not
>> under a hypervisor.
>>
>> Cheers,
>> Longman
> Yes. How could we detect that we run under a hypervisor portably? There's
> a flag X86_FEATURE_HYPERVISOR, but it's x86-only.

Right, that will be for x86 only. There is also a kernel boot command 
line parameter "workqueue.default_affinity_scope=" that one can use to 
set the default. It will be a bit easier to use than changing sysfs 
parameter at run time.

Cheers,
Longman

Re: dm-crypt performance regression due to workqueue changes

[Tejun Heo]

Hello,

On Sat, Jun 29, 2024 at 08:15:56PM +0200, Mikulas Patocka wrote:
> With 6.5, we get 3600MiB/s; with 6.6 we get 1400MiB/s.
> 
> The reason is that virt-manager by default sets up a topology where we 
> have 16 sockets, 1 core per socket, 1 thread per core. And that workqueue 
> patch avoids moving work items across sockets, so it processes all 
> encryption work only on one virtual CPU.
> 
> The performance degradation may be fixed with "echo 'system'
> >/sys/module/workqueue/parameters/default_affinity_scope" - but it is 
> regression anyway, as many users don't know about this option.
> 
> How should we fix it? There are several options:
> 1. revert back to 'numa' affinity
> 2. revert to 'numa' affinity only if we are in a virtual machine
> 3. hack dm-crypt to set the 'numa' affinity for the affected workqueues
> 4. any other solution?

Do you happen to know why libvirt is doing that? There are many other
implications to configuring the system that way and I don't think we want to
design kernel behaviors to suit topology information fed to VMs which can be
arbitrary.

Thanks.

-- 
tejun

Re: dm-crypt performance regression due to workqueue changes

[Mikulas Patocka]

On Sun, 30 Jun 2024, Tejun Heo wrote:

> Hello,
> 
> On Sat, Jun 29, 2024 at 08:15:56PM +0200, Mikulas Patocka wrote:
> 
> > With 6.5, we get 3600MiB/s; with 6.6 we get 1400MiB/s.
> > 
> > The reason is that virt-manager by default sets up a topology where we 
> > have 16 sockets, 1 core per socket, 1 thread per core. And that workqueue 
> > patch avoids moving work items across sockets, so it processes all 
> > encryption work only on one virtual CPU.
> > 
> > The performance degradation may be fixed with "echo 'system'
> > >/sys/module/workqueue/parameters/default_affinity_scope" - but it is 
> > regression anyway, as many users don't know about this option.
> > 
> > How should we fix it? There are several options:
> > 1. revert back to 'numa' affinity
> > 2. revert to 'numa' affinity only if we are in a virtual machine
> > 3. hack dm-crypt to set the 'numa' affinity for the affected workqueues
> > 4. any other solution?
> 
> Do you happen to know why libvirt is doing that? There are many other
> implications to configuring the system that way and I don't think we want to
> design kernel behaviors to suit topology information fed to VMs which can be
> arbitrary.
> 
> Thanks.

I don't know why. I added users@lists.libvirt.org to the CC.

How should libvirt properly advertise "we have 16 threads that are 
dynamically scheduled by the host kernel, so the latencies between them 
are changing and unpredictable"?

Mikulas

Re: dm-crypt performance regression due to workqueue changes

[Michal Prívozník]

On 6/30/24 20:49, Mikulas Patocka wrote:
> 
> 
> On Sun, 30 Jun 2024, Tejun Heo wrote:
> 
>> Hello,
>>
>> On Sat, Jun 29, 2024 at 08:15:56PM +0200, Mikulas Patocka wrote:
>>
>>> With 6.5, we get 3600MiB/s; with 6.6 we get 1400MiB/s.
>>>
>>> The reason is that virt-manager by default sets up a topology where we 
>>> have 16 sockets, 1 core per socket, 1 thread per core. And that workqueue 
>>> patch avoids moving work items across sockets, so it processes all 
>>> encryption work only on one virtual CPU.
>>>
>>> The performance degradation may be fixed with "echo 'system'
>>>> /sys/module/workqueue/parameters/default_affinity_scope" - but it is 
>>> regression anyway, as many users don't know about this option.
>>>
>>> How should we fix it? There are several options:
>>> 1. revert back to 'numa' affinity
>>> 2. revert to 'numa' affinity only if we are in a virtual machine
>>> 3. hack dm-crypt to set the 'numa' affinity for the affected workqueues
>>> 4. any other solution?
>>
>> Do you happen to know why libvirt is doing that? There are many other
>> implications to configuring the system that way and I don't think we want to
>> design kernel behaviors to suit topology information fed to VMs which can be
>> arbitrary.

Firstly, libvirt's not doing anything. It very specifically avoids doing
policy decisions. If something configures vCPUs so that they are in
separate sockets, then we should look at that something. Alternatively,
if "default" configuration does not work for your workflow well,
document recommended configuration.

>>
>> Thanks.
> 
> I don't know why. I added users@lists.libvirt.org to the CC.
> 
> How should libvirt properly advertise "we have 16 threads that are 
> dynamically scheduled by the host kernel, so the latencies between them 
> are changing and unpredictable"?

Libvirt advertises topology of physical CPUs (pCPUS) in so called
capabilities XML (virsh capabilities). For example:


  https://libvirt.org/formatcaps.html#examples

(not to be mixed with domain capabilities!)

From there you can see what pCPUs are in the same socket. And regarding
latency - unless you're doing real time, latency is unpredictable even
within single socket, isn't it.

Michal

Re: dm-crypt performance regression due to workqueue changes

[Daniel P. Berrangé]

On Mon, Jul 01, 2024 at 02:48:07PM +0200, Michal Prívozník wrote:
> On 6/30/24 20:49, Mikulas Patocka wrote:
> > 
> > 
> > On Sun, 30 Jun 2024, Tejun Heo wrote:
> > 
> >> Hello,
> >>
> >> On Sat, Jun 29, 2024 at 08:15:56PM +0200, Mikulas Patocka wrote:
> >>
> >>> With 6.5, we get 3600MiB/s; with 6.6 we get 1400MiB/s.
> >>>
> >>> The reason is that virt-manager by default sets up a topology where we 
> >>> have 16 sockets, 1 core per socket, 1 thread per core. And that workqueue 
> >>> patch avoids moving work items across sockets, so it processes all 
> >>> encryption work only on one virtual CPU.
> >>>
> >>> The performance degradation may be fixed with "echo 'system'
> >>>> /sys/module/workqueue/parameters/default_affinity_scope" - but it is 
> >>> regression anyway, as many users don't know about this option.
> >>>
> >>> How should we fix it? There are several options:
> >>> 1. revert back to 'numa' affinity
> >>> 2. revert to 'numa' affinity only if we are in a virtual machine
> >>> 3. hack dm-crypt to set the 'numa' affinity for the affected workqueues
> >>> 4. any other solution?
> >>
> >> Do you happen to know why libvirt is doing that? There are many other
> >> implications to configuring the system that way and I don't think we want to
> >> design kernel behaviors to suit topology information fed to VMs which can be
> >> arbitrary.
> 
> Firstly, libvirt's not doing anything. It very specifically avoids doing
> policy decisions. If something configures vCPUs so that they are in
> separate sockets, then we should look at that something. Alternatively,
> if "default" configuration does not work for your workflow well,
> document recommended configuration.

Actually in this particular case, it is strictly speaking libvirt.
If the guest XML config does not mention any <topology> info, then
libvirt explicitly tells QEMU to set sockets=N,cores=1,threads=1.
That matches QEMU's own historical built-in default topology.

None the less, my advice for mgmt applications using libvirt would
likely be to explicitly request sockets=1,cores=N,threads=1. This
is because it gives slightly better compatibility with unpleasant
software that applies licensing / subscription rules that penalize
use of many sockets, while being happy with any number of cores.


Either way though, the topology is a lie when the guest CPUs
are not pinned to host CPUs, so making performance decisions based
on this is unlikely to yield the desired results. Historically the
cores vs sockets distinction hasn't seemed to make much difference
to guest OS performance, as the OS' haven't made significant
decisions on this axis. Exposing threads != 1 though has always been
a big no though, unless strictly pinning 1:1 guest:host CPUs, as that
has had notable impacts on scheduling decisions.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: dm-crypt performance regression due to workqueue changes

[Mikulas Patocka]

[-- Attachment #1: Type: text/plain, Size: 2209 bytes --]



On Mon, 1 Jul 2024, Daniel P. Berrangé wrote:

> On Mon, Jul 01, 2024 at 02:48:07PM +0200, Michal Prívozník wrote:
> > On 6/30/24 20:49, Mikulas Patocka wrote:
> > > 
> > > 
> > > On Sun, 30 Jun 2024, Tejun Heo wrote:
> > >>
> > >> Do you happen to know why libvirt is doing that? There are many other
> > >> implications to configuring the system that way and I don't think we want to
> > >> design kernel behaviors to suit topology information fed to VMs which can be
> > >> arbitrary.
> > 
> > Firstly, libvirt's not doing anything. It very specifically avoids doing
> > policy decisions. If something configures vCPUs so that they are in
> > separate sockets, then we should look at that something. Alternatively,
> > if "default" configuration does not work for your workflow well,
> > document recommended configuration.
> 
> Actually in this particular case, it is strictly speaking libvirt.
> If the guest XML config does not mention any <topology> info, then
> libvirt explicitly tells QEMU to set sockets=N,cores=1,threads=1.
> That matches QEMU's own historical built-in default topology.
> 
> None the less, my advice for mgmt applications using libvirt would
> likely be to explicitly request sockets=1,cores=N,threads=1. This
> is because it gives slightly better compatibility with unpleasant
> software that applies licensing / subscription rules that penalize
> use of many sockets, while being happy with any number of cores.
> 
> 
> Either way though, the topology is a lie when the guest CPUs
> are not pinned to host CPUs, so making performance decisions based
> on this is unlikely to yield the desired results. Historically the
> cores vs sockets distinction hasn't seemed to make much difference
> to guest OS performance, as the OS' haven't made significant
> decisions on this axis. Exposing threads != 1 though has always been
> a big no though, unless strictly pinning 1:1 guest:host CPUs, as that
> has had notable impacts on scheduling decisions.
> 
> With regards,
> Daniel

I think there should be some way how to tell the guest kernel "the vCPUs 
are free-floating, the topology is a lie", so that it can stop making 
incorrect decisions based on the fake topology.

Mikulas

Re: dm-crypt performance regression due to workqueue changes

[Daniel P. Berrangé]

On Sun, Jun 30, 2024 at 08:49:48PM +0200, Mikulas Patocka wrote:
> 
> 
> On Sun, 30 Jun 2024, Tejun Heo wrote:
> 
> > Hello,
> > 
> > On Sat, Jun 29, 2024 at 08:15:56PM +0200, Mikulas Patocka wrote:
> > 
> > > With 6.5, we get 3600MiB/s; with 6.6 we get 1400MiB/s.
> > > 
> > > The reason is that virt-manager by default sets up a topology where we 
> > > have 16 sockets, 1 core per socket, 1 thread per core. And that workqueue 
> > > patch avoids moving work items across sockets, so it processes all 
> > > encryption work only on one virtual CPU.
>
> > > The performance degradation may be fixed with "echo 'system'
> > > >/sys/module/workqueue/parameters/default_affinity_scope" - but it is 
> > > regression anyway, as many users don't know about this option.
> > > 
> > > How should we fix it? There are several options:
> > > 1. revert back to 'numa' affinity
> > > 2. revert to 'numa' affinity only if we are in a virtual machine
> > > 3. hack dm-crypt to set the 'numa' affinity for the affected workqueues
> > > 4. any other solution?
> > 
> > Do you happen to know why libvirt is doing that? There are many other
> > implications to configuring the system that way and I don't think we want to
> > design kernel behaviors to suit topology information fed to VMs which can be
> > arbitrary.
> > 
> > Thanks.
> 
> I don't know why. I added users@lists.libvirt.org to the CC.
> 
> How should libvirt properly advertise "we have 16 threads that are 
> dynamically scheduled by the host kernel, so the latencies between them 
> are changing and unpredictable"?

NB, libvirt is just control plane, the actual virtual hardware exposed
is implemented across QEMU and the KVM kernel mod. Guest CPU topology
and/or NUMA cost information is the responsibility of QEMU.

When QEMU's virtual CPUs are floating freely across host CPUs there's
no perfect answer. The host admin needs to make a tradeoff in their
configuration

They can optimize for density, by allowing guest CPUs to float freely
and allow CPU overcommit against host CPUs, and the guest CPU topology
is essentially a lie.

They can optimize for predictable performance, by strictly pinning
guest CPUs 1:1 to host CPUs, and minimize CPU overcommit, and have
the guest CPU topology 1:1 match the host CPU topology.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: dm-crypt performance regression due to workqueue changes

[Mikulas Patocka]

[-- Attachment #1: Type: text/plain, Size: 2428 bytes --]



On Mon, 1 Jul 2024, Daniel P. Berrangé wrote:

> On Sun, Jun 30, 2024 at 08:49:48PM +0200, Mikulas Patocka wrote:
> > 
> > 
> > On Sun, 30 Jun 2024, Tejun Heo wrote:
> > 
> > > Do you happen to know why libvirt is doing that? There are many other
> > > implications to configuring the system that way and I don't think we want to
> > > design kernel behaviors to suit topology information fed to VMs which can be
> > > arbitrary.
> > > 
> > > Thanks.
> > 
> > I don't know why. I added users@lists.libvirt.org to the CC.
> > 
> > How should libvirt properly advertise "we have 16 threads that are 
> > dynamically scheduled by the host kernel, so the latencies between them 
> > are changing and unpredictable"?
> 
> NB, libvirt is just control plane, the actual virtual hardware exposed
> is implemented across QEMU and the KVM kernel mod. Guest CPU topology
> and/or NUMA cost information is the responsibility of QEMU.
> 
> When QEMU's virtual CPUs are floating freely across host CPUs there's
> no perfect answer. The host admin needs to make a tradeoff in their
> configuration
> 
> They can optimize for density, by allowing guest CPUs to float freely
> and allow CPU overcommit against host CPUs, and the guest CPU topology
> is essentially a lie.
> 
> They can optimize for predictable performance, by strictly pinning
> guest CPUs 1:1 to host CPUs, and minimize CPU overcommit, and have
> the guest CPU topology 1:1 match the host CPU topology.
> 
> With regards,
> Daniel

The problem that we have here is that the commit 
63c5484e74952f60f5810256bd69814d167b8d22 ("workqueue: Add multiple 
affinity scopes and interface to select them") changes the behavior of 
unbound workqueues, so that work items are only executed on CPUs that 
share last level cache with the task that submitted them.

If there are 16 virtual CPUs that are freely floating across physical 
CPUs, virt-manager by default selects a topology where it advertises 16 
sockets, 1 CPU per socket, 1 thread per CPU. The result is that the 
unbound workqueues are no longer unbound, they can't move work across 
sockets and they are bound to just one virtual CPU, causing dm-crypt 
performance degradation. (the crypto operations are no longer 
parallelized).

Whose bug is this? Is it a bug in virt-manager because it advertises 
invalid topology? Is this a bug in that patch 63c5484e7495 because it 
avoids moving work items across sockets?

Mikulas

Re: dm-crypt performance regression due to workqueue changes

[Daniel P. Berrangé]

On Mon, Jul 01, 2024 at 03:42:29PM +0200, Mikulas Patocka wrote:
> 
> 
> On Mon, 1 Jul 2024, Daniel P. Berrangé wrote:
> 
> > On Sun, Jun 30, 2024 at 08:49:48PM +0200, Mikulas Patocka wrote:
> > > 
> > > 
> > > On Sun, 30 Jun 2024, Tejun Heo wrote:
> > > 
> > > > Do you happen to know why libvirt is doing that? There are many other
> > > > implications to configuring the system that way and I don't think we want to
> > > > design kernel behaviors to suit topology information fed to VMs which can be
> > > > arbitrary.
> > > > 
> > > > Thanks.
> > > 
> > > I don't know why. I added users@lists.libvirt.org to the CC.
> > > 
> > > How should libvirt properly advertise "we have 16 threads that are 
> > > dynamically scheduled by the host kernel, so the latencies between them 
> > > are changing and unpredictable"?
> > 
> > NB, libvirt is just control plane, the actual virtual hardware exposed
> > is implemented across QEMU and the KVM kernel mod. Guest CPU topology
> > and/or NUMA cost information is the responsibility of QEMU.
> > 
> > When QEMU's virtual CPUs are floating freely across host CPUs there's
> > no perfect answer. The host admin needs to make a tradeoff in their
> > configuration
> > 
> > They can optimize for density, by allowing guest CPUs to float freely
> > and allow CPU overcommit against host CPUs, and the guest CPU topology
> > is essentially a lie.
> > 
> > They can optimize for predictable performance, by strictly pinning
> > guest CPUs 1:1 to host CPUs, and minimize CPU overcommit, and have
> > the guest CPU topology 1:1 match the host CPU topology.
> 
> The problem that we have here is that the commit 
> 63c5484e74952f60f5810256bd69814d167b8d22 ("workqueue: Add multiple 
> affinity scopes and interface to select them") changes the behavior of 
> unbound workqueues, so that work items are only executed on CPUs that 
> share last level cache with the task that submitted them.
> 
> If there are 16 virtual CPUs that are freely floating across physical 
> CPUs, virt-manager by default selects a topology where it advertises 16 
> sockets, 1 CPU per socket, 1 thread per CPU. The result is that the 
> unbound workqueues are no longer unbound, they can't move work across 
> sockets and they are bound to just one virtual CPU, causing dm-crypt 
> performance degradation. (the crypto operations are no longer 
> parallelized).
> 
> Whose bug is this? Is it a bug in virt-manager because it advertises 
> invalid topology? Is this a bug in that patch 63c5484e7495 because it 
> avoids moving work items across sockets?

It is hard to call it is a bug in anything. The Linux patch is reasonable
in honouring the CPU topology. The hypervisor is reasonable to exposing
sockets=N,cores=1 as there's no right answer for the topology choice, and
it has no idea how it may or may not impact guest OS behaviour or perf.
Letting CPUs float freely is sensible default behaviour too. All of them
conspire to have a perf impact here, but the deployment is not seeking
to maximise performance, rather to maximise flexibility & density.

None the less, I'd suggest that virt-manager should be a explicitly
asking for sockets=1,cores=N, as that has broader guest OS compatibility.
By chance that would also help this scenario, but that woudn't be a
driving factor as we can't pick defaults based on the needs of particular
versions of a particular guest kernel.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: dm-crypt performance regression due to workqueue changes

[Tejun Heo]

Hello,

On Mon, Jul 01, 2024 at 02:52:19PM +0100, Daniel P. Berrangé wrote:
...
> None the less, I'd suggest that virt-manager should be a explicitly
> asking for sockets=1,cores=N, as that has broader guest OS compatibility.

+1. Multiple sockets is pretty uncommon and often comes with significant
performance implications - e.g. if VM is also splitting memory into N nodes,
that can lead to significant higher overhead during reclaim due to node
imbalances and premature OOMs. If the reported topology is not real, it
makes a lot more sense to keep it basic.

Thanks.

-- 
tejun

Thread for updating minor properties of tracked regressions [consider ignoring it!]

[Thorsten Leemhuis]

Hi! This is the start of a thread I'll use for updating minor properties
of regressions tracked by regzbot. Consider telling your mailer to
ignore this thread, the replies are unlikely to be of relevance for you,
but done here to ensure a public paper trail.

Interacting with regzbot from arbitrary places is a recently added
feature of regzbot usable via the new '#regzbot report <url>' command.
The indented use of this feature is for updating minor aspect of
regressions already tracked, e.g. "change the title" or "tell regzbot
about a related discussion". Such things until now had to be done using
regzbot commands in a direct or indirect reply to report -- but there
they would mainly be noise for most others and just add to the huge
amount of mail people already get. It's also takes a more time to send a
lot of individual mails instead of preparing just one.

The new command will be documented on
https://gitlab.com/knurd42/regzbot/ once it got a bit more testing.

Ciao, Thorsten

Re: Thread for updating minor properties of tracked regressions [consider ignoring it!]

[Christian Heusel]

[-- Attachment #1: Type: text/plain, Size: 1332 bytes --]

On 24/03/09 11:18AM, Thorsten Leemhuis wrote:
> Hi! This is the start of a thread I'll use for updating minor properties
> of regressions tracked by regzbot. Consider telling your mailer to
> ignore this thread, the replies are unlikely to be of relevance for you,
> but done here to ensure a public paper trail.
> 
> Interacting with regzbot from arbitrary places is a recently added
> feature of regzbot usable via the new '#regzbot report <url>' command.
> The indented use of this feature is for updating minor aspect of
> regressions already tracked, e.g. "change the title" or "tell regzbot
> about a related discussion". Such things until now had to be done using
> regzbot commands in a direct or indirect reply to report -- but there
> they would mainly be noise for most others and just add to the huge
> amount of mail people already get. It's also takes a more time to send a
> lot of individual mails instead of preparing just one.
> 
> The new command will be documented on
> https://gitlab.com/knurd42/regzbot/ once it got a bit more testing.
> 
> Ciao, Thorsten
> 

#regzbot report: https://lore.kernel.org/linux-kernel/32fd8274-d5f-3eca-f5d2-1a9117fd8edb@redhat.com/
#regzbot introduced: 63c5484e74952f60f5810256bd69814d167b8d22
#regzbot summary: workqueue: dm-crypt performance regression

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

dm-crypt performance regression due to workqueue changes (was: Re: Thread for updating minor properties of tracked regressions [consider ignoring it!])

[Thorsten Leemhuis]

On 08.07.24 00:54, Christian Heusel wrote:
> On 24/03/09 11:18AM, Thorsten Leemhuis wrote:
>> Hi! This is the start of a thread I'll use for updating minor properties
>> of regressions tracked by regzbot. Consider telling your mailer to
>> ignore this thread, the replies are unlikely to be of relevance for you,
>> but done here to ensure a public paper trail.
> 
> #regzbot report: https://lore.kernel.org/linux-kernel/32fd8274-d5f-3eca-f5d2-1a9117fd8edb@redhat.com/
> #regzbot introduced: 63c5484e74952f60f5810256bd69814d167b8d22
> #regzbot summary: workqueue: dm-crypt performance regression

Christian, thx for adding this, but please allow me to ask: what's your
intention here?

Is this something like "I have seen a regression report and all
regressions must be fixed" or "I or someone else I know is affected by
this and we think this should be fixed".

Because the thing is: the former might not be enough to convince
developers to fix this, as that thread are mainly developers debating
the change and the culprit is in the tree for a while already; but the
latter case mentioned above might be. At least that's how I would assume
Linus stance would be in this case -- but I might be wrong there, hard
to say.

Ciao, Thorsten

Re: dm-crypt performance regression due to workqueue changes (was: Re: Thread for updating minor properties of tracked regressions [consider ignoring it!])

[Christian Heusel]

[-- Attachment #1: Type: text/plain, Size: 1985 bytes --]

On 24/07/08 09:08AM, Thorsten Leemhuis wrote:
> On 08.07.24 00:54, Christian Heusel wrote:
> > On 24/03/09 11:18AM, Thorsten Leemhuis wrote:
> >> Hi! This is the start of a thread I'll use for updating minor properties
> >> of regressions tracked by regzbot. Consider telling your mailer to
> >> ignore this thread, the replies are unlikely to be of relevance for you,
> >> but done here to ensure a public paper trail.
> > 
> > #regzbot report: https://lore.kernel.org/linux-kernel/32fd8274-d5f-3eca-f5d2-1a9117fd8edb@redhat.com/
> > #regzbot introduced: 63c5484e74952f60f5810256bd69814d167b8d22
> > #regzbot summary: workqueue: dm-crypt performance regression
> 
> Christian, 

Hey Thorsten,

> thx for adding this, but please allow me to ask: what's your
> intention here?
> 
> Is this something like "I have seen a regression report and all
> regressions must be fixed" or "I or someone else I know is affected by
> this and we think this should be fixed".

I have come across the linked report on Social Media[0] and thought it
would be good to have it tracked by regzbot since it looked like a valid
regression, so its a bit a mix of the two.

Is this creating significant overhead for you or anybody else? 
AFAIU the report will be autoclosed once a fix lands, so I thought
giving it some visibility on the regression overview wouldn't hurt.

> Because the thing is: the former might not be enough to convince
> developers to fix this, as that thread are mainly developers debating
> the change and the culprit is in the tree for a while already; but the
> latter case mentioned above might be. At least that's how I would assume
> Linus stance would be in this case -- but I might be wrong there, hard
> to say.

I think for a case where I need this fixed myself I would just get
myself involved in the discussion directly.

> Ciao, Thorsten

Have a great week!
Chris

[0]: https://chaos.social/@rfr@tilde.zone/112724184490069659

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: dm-crypt performance regression due to workqueue changes

[Thorsten Leemhuis]

On 08.07.24 12:51, Christian Heusel wrote:
> On 24/07/08 09:08AM, Thorsten Leemhuis wrote:
>> On 08.07.24 00:54, Christian Heusel wrote:
>>> On 24/03/09 11:18AM, Thorsten Leemhuis wrote:
>>>> Hi! This is the start of a thread I'll use for updating minor properties
>>>> of regressions tracked by regzbot. Consider telling your mailer to
>>>> ignore this thread, the replies are unlikely to be of relevance for you,
>>>> but done here to ensure a public paper trail.
>>>
>>> #regzbot report: https://lore.kernel.org/linux-kernel/32fd8274-d5f-3eca-f5d2-1a9117fd8edb@redhat.com/
>>> #regzbot introduced: 63c5484e74952f60f5810256bd69814d167b8d22
>>> #regzbot summary: workqueue: dm-crypt performance regression
>>
>> thx for adding this, but please allow me to ask: what's your
>> intention here?
>>
>> Is this something like "I have seen a regression report and all
>> regressions must be fixed" or "I or someone else I know is affected by
>> this and we think this should be fixed".
> 
> I have come across the linked report on Social Media[0] and thought it
> would be good to have it tracked by regzbot since it looked like a valid
> regression, so its a bit a mix of the two.

:-D

> Is this creating significant overhead for you or anybody else? 

It does, as I basically go through the list of tracked regressions every
few days to prevent something along the lines of "regzbot is yet another
bug tracker where the data does not match reality and where nobody is
podding stalled reports when needed". And every entry obviously takes
some time.

> AFAIU the report will be autoclosed once a fix lands, so I thought
> giving it some visibility on the regression overview wouldn't hurt.

If it's fixed with a proper Link: or Closes: tag, yes. But from the
discussion it looks a lot like this is a bug nobody wants to address.
Without a proper "I'm affected by this" report from a user (not sure if
Mikulas reports counts) it might not be worth trying to change that by
prodding the developers.

Ciao, Thorsten

Re: dm-crypt performance regression due to workqueue changes

[Christian Heusel]

[-- Attachment #1: Type: text/plain, Size: 1561 bytes --]

On 24/07/08 01:16PM, Thorsten Leemhuis wrote:
> On 08.07.24 12:51, Christian Heusel wrote:
> 
> > Is this creating significant overhead for you or anybody else? 
> 
> It does, as I basically go through the list of tracked regressions every
> few days to prevent something along the lines of "regzbot is yet another
> bug tracker where the data does not match reality and where nobody is
> podding stalled reports when needed". And every entry obviously takes
> some time.

So what would you suggest on how I should deal with this in the future?
I have no problem re-reviewing the status for the ones I have added to
regzbot, but I can also just (fully) leave adding regressions (that I
did not report/debug myself) to you.

With regard to scaling properties it would maybe be good to get some
more people involved, but I can also see how it causes some extra work
.. So what I want to say is that in general I'm open to help but only
when it's of actual help and not just causing extra work x)

> > AFAIU the report will be autoclosed once a fix lands, so I thought
> > giving it some visibility on the regression overview wouldn't hurt.
> 
> If it's fixed with a proper Link: or Closes: tag, yes. But from the
> discussion it looks a lot like this is a bug nobody wants to address.
> Without a proper "I'm affected by this" report from a user (not sure if
> Mikulas reports counts) it might not be worth trying to change that by
> prodding the developers.

Alright, thanks for the feedback!

> Ciao, Thorsten

Cheers,
Chris

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: dm-crypt performance regression due to workqueue changes

[Thorsten Leemhuis]

On 08.07.24 19:03, Christian Heusel wrote:
> On 24/07/08 01:16PM, Thorsten Leemhuis wrote:
>> On 08.07.24 12:51, Christian Heusel wrote:
>>
>>> Is this creating significant overhead for you or anybody else? 
>>
>> It does, as I basically go through the list of tracked regressions every
>> few days to prevent something along the lines of "regzbot is yet another
>> bug tracker where the data does not match reality and where nobody is
>> podding stalled reports when needed". And every entry obviously takes
>> some time.
> 
> So what would you suggest on how I should deal with this in the future?

It was totally fine to add this, don't worry. But at the same time it
looks like the discussion kinda ended already for now. So what is needed
now is someone that prods it and asks questions like "Mikulas, are you
satisfied with changing the config? Or do you think/should we consider
this something that should be fixed? Is it maybe possible to detect this
odd configuration and have a quirk for it to avoid the problem? Or is
that considered too much work for a corner case only found after 10
months and thus likely not something many people care about?".

But there are dragons here, the right tone is important: it's easy to
upset people and that does not help anyone in the end. So a blunt reply
along the lines of "this is a regression that has to be fixed" would be
a really bad idea.

Ciao, Thorsten