Hi, On 2022-08-15 16:21:51 -0400, Michael S. Tsirkin wrote:> On Mon, Aug 15, 2022 at 10:46:17AM -0700, Andres Freund wrote: > > Hi, > > > > On 2022-08-15 12:50:52 -0400, Michael S. Tsirkin wrote: > > > On Mon, Aug 15, 2022 at 09:45:03AM -0700, Andres Freund wrote: > > > > Hi, > > > > > > > > On 2022-08-15 11:40:59 -0400, Michael S. Tsirkin wrote: > > > > > OK so this gives us a quick revert as a solution for now. > > > > > Next, I would appreciate it if you just try this simple hack. > > > > > If it crashes we either have a long standing problem in virtio > > > > > code or more likely a gcp bug where it can't handle smaller > > > > > rings than what device requestes. > > > > > Thanks! > > > > > > > > I applied the below and the problem persists. > > > > > > > > [...] > > > > > > Okay! > > > > Just checking - I applied and tested this atop 6.0-rc1, correct? Or did you > > want me to test it with the 762faee5a267 reverted? I guess what you're trying > > to test if a smaller queue than what's requested you'd want to do so without > > the problematic patch applied... > > > > > > Either way, I did this, and there are no issues that I could observe. No > > oopses, no broken networking. But: > > > > To make sure it does something I added a debugging printk - which doesn't show > > up. I assume this is at a point at least earlyprintk should work (which I see > > getting enabled via serial)? > >> Sorry if I was unclear. I wanted to know whether the change somehow > exposes a driver bug or a GCP bug. So what I wanted to do is to test > this patch on top of *5.19*, not on top of the revert.Right, the 5.19 part was clear, just the earlier test:> > > > On 2022-08-15 11:40:59 -0400, Michael S. Tsirkin wrote: > > > > > OK so this gives us a quick revert as a solution for now. > > > > > Next, I would appreciate it if you just try this simple hack. > > > > > If it crashes we either have a long standing problem in virtio > > > > > code or more likely a gcp bug where it can't handle smaller > > > > > Thanks!I wasn't sure about. After I didn't see any effect on 5.19 + your patch, I grew a bit suspicious and added the printks.> Yes I think printk should work here.The reason the debug patch didn't change anything, and that my debug printk didn't show, is that gcp uses the legacy paths... If there were a bug in the legacy path, it'd explain why the problem only shows on gcp, and not in other situations. I'll queue testing the legacy path with the equivalent change. - Andres Greetings, Andres Freund
On 2022-08-15 13:53:31 -0700, Andres Freund wrote:> The reason the debug patch didn't change anything, and that my debug printk > didn't show, is that gcp uses the legacy paths... > > If there were a bug in the legacy path, it'd explain why the problem only > shows on gcp, and not in other situations. > > I'll queue testing the legacy path with the equivalent change.Booting with the equivalent change, atop 5.19, in the legacy setup_vq() reliably causes boot to hang: [ 0.718768] ACPI: button: Sleep Button [SLPF] [ 0.721989] ACPI: \_SB_.LNKC: Enabled at IRQ 11 [ 0.722688] adebug: use legacy: 0 [ 0.722724] virtio-pci 0000:00:03.0: virtio_pci: leaving for legacy driver [ 0.724286] adebug: probe modern: -19 [ 0.727353] ACPI: \_SB_.LNKD: Enabled at IRQ 10 [ 0.728719] adebug: use legacy: 0 [ 0.728766] virtio-pci 0000:00:04.0: virtio_pci: leaving for legacy driver [ 0.730422] adebug: probe modern: -19 [ 0.733552] ACPI: \_SB_.LNKA: Enabled at IRQ 10 [ 0.734923] adebug: use legacy: 0 [ 0.734957] virtio-pci 0000:00:05.0: virtio_pci: leaving for legacy driver [ 0.736426] adebug: probe modern: -19 [ 0.739039] ACPI: \_SB_.LNKB: Enabled at IRQ 11 [ 0.740350] adebug: use legacy: 0 [ 0.740390] virtio-pci 0000:00:06.0: virtio_pci: leaving for legacy driver [ 0.742142] adebug: probe modern: -19 [ 0.747627] adebug: legacy setup_vq [ 0.748243] virtio-pci 0000:00:05.0: adebug: legacy: not limiting queue size, only 256 [ 0.751081] adebug: legacy setup_vq [ 0.751110] virtio-pci 0000:00:05.0: adebug: legacy: not limiting queue size, only 256 [ 0.754028] adebug: legacy setup_vq [ 0.754059] virtio-pci 0000:00:05.0: adebug: legacy: not limiting queue size, only 1 [ 0.757760] Serial: 8250/16550 driver, 4 ports, IRQ sharing enabled [ 0.759135] 00:03: ttyS0 at I/O 0x3f8 (irq = 4, base_baud = 115200) is a 16550A [ 0.760399] 00:04: ttyS1 at I/O 0x2f8 (irq = 3, base_baud = 115200) is a 16550A [ 0.761610] 00:05: ttyS2 at I/O 0x3e8 (irq = 6, base_baud = 115200) is a 16550A [ 0.762923] 00:06: ttyS3 at I/O 0x2e8 (irq = 7, base_baud = 115200) is a 16550A [ 0.764222] Non-volatile memory driver v1.3 [ 0.768857] adebug: legacy setup_vq [ 0.768882] virtio-pci 0000:00:06.0: adebug: legacy: not limiting queue size, only 256 [ 0.773002] Linux agpgart interface v0.103 [ 0.775424] loop: module loaded [ 0.780513] adebug: legacy setup_vq [ 0.780538] virtio-pci 0000:00:03.0: adebug: legacy: limiting queue size from 8192 to 1024 [ 0.784075] adebug: legacy setup_vq [ 0.784104] virtio-pci 0000:00:03.0: adebug: legacy: limiting queue size from 8192 to 1024 [ 0.787073] adebug: legacy setup_vq [ 0.787101] virtio-pci 0000:00:03.0: adebug: legacy: limiting queue size from 8192 to 1024 [ 0.790379] scsi host0: Virtio SCSI HBA [ 0.795968] Freeing initrd memory: 7236K Greetings, Andres Freund
On Mon, Aug 15, 2022 at 01:53:30PM -0700, Andres Freund wrote:> Hi, > > On 2022-08-15 16:21:51 -0400, Michael S. Tsirkin wrote: > > On Mon, Aug 15, 2022 at 10:46:17AM -0700, Andres Freund wrote: > > > Hi, > > > > > > On 2022-08-15 12:50:52 -0400, Michael S. Tsirkin wrote: > > > > On Mon, Aug 15, 2022 at 09:45:03AM -0700, Andres Freund wrote: > > > > > Hi, > > > > > > > > > > On 2022-08-15 11:40:59 -0400, Michael S. Tsirkin wrote: > > > > > > OK so this gives us a quick revert as a solution for now. > > > > > > Next, I would appreciate it if you just try this simple hack. > > > > > > If it crashes we either have a long standing problem in virtio > > > > > > code or more likely a gcp bug where it can't handle smaller > > > > > > rings than what device requestes. > > > > > > Thanks! > > > > > > > > > > I applied the below and the problem persists. > > > > > > > > > > [...] > > > > > > > > Okay! > > > > > > Just checking - I applied and tested this atop 6.0-rc1, correct? Or did you > > > want me to test it with the 762faee5a267 reverted? I guess what you're trying > > > to test if a smaller queue than what's requested you'd want to do so without > > > the problematic patch applied... > > > > > > > > > Either way, I did this, and there are no issues that I could observe. No > > > oopses, no broken networking. But: > > > > > > To make sure it does something I added a debugging printk - which doesn't show > > > up. I assume this is at a point at least earlyprintk should work (which I see > > > getting enabled via serial)? > > > > > > Sorry if I was unclear. I wanted to know whether the change somehow > > exposes a driver bug or a GCP bug. So what I wanted to do is to test > > this patch on top of *5.19*, not on top of the revert. > > Right, the 5.19 part was clear, just the earlier test: > > > > > > On 2022-08-15 11:40:59 -0400, Michael S. Tsirkin wrote: > > > > > > OK so this gives us a quick revert as a solution for now. > > > > > > Next, I would appreciate it if you just try this simple hack. > > > > > > If it crashes we either have a long standing problem in virtio > > > > > > code or more likely a gcp bug where it can't handle smaller > > > > > > Thanks! > > I wasn't sure about. > > After I didn't see any effect on 5.19 + your patch, I grew a bit suspicious > and added the printks. > > > > Yes I think printk should work here. > > The reason the debug patch didn't change anything, and that my debug printk > didn't show, is that gcp uses the legacy paths...Wait a second. Eureka I think! So I think GCP is not broken. I think what's broken is this patch: commit cdb44806fca2d0ad29ca644cbf1505433902ee0c Author: Xuan Zhuo <xuanzhuo at linux.alibaba.com> Date: Mon Aug 1 14:38:54 2022 +0800 virtio_pci: support the arg sizes of find_vqs() Specifically: diff --git a/drivers/virtio/virtio_pci_legacy.c b/drivers/virtio/virtio_pci_legacy.c index 2257f1b3d8ae..d75e5c4e637f 100644 --- a/drivers/virtio/virtio_pci_legacy.c +++ b/drivers/virtio/virtio_pci_legacy.c @@ -112,6 +112,7 @@ static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev, unsigned int index, void (*callback)(struct virtqueue *vq), const char *name, + u32 size, bool ctx, u16 msix_vec) { @@ -125,10 +126,13 @@ static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev, if (!num || vp_legacy_get_queue_enable(&vp_dev->ldev, index)) return ERR_PTR(-ENOENT); + if (!size || size > num) + size = num; + info->msix_vector = msix_vec; /* create the vring */ - vq = vring_create_virtqueue(index, num, + vq = vring_create_virtqueue(index, size, VIRTIO_PCI_VRING_ALIGN, &vp_dev->vdev, true, false, ctx, vp_notify, callback, name); So if you pass the size parameter for a legacy device it will try to make the ring smaller and that is not legal with legacy at all. But the driver treats legacy and modern the same, it allocates a smaller queue anyway. Lo and behold, I pass disable-modern=on to qemu and it happily corrupts memory exactly the same as GCP does. So the new find_vqs API is actually completely broken, it can not work for legacy at all and for added fun there's no way to find out that it's legacy. Maybe we should interpret the patch So I think I will also revert 04ca0b0b16f11faf74fa92468dab51b8372586cd..fe3dc04e31aa51f91dc7f741a5f76cc4817eb5b4> If there were a bug in the legacy path, it'd explain why the problem only > shows on gcp, and not in other situations. > > I'll queue testing the legacy path with the equivalent change. > > - Andres > > > Greetings, > > Andres Freund