[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v4] hw/nvme: Use ioeventfd to handle doorbell updates
From: |
Klaus Jensen |
Subject: |
Re: [PATCH v4] hw/nvme: Use ioeventfd to handle doorbell updates |
Date: |
Tue, 26 Jul 2022 14:08:09 +0200 |
On Jul 26 13:32, Klaus Jensen wrote:
> On Jul 26 13:24, Klaus Jensen wrote:
> > On Jul 26 12:09, Klaus Jensen wrote:
> > > On Jul 26 11:19, Klaus Jensen wrote:
> > > > On Jul 26 15:55, Jinhao Fan wrote:
> > > > > at 3:41 PM, Klaus Jensen <its@irrelevant.dk> wrote:
> > > > >
> > > > > > On Jul 26 15:35, Jinhao Fan wrote:
> > > > > >> at 4:55 AM, Klaus Jensen <its@irrelevant.dk> wrote:
> > > > > >>
> > > > > >>> We have a regression following this patch that we need to address.
> > > > > >>>
> > > > > >>> With this patch, issuing a reset on the device (`nvme reset
> > > > > >>> /dev/nvme0`
> > > > > >>> will do the trick) causes QEMU to hog my host cpu at 100%.
> > > > > >>>
> > > > > >>> I'm still not sure what causes this. The trace output is a bit
> > > > > >>> inconclusive still.
> > > > > >>>
> > > > > >>> I'll keep looking into it.
> > > > > >>
> > > > > >> I cannot reproduce this bug. I just start the VM and used `nvme
> > > > > >> reset
> > > > > >> /dev/nvme0`. Did you do anything before the reset?
> > > > > >
> > > > > > Interesting and thanks for checking! Looks like a kernel issue then!
> > > > > >
> > > > > > I remember that I'm using a dev branch (nvme-v5.20) of the kernel
> > > > > > and
> > > > > > reverting to a stock OS kernel did not produce the bug.
> > > > >
> > > > > I’m using 5.19-rc4 which I pulled from linux-next on Jul 1. It works
> > > > > ok on
> > > > > my machine.
> > > >
> > > > Interesting. I can reproduce on 5.19-rc4 from torvalds tree. Can you
> > > > drop your qemu command line here?
> > > >
> > > > This is mine.
> > > >
> > > > /home/kbj/work/src/qemu/build/x86_64-softmmu/qemu-system-x86_64 \
> > > > -nodefaults \
> > > > -display "none" \
> > > > -machine "q35,accel=kvm,kernel-irqchip=split" \
> > > > -cpu "host" \
> > > > -smp "4" \
> > > > -m "8G" \
> > > > -device "intel-iommu" \
> > > > -netdev "user,id=net0,hostfwd=tcp::2222-:22" \
> > > > -device "virtio-net-pci,netdev=net0" \
> > > > -device "virtio-rng-pci" \
> > > > -drive
> > > > "id=boot,file=/home/kbj/work/vol/machines/img/nvme.qcow2,format=qcow2,if=virtio,discard=unmap,media=disk,read-only=no"
> > > > \
> > > > -device "pcie-root-port,id=pcie_root_port1,chassis=1,slot=0" \
> > > > -device "nvme,id=nvme0,serial=deadbeef,bus=pcie_root_port1,mdts=7" \
> > > > -drive
> > > > "id=null,if=none,file=null-co://,file.read-zeroes=on,format=raw" \
> > > > -device
> > > > "nvme-ns,id=nvm-1,drive=nvm-1,bus=nvme0,nsid=1,drive=null,logical_block_size=4096,physical_block_size=4096"
> > > > \
> > > > -pidfile "/home/kbj/work/vol/machines/run/null/pidfile" \
> > > > -kernel "/home/kbj/work/src/kernel/linux/arch/x86_64/boot/bzImage" \
> > > > -append "root=/dev/vda1 console=ttyS0,115200 audit=0 intel_iommu=on" \
> > > > -virtfs
> > > > "local,path=/home/kbj/work/src/kernel/linux,security_model=none,readonly=on,mount_tag=kernel_dir"
> > > > \
> > > > -serial "mon:stdio" \
> > > > -d "guest_errors" \
> > > > -D "/home/kbj/work/vol/machines/log/null/qemu.log" \
> > > > -trace "pci_nvme*"
> > >
> > > Alright. It was *some* config issue with my kernel. Reverted to a
> > > defconfig + requirements and the issue went away.
> > >
> >
> > And it went away because I didn't include iommu support in that kernel (and
> > its
> > not enabled by default on the stock OS kernel).
> >
> > > I'll try to track down what happended, but doesnt look like qemu is at
> > > fault here.
> >
> > OK. So.
> >
> > I can continue to reproduce this if the machine has a virtual intel iommu
> > enabled. And it only happens when this commit is applied.
> >
> > I even backported this patch (and the shadow doorbell patch) to v7.0 and
> > v6.2
> > (i.e. no SRIOV or CC logic changes that could be buggy) and it still
> > exhibits
> > this behavior. Sometimes QEMU coredumps on poweroff and I managed to grab
> > one:
> >
> > Program terminated with signal SIGSEGV, Segmentation fault.
> > #0 nvme_process_sq (opaque=0x556329708110) at ../hw/nvme/ctrl.c:5720
> > 5720 NvmeCQueue *cq = n->cq[sq->cqid];
> > [Current thread is 1 (Thread 0x7f7363553cc0 (LWP 2554896))]
> > (gdb) bt
> > #0 nvme_process_sq (opaque=0x556329708110) at ../hw/nvme/ctrl.c:5720
> > #1 0x0000556326e82e28 in nvme_sq_notifier (e=0x556329708148) at
> > ../hw/nvme/ctrl.c:3993
> > #2 0x000055632738396a in aio_dispatch_handler (ctx=0x5563291c3160,
> > node=0x55632a228b60) at ../util/aio-posix.c:329
> > #3 0x0000556327383b22 in aio_dispatch_handlers (ctx=0x5563291c3160) at
> > ../util/aio-posix.c:372
> > #4 0x0000556327383b78 in aio_dispatch (ctx=0x5563291c3160) at
> > ../util/aio-posix.c:382
> > #5 0x000055632739d748 in aio_ctx_dispatch (source=0x5563291c3160,
> > callback=0x0, user_data=0x0) at ../util/async.c:311
> > #6 0x00007f7369398163 in g_main_context_dispatch () at
> > /usr/lib64/libglib-2.0.so.0
> > #7 0x00005563273af279 in glib_pollfds_poll () at ../util/main-loop.c:232
> > #8 0x00005563273af2f6 in os_host_main_loop_wait (timeout=0x1dbe22c0) at
> > ../util/main-loop.c:255
> > #9 0x00005563273af404 in main_loop_wait (nonblocking=0x0) at
> > ../util/main-loop.c:531
> > #10 0x00005563270714d9 in qemu_main_loop () at ../softmmu/runstate.c:726
> > #11 0x0000556326c7ea46 in main (argc=0x2e, argv=0x7ffc6977f198,
> > envp=0x7ffc6977f310) at ../softmmu/main.c:50
> >
> > At this point, there should not be any CQ/SQs (I detached the device from
> > the
> > kernel driver which deletes all queues and bound it to vfio-pci instead),
> > but
> > somehow a stale notifier is called on poweroff and the queue is bogus,
> > causing
> > the segfault.
> >
> > (gdb) p cq->cqid
> > $2 = 0x7880
> >
> > My guess would be that we are not cleaning up the notifier properly.
> > Currently
> > we do this
> >
> > if (cq->ioeventfd_enabled) {
> > memory_region_del_eventfd(&n->iomem,
> > 0x1000 + offset, 4, false, 0,
> > &cq->notifier);
> > event_notifier_cleanup(&cq->notifier);
> > }
> >
> >
> > Any ioeventfd experts that has some insights into what we are doing
> > wrong here? Something we need to flush? I tried with a test_and_clear on
> > the eventfd but that didnt do the trick.
> >
> > I think we'd need to revert this until we can track down what is going
> > wrong.
>
> One more thing - I now also triggered the coredump with just a `modprobe
> vfio-pci` following a `nvme reset /dev/nvme0`.
>
> Similar backtrace.
Alright. Forget about the iommu, that was just a coincidence.
This patch seems to fix it. I guess it is the
event_notifier_set_handler(..., NULL) that does the trick, but I'd like
to understand why ;)
diff --git i/hw/nvme/ctrl.c w/hw/nvme/ctrl.c
index 533ad14e7a61..3bc3c6bfbe78 100644
--- i/hw/nvme/ctrl.c
+++ w/hw/nvme/ctrl.c
@@ -4238,7 +4238,9 @@ static void nvme_cq_notifier(EventNotifier *e)
NvmeCQueue *cq = container_of(e, NvmeCQueue, notifier);
NvmeCtrl *n = cq->ctrl;
- event_notifier_test_and_clear(&cq->notifier);
+ if (!event_notifier_test_and_clear(e)) {
+ return;
+ }
nvme_update_cq_head(cq);
@@ -4275,7 +4277,9 @@ static void nvme_sq_notifier(EventNotifier *e)
{
NvmeSQueue *sq = container_of(e, NvmeSQueue, notifier);
- event_notifier_test_and_clear(&sq->notifier);
+ if (!event_notifier_test_and_clear(e)) {
+ return;
+ }
nvme_process_sq(sq);
}
@@ -4307,6 +4311,8 @@ static void nvme_free_sq(NvmeSQueue *sq, NvmeCtrl *n)
if (sq->ioeventfd_enabled) {
memory_region_del_eventfd(&n->iomem,
0x1000 + offset, 4, false, 0, &sq->notifier);
+ event_notifier_set_handler(&sq->notifier, NULL);
+ nvme_sq_notifier(&sq->notifier);
event_notifier_cleanup(&sq->notifier);
}
g_free(sq->io_req);
@@ -4697,6 +4703,8 @@ static void nvme_free_cq(NvmeCQueue *cq, NvmeCtrl *n)
if (cq->ioeventfd_enabled) {
memory_region_del_eventfd(&n->iomem,
0x1000 + offset, 4, false, 0, &cq->notifier);
+ event_notifier_set_handler(&cq->notifier, NULL);
+ nvme_cq_notifier(&cq->notifier);
event_notifier_cleanup(&cq->notifier);
}
if (msix_enabled(&n->parent_obj)) {
signature.asc
Description: PGP signature
- Re: [PATCH v4] hw/nvme: Use ioeventfd to handle doorbell updates, (continued)
- Re: [PATCH v4] hw/nvme: Use ioeventfd to handle doorbell updates, Klaus Jensen, 2022/07/25
- Re: [PATCH v4] hw/nvme: Use ioeventfd to handle doorbell updates, Jinhao Fan, 2022/07/26
- Re: [PATCH v4] hw/nvme: Use ioeventfd to handle doorbell updates, Klaus Jensen, 2022/07/26
- Re: [PATCH v4] hw/nvme: Use ioeventfd to handle doorbell updates, Jinhao Fan, 2022/07/26
- Re: [PATCH v4] hw/nvme: Use ioeventfd to handle doorbell updates, Klaus Jensen, 2022/07/26
- Re: [PATCH v4] hw/nvme: Use ioeventfd to handle doorbell updates, Klaus Jensen, 2022/07/26
- Re: [PATCH v4] hw/nvme: Use ioeventfd to handle doorbell updates, Klaus Jensen, 2022/07/26
- Re: [PATCH v4] hw/nvme: Use ioeventfd to handle doorbell updates, Klaus Jensen, 2022/07/26
- Re: [PATCH v4] hw/nvme: Use ioeventfd to handle doorbell updates,
Klaus Jensen <=
- Re: [PATCH v4] hw/nvme: Use ioeventfd to handle doorbell updates, Klaus Jensen, 2022/07/27
- Re: [PATCH v4] hw/nvme: Use ioeventfd to handle doorbell updates, Jinhao Fan, 2022/07/27
- Re: [PATCH v4] hw/nvme: Use ioeventfd to handle doorbell updates, Klaus Jensen, 2022/07/27
- Re: [PATCH v4] hw/nvme: Use ioeventfd to handle doorbell updates, Jinhao Fan, 2022/07/27
Re: [PATCH v4] hw/nvme: Use ioeventfd to handle doorbell updates, Stefan Hajnoczi, 2022/07/26