[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH V7 08/16] virtio: introduce bus specific queue l
From: |
Cornelia Huck |
Subject: |
Re: [Qemu-devel] [PATCH V7 08/16] virtio: introduce bus specific queue limit |
Date: |
Tue, 28 Apr 2015 15:33:37 +0200 |
On Tue, 28 Apr 2015 14:47:11 +0200
"Michael S. Tsirkin" <address@hidden> wrote:
> On Tue, Apr 28, 2015 at 01:39:51PM +0200, Cornelia Huck wrote:
> > On Tue, 28 Apr 2015 12:55:40 +0200
> > "Michael S. Tsirkin" <address@hidden> wrote:
> >
> > > On Tue, Apr 28, 2015 at 12:40:07PM +0200, Cornelia Huck wrote:
> > > > On Tue, 28 Apr 2015 10:16:04 +0200
> > > > "Michael S. Tsirkin" <address@hidden> wrote:
> > > >
> > > > > On Tue, Apr 28, 2015 at 10:04:15AM +0200, Cornelia Huck wrote:
> > > > > > On Tue, 28 Apr 2015 09:14:07 +0200
> > > > > > "Michael S. Tsirkin" <address@hidden> wrote:
> > > > > >
> > > > > > > On Tue, Apr 28, 2015 at 02:13:20PM +0800, Jason Wang wrote:
> > > > > > > >
> > > > > > > >
> > > > > > > > On Tue, Apr 28, 2015 at 1:13 PM, Michael S. Tsirkin
> > > > > > > > <address@hidden> wrote:
> > > > > > > > >On Tue, Apr 28, 2015 at 11:14:04AM +0800, Jason Wang wrote:
> > > > > > > > >> On Mon, Apr 27, 2015 at 7:05 PM, Michael S. Tsirkin
> > > > > > > > >><address@hidden> wrote:
> > > > > > > > >> >On Thu, Apr 23, 2015 at 02:21:41PM +0800, Jason Wang wrote:
> > > > > > > > >> >> This patch introduces a bus specific queue limitation. It
> > > > > > > > >> >> will be
> > > > > > > > >> >> useful for increasing the limit for one of the bus without
> > > > > > > > >>disturbing
> > > > > > > > >> >> other buses.
> > > > > > > > >> >> Cc: Michael S. Tsirkin <address@hidden>
> > > > > > > > >> >> Cc: Alexander Graf <address@hidden>
> > > > > > > > >> >> Cc: Richard Henderson <address@hidden>
> > > > > > > > >> >> Cc: Cornelia Huck <address@hidden>
> > > > > > > > >> >> Cc: Christian Borntraeger <address@hidden>
> > > > > > > > >> >> Cc: Paolo Bonzini <address@hidden>
> > > > > > > > >> >> Signed-off-by: Jason Wang <address@hidden>
> > > > > > > > >> >> Reviewed-by: Cornelia Huck <address@hidden>
> > > > > > > > >> >
> > > > > > > > >> >Is this still needed if you drop the attempt to
> > > > > > > > >> >keep the limit around for old machine types?
> > > > > > > > >> If we agree to drop, we probably need transport specific
> > > > > > > > >> macro.
> > > > > > > > >
> > > > > > > > >You mean just rename VIRTIO_PCI_QUEUE_MAX to VIRTIO_QUEUE_MAX?
> > > > > > > > >Fine, why not.
> > > > > > > >
> > > > > > > > I mean keeping VIRTIO_PCI_QUEUE_MAX for pci only and just
> > > > > > > > increase pci
> > > > > > > > limit. And introduce e.g VIRTIO_PCI_QUEUE_CCW for ccw and keep
> > > > > > > > it as 64.
> > > > > > > > Since to my understanding, it's not safe to increase the limit
> > > > > > > > for all other
> > > > > > > > transports which was pointed out by Cornelia in V1:
> > > > > > > > http://permalink.gmane.org/gmane.comp.emulators.qemu/318245.
> > > > > > >
> > > > > > > I think all you need is add a check to CCW_CMD_SET_IND:
> > > > > > > limit to 64 for legacy interrupts only.
> > > > > >
> > > > > > It isn't that easy.
> > > > > >
> > > > > > What is easy is to add a check to the guest driver that fails setup
> > > > > > for
> > > > > > devices with more than 64 queues not using adapter interrupts.
> > > > > >
> > > > > > On the host side, we're lacking information when interpreting
> > > > > > CCW_CMD_SET_IND (the command does not contain a queue count, and the
> > > > > > actual number of virtqueues is not readily available.)
> > > > >
> > > > > Why isn't it available? All devices call virtio_add_queue
> > > > > as appropriate. Just fail legacy adaptors.
> > > >
> > > > Because we don't know what the guest is going to use? It is free to
> > > > use per-subchannel indicators, even if it is operating in virtio-1 mode.
> > > > >
> > > > > > We also can't
> > > > > > fence off when setting up the vqs, as this happens before we know
> > > > > > which
> > > > > > kind of indicators the guest wants to use.
> > > > > >
> > > > > > More importantly, we haven't even speced what we want to do in this
> > > > > > case. Do we want to reject SET_IND for devices with more than 64
> > > > > > queues? (Probably yes.)
> > > > > >
> > > > > > All this involves more work, and I'd prefer to do Jason's changes
> > > > > > instead as this gives us some more time to figure this out properly.
> > > > > >
> > > > > > And we haven't even considered s390-virtio yet, which I really want
> > > > > > to
> > > > > > touch as little as possible :)
> > > > >
> > > > > Well this patch does touch it anyway :)
> > > >
> > > > But only small, self-evident changes.
> > > >
> > >
> > > Sorry, I don't see what you are trying to say.
> > > There's no chance legacy interrupts work with > 64 queues.
> > > Guests should have validated the # of queues, and not
> > > attempted to use >64 queues. Looks like there's no
> > > such validation in guest, right?
> >
> > I have no idea whether > 64 queues would work with s390-virtio - it
> > might well work, but I'm not willing to extend any effort to verifying
> > that.
>
> Well this doesn't mean we won't make any changes, ever,
> just so we can reduce verification costs.
> Let's make the change everywhere, if we see issues
> we'll backtrack.
I don't like possibly breaking things with a seeing eye. And I know
that some virtio-ccw setups will break.
>
> > >
> > > Solution - don't specify this configuration with legacy guests.
> > >
> > > Modern guests work so there's value in supporting such
> > > configuration in QEMU, I don't see why we must deny it in QEMU.
> >
> > What is "legacy guest" in your context? A guest running with the legacy
> > transport or a guest using ccw but not virtio-1? A ccw guest using
> > adapter interrupts but not virtio-1 should be fine.
>
> A guest not using adapter interrupts.
There's nothing about that that's per-guest. It is a choice per-device.
In fact, the Linux guest driver falls back to classic interrupts if it
fails to setup adapter interrupts for a device - and this might happen
for large guests when the host adapter routing table is full.
>
> > >
> > > > > For s390 just check and fail at init if you like.
> > > >
> > > > What about devices that may change their number of queues? I'd really
> > > > prefer large queue numbers to be fenced off in the the individual
> > > > devices, and for that they need to be able to grab a transport-specific
> > > > queue limit.
> > >
> > > This is why I don't want bus specific limits in core,
> > > it just makes it too easy to sweep dirt under the carpet.
> > > s390 is legacy - fine, but don't perpetuate the issue
> > > in devices.
> >
> > What is "swept under the carpet" here? A device can have min(max queues
> > from transport, max queues from device type) queues. I think it's
> > easier to refuse instantiating with too many queues per device type (as
> > most will be fine with 64 queues), so I don't want that code in the
> > transport (beyond making the limit available).
> >
> > For s390 I'd like in the end:
> > - s390-virtio: legacy - keep it working as best-can-do, so I'd prefer
> > to keep it at 64 queues, even if more might work
> > - virtio-ccw, devices in legacy or virtio-1 mode: works with adapter
> > interrupts, so let's fence off setting per-subchannel indicators if a
> > device has more than 64 queues (needs work and a well thought-out
> > rejection mechanism)
> >
> > That's _in the end_: I'd like to keep ccw at 64 queues _for now_ so
> > that we don't have a rushed interface change - and at the same time, I
> > don't want to hold off pci. Makes sense?
>
> If you want to fail configurations with > 64 queues in ccw or s390,
> that's fine by me. I don't want work arounds for these bugs in virtio
> core though. So transports should not have a say in how many queues can
> be supported, but they can fail configurations they can't support if
> they want to.
Eh, isn't that a contradiction? Failing a configuration means that the
transport does indeed have a say?
- Re: [Qemu-devel] [PATCH V7 08/16] virtio: introduce bus specific queue limit, (continued)
- Re: [Qemu-devel] [PATCH V7 08/16] virtio: introduce bus specific queue limit, Jason Wang, 2015/04/27
- Re: [Qemu-devel] [PATCH V7 08/16] virtio: introduce bus specific queue limit, Michael S. Tsirkin, 2015/04/28
- Re: [Qemu-devel] [PATCH V7 08/16] virtio: introduce bus specific queue limit, Jason Wang, 2015/04/28
- Re: [Qemu-devel] [PATCH V7 08/16] virtio: introduce bus specific queue limit, Michael S. Tsirkin, 2015/04/28
- Re: [Qemu-devel] [PATCH V7 08/16] virtio: introduce bus specific queue limit, Cornelia Huck, 2015/04/28
- Re: [Qemu-devel] [PATCH V7 08/16] virtio: introduce bus specific queue limit, Michael S. Tsirkin, 2015/04/28
- Re: [Qemu-devel] [PATCH V7 08/16] virtio: introduce bus specific queue limit, Cornelia Huck, 2015/04/28
- Re: [Qemu-devel] [PATCH V7 08/16] virtio: introduce bus specific queue limit, Michael S. Tsirkin, 2015/04/28
- Re: [Qemu-devel] [PATCH V7 08/16] virtio: introduce bus specific queue limit, Cornelia Huck, 2015/04/28
- Re: [Qemu-devel] [PATCH V7 08/16] virtio: introduce bus specific queue limit, Michael S. Tsirkin, 2015/04/28
- Re: [Qemu-devel] [PATCH V7 08/16] virtio: introduce bus specific queue limit,
Cornelia Huck <=
- Re: [Qemu-devel] [PATCH V7 08/16] virtio: introduce bus specific queue limit, Michael S. Tsirkin, 2015/04/28
[Qemu-devel] [PATCH V7 09/16] virtio-ccw: introduce ccw specific queue limit, Jason Wang, 2015/04/23
[Qemu-devel] [PATCH V7 11/16] virtio-mmio: switch to bus specific queue limit, Jason Wang, 2015/04/23
[Qemu-devel] [PATCH V7 10/16] virtio-s390: switch to bus specific queue limit, Jason Wang, 2015/04/23
[Qemu-devel] [PATCH V7 12/16] virtio-pci: switch to use bus specific queue limit, Jason Wang, 2015/04/23
[Qemu-devel] [PATCH V7 13/16] virtio: introduce vector to virtqueues mapping, Jason Wang, 2015/04/23
[Qemu-devel] [PATCH V7 14/16] virtio-pci: speedup MSI-X masking and unmasking, Jason Wang, 2015/04/23
[Qemu-devel] [PATCH V7 15/16] virtio-pci: increase the maximum number of virtqueues to 513, Jason Wang, 2015/04/23