On 31 Jan 2024, at 16:27, Edwin Torok <edwin.torok@xxxxxxxxx> wrote:
On 31 Jan 2024, at 11:17, Christian Lindig <christian.lindig@xxxxxxxxx> wrote:
On 31 Jan 2024, at 10:52, Edwin Török <edwin.torok@xxxxxxxxx> wrote:
Now that we no longer have a hashtable inside we can make Quota.t pure,
and push the mutable update to its callers.
Store.t already had a mutable Quota.t field.
No functional change.
Acked-by: Christian Lindig <christian.lindig@xxxxxxxxx>
This is shifting copying working to GC work, at least potentially. I would agree that this is a good trade-off and the code looks correct to me. But I think we should see more testing and benchmarking before merging this unless we are fine merging speculative improvements.
— C
I’ve written this [1] microbenchmark which creates ~300_000 entries in xenstore, assigns quota to 1000 domains and then measure how long it takes to list xenstore
(It doesn’t matter whether these domains exist or not).
It shows a measurable improvement with this patch, once I’ve run a more realistic test on the original machine with 1000 real VMs I’ll post those results too:
The machine that can run this test is offline now due to a lab move, but I managed to get this data before it went away, and I think this patch series is ready to be committed.
(There are of course still hashtbl in the flame graph, due to the well-known inefficient poll_select implementation, and we see hashtbl iteration as a parent caller, which is fine)
IMHO this means the patch series is a worthwhile improvement: it removes a codepath that was previously a hotspot completely from oxenstored.
The timings on the test also show improvements with this patch:
Booting 575 VMs:
* without this patch series: 1099s
* with this patch series: 604s
Booting 626 VMs:
* without this patch series: 4027s
* with this patch series: 2115s
Booting 627 VMs:
* without this patch series: 4038s
* with this patch series: 4120s
This shows that *with* the patch series the system scales better until it hits a breaking point around 627 VMs where everything is ~equally slow
Not everything is ideal, there is also a slowdown around the 789 booted VM mark:
* without this patch series: 168s
* with this patch series: 394s
I wouldn’t worry about this result, because by this point some VMs have already crashed, and I’d consider the test to have failed by this point. What results you get at this point depends on how much CPU oxenstored gets compared to the qemus ioreq handler that is spinning handling the crash on the serial port.
To actually reach 1000 VMs it will require more fixes outsides of oxenstored (e.g. wrt to using correct groups with tapdisk, qemu, etc., or making qemu better cope with flooding on serial port from the guest), and probably some fixes to the poll_select in oxenstored that I may address in a future patch series.
Best regards,
—Edwin
On an Intel Xeon Gold 6354 @ 3.0 Ghz:
* without my patch: 12.756 +- 0.152 seconds time elapsed ( +- 1.19% )
* with my patch: 5.6051 +- 0.0467 seconds time elapsed ( +- 0.83% )
[1]
# cat >create.py <<EOF
#!/usr/bin/env python3
from xen.lowlevel.xs import xs
xenstore = xs()
for i in range(1,1000):
txn = xenstore.transaction_start()
for j in range(1,10):
for k in range(1,30):
path=f"/quotatest/{i}/{j}/{k}/x"
xenstore.write(txn, path, "val")
# assign quota to domid i
xenstore.set_permissions(txn, path, [{"dom": i}])
xenstore.transaction_end(txn)
EOF
# python3 create.py
# perf stat -r 10 sh -c 'xenstore-ls $(for i in $(seq 1 100); do echo "/quotatest/$i"; done) >/dev/null’
Best regards,
—Edwin
