* Re: [PATCH] virtio-balloon spec: provide a version of the "silent deflate" feature that works
From: Michael S. Tsirkin @ 2012-09-06 9:44 UTC (permalink / raw)
To: Paolo Bonzini
Cc: fes, aarcange, riel, kvm, linux-kernel, mikew, yinghan,
virtualization
In-Reply-To: <50486BB2.7070108@redhat.com>
On Thu, Sep 06, 2012 at 11:24:02AM +0200, Paolo Bonzini wrote:
> Il 06/09/2012 10:47, Michael S. Tsirkin ha scritto:
> >> > - a migration from non-MUST_TELL_HOST to MUST_TELL_HOST will succeed,
> >> > which is wrong;
> >> >
> >> > - a migration from MUST_TELL_HOST to non-MUST_TELL_HOST will fail, which
> >> > is useless.
> >> >
> >> > Add instead a new feature VIRTIO_BALLOON_F_SILENT_DEFLATE, and deprecate
> >> > VIRTIO_BALLOON_F_MUST_TELL_HOST since it is never actually used.
> >> >
> >> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > Frankly I think it's a qemu migration bug. I don't see why
> > we need to tweak spec: just teach qemu to be smarter
> > during migration.
>
> Of course you can just teach QEMU to be smarter, but that would be a
> one-off hack for the only ill-defined feature that says something is
> _not_ supported. Since in practice all virtio_balloon-enbled
> hypervisors supported silent deflate (so the bit was always zero), and
> no client used it (so it was never checked), it's easier to just reverse
> the direction.
>
> In fact, it's not clear how the driver should use the feature. My guess
> is that, if it wants to use silent deflate, it tries to negotiate
> VIRTIO_BALLOON_F_MUST_TELL_HOST, and can use silent deflate if
> negotiation fails. This is against the logic of all other features.
Let's take a step back from the implementation details.
You are trying to add a new feature bit, after all.
Why? Why is silent deflate useful? This is what is
missing in all this discussion. If it is not useful
we do not need a bit for it.
> > Can you show a scenario with old driver/new hypervisor or
> > new driver/old hypervisor that fails?
>
> Suppose the driver tried to negotiate the feature as above. We then
> have the two scenarios above.
>
> In the harmless but annoying scenario, the source hypervisor doesn't
> support silent deflate, so VIRTIO_BALLOON_F_MUST_TELL_HOST has been
> negotiated successfully. The destination hypervisor supports silent
> deflate, so it does _not_ include the feature. It sees that the guest
> requests VIRTIO_BALLOON_F_MUST_TELL_HOST, and fails migration.
>
> In the incorrect scenario, you are migrating to an older hypervisor.
> The source hypervisor is newer and supports silent deflate, so
> VIRTIO_BALLOON_F_MUST_TELL_HOST was _not_ negotiated. The destination
> hypervisor does not supports silent deflate. However, the guest is not
> requesting VIRTIO_BALLOON_F_MUST_TELL_HOST, and migration succeeds.
> Next time the guest tries to do use a page from the balloon, badness
> happens, because the hypervisor does not expect it.
>
> Paolo
Sorry this is not the example I asked for. Please give and example
without migration.
Migration is qemu's problem: it is hypervisor's job to
make sure guest sees no change during migration.
It should be able to do this with any hardware it emulates,
there should be no need to change hardware to make it
"migrateable" somehow.
--
MST
^ permalink raw reply
* Re: [PATCH] virtio-blk: Fix kconfig option
From: Michael S. Tsirkin @ 2012-09-06 9:49 UTC (permalink / raw)
To: Kent Overstreet; +Cc: linux-kernel, virtualization
In-Reply-To: <20120906092512.GB5902@koverstreet-glaptop>
On Thu, Sep 06, 2012 at 02:25:12AM -0700, Kent Overstreet wrote:
> On Thu, Sep 06, 2012 at 11:44:03AM +0300, Michael S. Tsirkin wrote:
> > On Thu, Sep 06, 2012 at 12:41:13AM -0700, Kent Overstreet wrote:
> > > On Tue, Sep 04, 2012 at 03:53:53PM +0930, Rusty Russell wrote:
> > > > Kent Overstreet <koverstreet@google.com> writes:
> > > >
> > > > > CONFIG_VIRTIO isn't exposed, everything else is supposed to select it
> > > > > instead.
> > > >
> > > > This is a slight mis-understanding. It's supposed to be selected by
> > > > the particular driver, probably virtio_pci in your case.
> > >
> > > So are you saying virtio-blk depends on virtio-pci? If so, the kconfig
> > > should have that.
> > >
> > > As is, VIRTIO_BLK just has:
> > > depends on EXPERIMENTAL && VIRTIO
> > >
> > > which is flat out broken.
> >
> > I don't think anything is broken.
> > Can you show an example of a broken configuration?
>
> Do you not understand the difference between depends an selects?
> Or did you not read my original mail?
> Flip off everything in drivers -> virtio
>
> Now go to drivers -> block and try to turn on virtio-blk.
>
> It's not listed!
Yes. Because you disabled all virtio backends.
It does not make sense to have any frontends.
> Now go back to drivers -> virtio and turn on (randomly) balloon.
>
> Go back to drivers -> block, and now you can turn on virtio-blk!
>
> Do you see what's wrong with this picture?
Yes. You got unlucky with your random guess.
It's a bug in balloon kconfig: it should not
select virtio.
I sent a patch to fix that yesterday.
--
MST
^ permalink raw reply
* Re: [PATCH] virtio-balloon spec: provide a version of the "silent deflate" feature that works
From: Paolo Bonzini @ 2012-09-06 9:57 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: fes, aarcange, riel, kvm, linux-kernel, mikew, yinghan,
virtualization
In-Reply-To: <20120906094442.GA22816@redhat.com>
Il 06/09/2012 11:44, Michael S. Tsirkin ha scritto:
>> In fact, it's not clear how the driver should use the feature. My guess
>> is that, if it wants to use silent deflate, it tries to negotiate
>> VIRTIO_BALLOON_F_MUST_TELL_HOST, and can use silent deflate if
>> negotiation fails. This is against the logic of all other features.
>
> Let's take a step back from the implementation details.
> You are trying to add a new feature bit, after all.
> Why? Why is silent deflate useful? This is what is
> missing in all this discussion. If it is not useful
> we do not need a bit for it.
It is useful because it lets guests inflate the balloon aggressively,
and then use ballooned-out pages even in places where the guest OS
cannot sleep, such as kmalloc(GFP_ATOMIC).
>>> Can you show a scenario with old driver/new hypervisor or
>>> new driver/old hypervisor that fails?
>
> Sorry this is not the example I asked for. Please give and example
> without migration.
>
> Migration is qemu's problem: it is hypervisor's job to
> make sure guest sees no change during migration.
Quoting my message: "Of course you can just teach QEMU to be smarter,
but that would be a one-off hack for the only ill-defined feature that
says something is _not_ supported".
Currently migration works the same way for all virtio devices, and
assumes that features are defined only in the "positive" direction:
drivers request features if they want to use it, devices provide
features to say they support something.
Instead, in the case of this feature, the driver requests it before
relying on its lack (which is odd); the device provides if they do not
support something (which is wrong). You can see that this just cannot
provide backwards-compatibility in the device; it happens to work only
because the feature was there in the first version of the spec.
> It should be able to do this with any hardware it emulates,
> there should be no need to change hardware to make it
> "migrateable" somehow.
Of course, but if we can fix the hardware with no bad effects, let's do
that instead.
Paolo
^ permalink raw reply
* Re: [PATCH] virtio-blk: Fix kconfig option
From: Kent Overstreet @ 2012-09-06 10:02 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: linux-kernel, virtualization
In-Reply-To: <20120906094956.GB22816@redhat.com>
On Thu, Sep 06, 2012 at 12:49:56PM +0300, Michael S. Tsirkin wrote:
> On Thu, Sep 06, 2012 at 02:25:12AM -0700, Kent Overstreet wrote:
> > On Thu, Sep 06, 2012 at 11:44:03AM +0300, Michael S. Tsirkin wrote:
> > > On Thu, Sep 06, 2012 at 12:41:13AM -0700, Kent Overstreet wrote:
> > > > On Tue, Sep 04, 2012 at 03:53:53PM +0930, Rusty Russell wrote:
> > > > > Kent Overstreet <koverstreet@google.com> writes:
> > > > >
> > > > > > CONFIG_VIRTIO isn't exposed, everything else is supposed to select it
> > > > > > instead.
> > > > >
> > > > > This is a slight mis-understanding. It's supposed to be selected by
> > > > > the particular driver, probably virtio_pci in your case.
> > > >
> > > > So are you saying virtio-blk depends on virtio-pci? If so, the kconfig
> > > > should have that.
> > > >
> > > > As is, VIRTIO_BLK just has:
> > > > depends on EXPERIMENTAL && VIRTIO
> > > >
> > > > which is flat out broken.
> > >
> > > I don't think anything is broken.
> > > Can you show an example of a broken configuration?
> >
> > Do you not understand the difference between depends an selects?
> > Or did you not read my original mail?
> > Flip off everything in drivers -> virtio
> >
> > Now go to drivers -> block and try to turn on virtio-blk.
> >
> > It's not listed!
>
> Yes. Because you disabled all virtio backends.
> It does not make sense to have any frontends.
How's a user - or even another kernel developer who isn't familiar with
virtio - supposed to know that?
I still don't know what exactly a virtio backend is - the term isn't
even mentioned anywhere that I've seen.
Whatever it is though virtio-blk should be depending on _that_, not a
config option that _isn't exposed in the menu_!
> > Now go back to drivers -> virtio and turn on (randomly) balloon.
> >
> > Go back to drivers -> block, and now you can turn on virtio-blk!
> >
> > Do you see what's wrong with this picture?
>
> Yes. You got unlucky with your random guess.
> It's a bug in balloon kconfig: it should not
> select virtio.
> I sent a patch to fix that yesterday.
Then it's also a bug in the comments at the top of
drivers/virtio/Kconfig.
And besides that, how the _hell_ is a user supposed to know to turn on
VIRTIO_PCI before VIRTIO_BLK? It's not documented anywhere (if that is
what's supposed to happen! I still don't know) and even if it was
documented, having one kconfig option depend on something that's exposed
in a _completely different menu_ is just made of fail.
^ permalink raw reply
* Re: [PATCH] virtio-blk: Fix kconfig option
From: Michael S. Tsirkin @ 2012-09-06 10:18 UTC (permalink / raw)
To: Kent Overstreet; +Cc: linux-kernel, virtualization
In-Reply-To: <20120906100247.GA27355@koverstreet-glaptop>
On Thu, Sep 06, 2012 at 03:02:48AM -0700, Kent Overstreet wrote:
> On Thu, Sep 06, 2012 at 12:49:56PM +0300, Michael S. Tsirkin wrote:
> > On Thu, Sep 06, 2012 at 02:25:12AM -0700, Kent Overstreet wrote:
> > > On Thu, Sep 06, 2012 at 11:44:03AM +0300, Michael S. Tsirkin wrote:
> > > > On Thu, Sep 06, 2012 at 12:41:13AM -0700, Kent Overstreet wrote:
> > > > > On Tue, Sep 04, 2012 at 03:53:53PM +0930, Rusty Russell wrote:
> > > > > > Kent Overstreet <koverstreet@google.com> writes:
> > > > > >
> > > > > > > CONFIG_VIRTIO isn't exposed, everything else is supposed to select it
> > > > > > > instead.
> > > > > >
> > > > > > This is a slight mis-understanding. It's supposed to be selected by
> > > > > > the particular driver, probably virtio_pci in your case.
> > > > >
> > > > > So are you saying virtio-blk depends on virtio-pci? If so, the kconfig
> > > > > should have that.
> > > > >
> > > > > As is, VIRTIO_BLK just has:
> > > > > depends on EXPERIMENTAL && VIRTIO
> > > > >
> > > > > which is flat out broken.
> > > >
> > > > I don't think anything is broken.
> > > > Can you show an example of a broken configuration?
> > >
> > > Do you not understand the difference between depends an selects?
> > > Or did you not read my original mail?
> > > Flip off everything in drivers -> virtio
> > >
> > > Now go to drivers -> block and try to turn on virtio-blk.
> > >
> > > It's not listed!
> >
> > Yes. Because you disabled all virtio backends.
> > It does not make sense to have any frontends.
>
> How's a user - or even another kernel developer who isn't familiar with
> virtio - supposed to know that?
>
> I still don't know what exactly a virtio backend is - the term isn't
> even mentioned anywhere that I've seen.
>
> Whatever it is though virtio-blk should be depending on _that_, not a
> config option that _isn't exposed in the menu_!
>
> > > Now go back to drivers -> virtio and turn on (randomly) balloon.
> > >
> > > Go back to drivers -> block, and now you can turn on virtio-blk!
> > >
> > > Do you see what's wrong with this picture?
> >
> > Yes. You got unlucky with your random guess.
> > It's a bug in balloon kconfig: it should not
> > select virtio.
> > I sent a patch to fix that yesterday.
>
> Then it's also a bug in the comments at the top of
> drivers/virtio/Kconfig.
>
> And besides that, how the _hell_ is a user supposed to know to turn on
> VIRTIO_PCI before VIRTIO_BLK? It's not documented anywhere (if that is
> what's supposed to happen! I still don't know)
Well, what kind of device do you have? Tell us :)
If it's a virtio pci device,
you need to enable virtio-pci and virtio-blk.
> and even if it was
> documented, having one kconfig option depend on something that's exposed
> in a _completely different menu_ is just made of fail.
Fine, but why pick on virtio?
This is extremely common in kconfig.
For example, a ton of network drivers depend
on PCI, it's exactly the same thing.
--
MST
^ permalink raw reply
* Re: [PATCH] virtio-blk: Fix kconfig option
From: Kent Overstreet @ 2012-09-06 10:31 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: linux-kernel, virtualization
In-Reply-To: <20120906101842.GB32325@redhat.com>
On Thu, Sep 06, 2012 at 01:18:43PM +0300, Michael S. Tsirkin wrote:
> On Thu, Sep 06, 2012 at 03:02:48AM -0700, Kent Overstreet wrote:
> > On Thu, Sep 06, 2012 at 12:49:56PM +0300, Michael S. Tsirkin wrote:
> > > On Thu, Sep 06, 2012 at 02:25:12AM -0700, Kent Overstreet wrote:
> > > > On Thu, Sep 06, 2012 at 11:44:03AM +0300, Michael S. Tsirkin wrote:
> > > > > On Thu, Sep 06, 2012 at 12:41:13AM -0700, Kent Overstreet wrote:
> > > > > > On Tue, Sep 04, 2012 at 03:53:53PM +0930, Rusty Russell wrote:
> > > > > > > Kent Overstreet <koverstreet@google.com> writes:
> > > > > > >
> > > > > > > > CONFIG_VIRTIO isn't exposed, everything else is supposed to select it
> > > > > > > > instead.
> > > > > > >
> > > > > > > This is a slight mis-understanding. It's supposed to be selected by
> > > > > > > the particular driver, probably virtio_pci in your case.
> > > > > >
> > > > > > So are you saying virtio-blk depends on virtio-pci? If so, the kconfig
> > > > > > should have that.
> > > > > >
> > > > > > As is, VIRTIO_BLK just has:
> > > > > > depends on EXPERIMENTAL && VIRTIO
> > > > > >
> > > > > > which is flat out broken.
> > > > >
> > > > > I don't think anything is broken.
> > > > > Can you show an example of a broken configuration?
> > > >
> > > > Do you not understand the difference between depends an selects?
> > > > Or did you not read my original mail?
> > > > Flip off everything in drivers -> virtio
> > > >
> > > > Now go to drivers -> block and try to turn on virtio-blk.
> > > >
> > > > It's not listed!
> > >
> > > Yes. Because you disabled all virtio backends.
> > > It does not make sense to have any frontends.
> >
> > How's a user - or even another kernel developer who isn't familiar with
> > virtio - supposed to know that?
> >
> > I still don't know what exactly a virtio backend is - the term isn't
> > even mentioned anywhere that I've seen.
> >
> > Whatever it is though virtio-blk should be depending on _that_, not a
> > config option that _isn't exposed in the menu_!
> >
> > > > Now go back to drivers -> virtio and turn on (randomly) balloon.
> > > >
> > > > Go back to drivers -> block, and now you can turn on virtio-blk!
> > > >
> > > > Do you see what's wrong with this picture?
> > >
> > > Yes. You got unlucky with your random guess.
> > > It's a bug in balloon kconfig: it should not
> > > select virtio.
> > > I sent a patch to fix that yesterday.
> >
> > Then it's also a bug in the comments at the top of
> > drivers/virtio/Kconfig.
> >
> > And besides that, how the _hell_ is a user supposed to know to turn on
> > VIRTIO_PCI before VIRTIO_BLK? It's not documented anywhere (if that is
> > what's supposed to happen! I still don't know)
>
> Well, what kind of device do you have? Tell us :)
> If it's a virtio pci device,
> you need to enable virtio-pci and virtio-blk.
I run qemu with -drive if=virtio. You tell me!
Better yet, tell me how the user is supposed to figure it out!
>
> > and even if it was
> > documented, having one kconfig option depend on something that's exposed
> > in a _completely different menu_ is just made of fail.
>
> Fine, but why pick on virtio?
> This is extremely common in kconfig.
> For example, a ton of network drivers depend
> on PCI, it's exactly the same thing.
Never noticed where CONFIG_PCI is exposed in bus options?
Nope, not the same thing.
^ permalink raw reply
* Re: [PATCH] virtio-balloon spec: provide a version of the "silent deflate" feature that works
From: Michael S. Tsirkin @ 2012-09-06 10:53 UTC (permalink / raw)
To: Paolo Bonzini
Cc: fes, aarcange, riel, kvm, linux-kernel, mikew, yinghan,
virtualization
In-Reply-To: <50487382.8030303@redhat.com>
On Thu, Sep 06, 2012 at 11:57:22AM +0200, Paolo Bonzini wrote:
> Il 06/09/2012 11:44, Michael S. Tsirkin ha scritto:
> >> In fact, it's not clear how the driver should use the feature. My guess
> >> is that, if it wants to use silent deflate, it tries to negotiate
> >> VIRTIO_BALLOON_F_MUST_TELL_HOST, and can use silent deflate if
> >> negotiation fails. This is against the logic of all other features.
> >
> > Let's take a step back from the implementation details.
> > You are trying to add a new feature bit, after all.
> > Why? Why is silent deflate useful? This is what is
> > missing in all this discussion. If it is not useful
> > we do not need a bit for it.
>
> It is useful because it lets guests inflate the balloon aggressively,
> and then use ballooned-out pages even in places where the guest OS
> cannot sleep, such as kmalloc(GFP_ATOMIC).
Interesting.
Do you intend to develop a driver patch using this? I'd like to see how
that works. Because if not, IMO it's best to wait until someone asks
for it.
> >>> Can you show a scenario with old driver/new hypervisor or
> >>> new driver/old hypervisor that fails?
> >
> > Sorry this is not the example I asked for. Please give and example
> > without migration.
> >
> > Migration is qemu's problem: it is hypervisor's job to
> > make sure guest sees no change during migration.
>
> Quoting my message: "Of course you can just teach QEMU to be smarter,
> but that would be a one-off hack for the only ill-defined feature that
> says something is _not_ supported".
>
> Currently migration works the same way for all virtio devices,
> and
> assumes that features are defined only in the "positive" direction:
> drivers request features if they want to use it, devices provide
> features to say they support something.
Well this approach is buggy. If I reread features after migration what
do I see? Something changed right? So this is a bug. Migration should
not change hardware. And it is not a "one off" thing it is
fundamental for any hardware.
Fix that in qemu, and the problem goes away without spec changes.
> Instead, in the case of this feature, the driver requests it before
> relying on its lack (which is odd);
Which code in driver do you refer to?
> the device provides if they do not
> support something (which is wrong).
Not support?
It just seems to be asking guest to tell it about deflates.
If guest acks the bit, we know it will. If it does not,
it will not.
> You can see that this just cannot
> provide backwards-compatibility in the device;
Sorry I do not understand this meta argument.
There should be an example where a driver and device
fail to work together. And without migration: as
I showed migration is simply broken atm for
an unrelated reason. Otherwise all's well.
> it happens to work only
> because the feature was there in the first version of the spec.
This is how we do compatiblity in virtio. If we want driver to do
something, we add a feature and it can ack, if it does we know it will
do what we want. Another example is network announce bit. If driver
acks it, we know we do not need to send gratitious arp from qemu. You
are saying it is also broken?
> > It should be able to do this with any hardware it emulates,
> > there should be no need to change hardware to make it
> > "migrateable" somehow.
>
> Of course, but if we can fix the hardware with no bad effects, let's do
> that instead.
>
> Paolo
Don't fix what is not broken. We get to carry compatibility
in both driver and host for a long time for each feature.
Note: adding
new features adds zero value in this respect - it will not
allow simplifying the hypervisor.
--
MST
^ permalink raw reply
* Re: [PATCH] virtio-blk: Fix kconfig option
From: Michael S. Tsirkin @ 2012-09-06 11:09 UTC (permalink / raw)
To: Kent Overstreet; +Cc: linux-kernel, virtualization
In-Reply-To: <20120906103143.GA22299@moria.home.lan>
On Thu, Sep 06, 2012 at 03:31:44AM -0700, Kent Overstreet wrote:
> On Thu, Sep 06, 2012 at 01:18:43PM +0300, Michael S. Tsirkin wrote:
> > On Thu, Sep 06, 2012 at 03:02:48AM -0700, Kent Overstreet wrote:
> > > On Thu, Sep 06, 2012 at 12:49:56PM +0300, Michael S. Tsirkin wrote:
> > > > On Thu, Sep 06, 2012 at 02:25:12AM -0700, Kent Overstreet wrote:
> > > > > On Thu, Sep 06, 2012 at 11:44:03AM +0300, Michael S. Tsirkin wrote:
> > > > > > On Thu, Sep 06, 2012 at 12:41:13AM -0700, Kent Overstreet wrote:
> > > > > > > On Tue, Sep 04, 2012 at 03:53:53PM +0930, Rusty Russell wrote:
> > > > > > > > Kent Overstreet <koverstreet@google.com> writes:
> > > > > > > >
> > > > > > > > > CONFIG_VIRTIO isn't exposed, everything else is supposed to select it
> > > > > > > > > instead.
> > > > > > > >
> > > > > > > > This is a slight mis-understanding. It's supposed to be selected by
> > > > > > > > the particular driver, probably virtio_pci in your case.
> > > > > > >
> > > > > > > So are you saying virtio-blk depends on virtio-pci? If so, the kconfig
> > > > > > > should have that.
> > > > > > >
> > > > > > > As is, VIRTIO_BLK just has:
> > > > > > > depends on EXPERIMENTAL && VIRTIO
> > > > > > >
> > > > > > > which is flat out broken.
> > > > > >
> > > > > > I don't think anything is broken.
> > > > > > Can you show an example of a broken configuration?
> > > > >
> > > > > Do you not understand the difference between depends an selects?
> > > > > Or did you not read my original mail?
> > > > > Flip off everything in drivers -> virtio
> > > > >
> > > > > Now go to drivers -> block and try to turn on virtio-blk.
> > > > >
> > > > > It's not listed!
> > > >
> > > > Yes. Because you disabled all virtio backends.
> > > > It does not make sense to have any frontends.
> > >
> > > How's a user - or even another kernel developer who isn't familiar with
> > > virtio - supposed to know that?
> > >
> > > I still don't know what exactly a virtio backend is - the term isn't
> > > even mentioned anywhere that I've seen.
> > >
> > > Whatever it is though virtio-blk should be depending on _that_, not a
> > > config option that _isn't exposed in the menu_!
> > >
> > > > > Now go back to drivers -> virtio and turn on (randomly) balloon.
> > > > >
> > > > > Go back to drivers -> block, and now you can turn on virtio-blk!
> > > > >
> > > > > Do you see what's wrong with this picture?
> > > >
> > > > Yes. You got unlucky with your random guess.
> > > > It's a bug in balloon kconfig: it should not
> > > > select virtio.
> > > > I sent a patch to fix that yesterday.
> > >
> > > Then it's also a bug in the comments at the top of
> > > drivers/virtio/Kconfig.
> > >
> > > And besides that, how the _hell_ is a user supposed to know to turn on
> > > VIRTIO_PCI before VIRTIO_BLK? It's not documented anywhere (if that is
> > > what's supposed to happen! I still don't know)
> >
> > Well, what kind of device do you have? Tell us :)
> > If it's a virtio pci device,
> > you need to enable virtio-pci and virtio-blk.
>
> I run qemu with -drive if=virtio. You tell me!
-drive if= is a compatibility option. qemu makes
an effort to guess what it is you want to do.
Result is usually correct but it means people building
their own kernels get confused.
For x86 kvm the modern equivalent is:
-device virtio-blk-pci,drive=foobar -drive if=no,...
If you use this you get what you asked for :).
Yes this usage is not documented anywhere, but this is
not guest driver's problem.
> Better yet, tell me how the user is supposed to figure it out!
As usual when you do not know which driver to select.
Boot a distro kernel and look around.
Where is your virtio device? On a pci bus?
There you are.
> >
> > > and even if it was
> > > documented, having one kconfig option depend on something that's exposed
> > > in a _completely different menu_ is just made of fail.
> >
> > Fine, but why pick on virtio?
> > This is extremely common in kconfig.
> > For example, a ton of network drivers depend
> > on PCI, it's exactly the same thing.
>
> Never noticed where CONFIG_PCI is exposed in bus options?
I see it:
CONFIG_PCI:
│ Find out whether you have a PCI motherboard. PCI is the name of a │
│ bus system, i.e. the way the CPU talks to the other stuff inside │
│ your box. Other bus systems are ISA, EISA, MicroChannel (MCA) or │
│ VESA. If you have PCI, say Y, otherwise N. │
> Nope, not the same thing.
You just happen to know what PCI is but not what VIRTIO PCI is.
This is fair enough, but not sure how to help in this case.
Your patch won't help though.
--
MST
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* [PATCHv3] virtio-spec: virtio network device multiqueue support
From: Michael S. Tsirkin @ 2012-09-06 12:08 UTC (permalink / raw)
To: Jason Wang; +Cc: netdev, kvm, virtualization
Add multiqueue support to virtio network device.
Add a new feature flag VIRTIO_NET_F_MULTIQUEUE for this feature, a new
configuration field max_virtqueue_pairs to detect supported number of
virtqueues as well as a new command VIRTIO_NET_CTRL_STEERING to program
packet steering.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
--
Changes from v2:
Address Jason's comments on v2:
- Changed STEERING_HOST to STEERING_RX_FOLLOWS_TX:
this is both clearer and easier to support.
It does not look like we need a separate steering command
since host can just watch tx packets as they go.
- Moved RX and TX steering sections near each other.
- Add motivation for other changes in v2
Changes from Jason's rfc:
- reserved vq 3: this makes all rx vqs even and tx vqs odd, which
looks nicer to me.
- documented packet steering, added a generalized steering programming
command. Current modes are single queue and host driven multiqueue,
but I envision support for guest driven multiqueue in the future.
- make default vqs unused when in mq mode - this wastes some memory
but makes it more efficient to switch between modes as
we can avoid this causing packet reordering.
Rusty, could you please take a look and comment?
If this looks OK to everyone, we can proceed with finalizing the
implementation. This patch is against
eb9fc84d0d3c46438aaab190e2401a9e5409a052 in virtio-spec git tree.
diff --git a/virtio-spec.lyx b/virtio-spec.lyx
index 7a073f4..a713807 100644
--- a/virtio-spec.lyx
+++ b/virtio-spec.lyx
@@ -58,6 +58,7 @@
\html_be_strict false
\author -608949062 "Rusty Russell,,,"
\author 1531152142 "Paolo Bonzini,,,"
+\author 1986246365 "Michael S. Tsirkin"
\end_header
\begin_body
@@ -3896,6 +3897,37 @@ Only if VIRTIO_NET_F_CTRL_VQ set
\end_inset
+\change_inserted 1986246365 1346663522
+ 3: reserved
+\end_layout
+
+\begin_layout Description
+
+\change_inserted 1986246365 1346663550
+4: receiveq1.
+ 5: transmitq1.
+ 6: receiveq2.
+ 7.
+ transmitq2.
+ ...
+ 2N+2:receivqN, 2N+3:transmitqN
+\begin_inset Foot
+status open
+
+\begin_layout Plain Layout
+
+\change_inserted 1986246365 1346663558
+Only if VIRTIO_NET_F_CTRL_VQ set.
+ N is indicated by max_virtqueue_pairs field.
+\change_unchanged
+
+\end_layout
+
+\end_inset
+
+
+\change_unchanged
+
\end_layout
\begin_layout Description
@@ -4056,6 +4088,17 @@ VIRTIO_NET_F_CTRL_VLAN
\begin_layout Description
VIRTIO_NET_F_GUEST_ANNOUNCE(21) Guest can send gratuitous packets.
+\change_inserted 1986246365 1346617842
+
+\end_layout
+
+\begin_layout Description
+
+\change_inserted 1986246365 1346618103
+VIRTIO_NET_F_MULTIQUEUE(22) Device has multiple receive and transmission
+ queues.
+\change_unchanged
+
\end_layout
\end_deeper
@@ -4068,11 +4111,45 @@ configuration
\begin_inset space ~
\end_inset
-layout Two configuration fields are currently defined.
+layout
+\change_deleted 1986246365 1346671560
+Two
+\change_inserted 1986246365 1346671647
+Six
+\change_unchanged
+ configuration fields are currently defined.
The mac address field always exists (though is only valid if VIRTIO_NET_F_MAC
is set), and the status field only exists if VIRTIO_NET_F_STATUS is set.
Two read-only bits are currently defined for the status field: VIRTIO_NET_S_LIN
K_UP and VIRTIO_NET_S_ANNOUNCE.
+
+\change_inserted 1986246365 1346930950
+ The following four read-only fields only exists if VIRTIO_NET_F_MULTIQUEUE
+ is set.
+ The max_virtqueue_pairs field specifies the maximum number of each of transmit
+ and receive virtqueues that can be used for multiqueue operation.
+ The following read-only fields:
+\emph on
+current_steering_rule
+\emph default
+,
+\emph on
+reserved
+\emph default
+ and
+\emph on
+current_steering_param
+\emph default
+ store the last successful VIRTIO_NET_CTRL_STEERING
+\begin_inset CommandInset ref
+LatexCommand ref
+reference "sub:Transmit-Packet-Steering"
+
+\end_inset
+
+ command executed by driver, for debugging purposes.
+
+\change_unchanged
\begin_inset listings
inline false
@@ -4105,6 +4182,40 @@ struct virtio_net_config {
\begin_layout Plain Layout
u16 status;
+\change_inserted 1986246365 1346671221
+
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 1986246365 1346671532
+
+ u16 max_virtqueue_pairs;
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 1986246365 1346671531
+
+ u8 current_steering_rule;
+\change_unchanged
+
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 1986246365 1346671499
+
+ u8 reserved;
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 1986246365 1346671530
+
+ u16 current_steering_param;
+\change_unchanged
+
\end_layout
\begin_layout Plain Layout
@@ -4151,6 +4262,18 @@ physical
\begin_layout Enumerate
If the VIRTIO_NET_F_CTRL_VQ feature bit is negotiated, identify the control
virtqueue.
+\change_inserted 1986246365 1346618052
+
+\end_layout
+
+\begin_layout Enumerate
+
+\change_inserted 1986246365 1346618175
+If VIRTIO_NET_F_MULTIQUEUE feature bit is negotiated, identify the receive
+ and transmission queues that are going to be used in multiqueue mode.
+ Only queues that are going to be used need to be initialized.
+\change_unchanged
+
\end_layout
\begin_layout Enumerate
@@ -4168,7 +4291,11 @@ status
\end_layout
\begin_layout Enumerate
-The receive virtqueue should be filled with receive buffers.
+The receive virtqueue
+\change_inserted 1986246365 1346618180
+(s)
+\change_unchanged
+ should be filled with receive buffers.
This is described in detail below in
\begin_inset Quotes eld
\end_inset
@@ -4513,6 +4640,8 @@ Note that the header will be two bytes longer for the VIRTIO_NET_F_MRG_RXBUF
\end_inset
+\change_deleted 1986246365 1346932640
+
\end_layout
\begin_layout Subsection*
@@ -4988,8 +5117,24 @@ status open
The Guest needs to check VIRTIO_NET_S_ANNOUNCE bit in status field when
it notices the changes of device configuration.
The command VIRTIO_NET_CTRL_ANNOUNCE_ACK is used to indicate that driver
- has recevied the notification and device would clear the VIRTIO_NET_S_ANNOUNCE
- bit in the status filed after it received this command.
+ has rece
+\change_inserted 1986246365 1346663932
+i
+\change_unchanged
+v
+\change_deleted 1986246365 1346663934
+i
+\change_unchanged
+ed the notification and device would clear the VIRTIO_NET_S_ANNOUNCE bit
+ in the status fi
+\change_inserted 1986246365 1346663942
+e
+\change_unchanged
+l
+\change_deleted 1986246365 1346663943
+e
+\change_unchanged
+d after it received this command.
\end_layout
\begin_layout Standard
@@ -5004,10 +5149,298 @@ Sending the gratuitous packets or marking there are pending gratuitous packets
\begin_layout Enumerate
Sending VIRTIO_NET_CTRL_ANNOUNCE_ACK command through control vq.
+\change_deleted 1986246365 1346662247
+
\end_layout
-\begin_layout Enumerate
+\begin_layout Subsection*
+
+\change_inserted 1986246365 1346932658
+\begin_inset CommandInset label
+LatexCommand label
+name "sub:Transmit-Packet-Steering"
+
+\end_inset
+
+Transmit Packet Steering
+\end_layout
+
+\begin_layout Standard
+
+\change_inserted 1986246365 1346932658
+When VIRTIO_NET_F_MULTIQUEUE feature bit is negotiated, guest can use any
+ of multiple configured transmit queues to transmit a given packet.
+ To avoid packet reordering by device (which generally leads to performance
+ degradation) driver should attempt to utilize the same transmit virtqueue
+ for all packets of a given transmit flow.
+ For bi-directional protocols (in practice, TCP), a given network connection
+ can utilize both transmit and receive queues.
+ For best performance, packets from a single connection should utilize the
+ paired transmit and receive queues from the same virtqueue pair; for example
+ both transmitqN and receiveqN.
+ This rule makes it possible to optimize processing on the device side,
+ but this is not a hard requirement: devices should function correctly even
+ when this rule is not followed.
+\end_layout
+
+\begin_layout Standard
+
+\change_inserted 1986246365 1346932658
+Driver selects an active steering rule using VIRTIO_NET_CTRL_STEERING command
+ (this controls both which virtqueue is selected for a given packet for
+ receive and notifies the device which virtqueues are about to be used for
+ transmit).
+\end_layout
+
+\begin_layout Standard
+
+\change_inserted 1986246365 1346932658
+This command accepts a single out argument in the following format:
+\end_layout
+
+\begin_layout Standard
+
+\change_inserted 1986246365 1346932658
+\begin_inset listings
+inline false
+status open
+
+\begin_layout Plain Layout
+
+\change_inserted 1986246365 1346932658
+
+#define VIRTIO_NET_CTRL_STEERING 4
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 1986246365 1346932658
+
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 1986246365 1346932658
+
+struct virtio_net_ctrl_steering {
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 1986246365 1346932658
+
+ u8 current_steering_rule;
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 1986246365 1346932658
+
+ u8 reserved;
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 1986246365 1346932658
+
+ u16 current_steering_param;
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 1986246365 1346932658
+
+};
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 1986246365 1346932658
+
+#define VIRTIO_NET_CTRL_STEERING_SINGLE 0
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 1986246365 1346932658
+
+#define VIRTIO_NET_CTRL_STEERING_HOST 1
+\end_layout
+
+\end_inset
+
+
+\end_layout
+
+\begin_layout Standard
+
+\change_inserted 1986246365 1346932658
+The field
+\emph on
+rule
+\emph default
+ specifies the function used to select transmit virtqueue for a given packet;
+ the field
+\emph on
+param
+\emph default
+ makes it possible to pass an extra parameter if appropriate.
+ When
+\emph on
+rule
+\emph default
+ is set to VIRTIO_NET_CTRL_STEERING_SINGLE (this is the default) all packets
+ are steered to the default virtqueue transmitq (1); param is unused; this
+ is the default.
+ With any other rule, When
+\emph on
+rule
+\emph default
+ is set to VIRTIO_NET_CTRL_STEERING_RX_FOLLOWS_TX packets are steered by
+ driver to the first (
+\emph on
+param
+\emph default
++1) multiqueue virtqueues transmitq1...transmitqN; the default transmitq is
+ unused.
+ Driver must have configured all these (
+\emph on
+param
+\emph default
++1) virtqueues beforehand.
+\end_layout
+
+\begin_layout Standard
+
+\change_inserted 1986246365 1346932658
+Supported steering rules can be added and removed in the future.
+ Driver should check that the request to change the steering rule was successful
+ by checking ack values of the command.
+ As selecting a specific steering ais n optimization feature, drivers should
+ avoid hard failure and fall back on using a supported steering rule if
+ this command fails.
+ The default steering rule is VIRTIO_NET_CTRL_STEERING_SINGLE.
+ It will not be removed.
+\end_layout
+
+\begin_layout Standard
+
+\change_inserted 1986246365 1346932658
+When the steering rule is modified, some packets can still be outstanding
+ in one or more of the transmit virtqueues.
+ Since drivers might choose to modify the current steering rule at a high
+ rate (e.g.
+ adaptively in response to changes in the workload) to avoid reordering
+ packets, device is recommended to complete processing of the transmit queue(s)
+ utilized by the original steering before processing any packets delivered
+ by the modified steering rule.
+\end_layout
+
+\begin_layout Standard
+
+\change_inserted 1986246365 1346932658
+For debugging, the current steering rule can also be read from the configuration
+ space.
+\end_layout
+
+\begin_layout Subsection*
+
+\change_inserted 1986246365 1346670357
+\begin_inset CommandInset label
+LatexCommand label
+name "sub:Receive-Packet-Steering"
+
+\end_inset
+
+Receive Packet Steering
+\end_layout
+
+\begin_layout Standard
+
+\change_inserted 1986246365 1346671046
+When VIRTIO_NET_F_MULTIQUEUE feature bit is negotiated, device can use any
+ of multiple configured receive queues to pass a given packet to driver.
+ Driver controls which virtqueue is selected in practice by configuring
+ packet steering rule using VIRTIO_NET_CTRL_STEERING command, as described
+ above
+\begin_inset CommandInset ref
+LatexCommand ref
+reference "sub:Transmit-Packet-Steering"
+
+\end_inset
+
+.
+\end_layout
+
+\begin_layout Standard
+
+\change_inserted 1986246365 1346931532
+The field
+\emph on
+rule
+\emph default
+ specifies the function used to select receive virtqueue for a given packet;
+ the field
+\emph on
+param
+\emph default
+ makes it possible to pass an extra parameter if appropriate.
+ When
+\emph on
+rule
+\emph default
+ is set to VIRTIO_NET_CTRL_STEERING_SINGLE all packets are steered to the
+ default virtqueue receveq (0); param is unused; this is the default.
+ When
+\emph on
+rule
+\emph default
+ is set to VIRTIO_NET_CTRL_STEERING_RX_FOLLOWS_TX packets are steered by
+ host to the first (
+\emph on
+param
+\emph default
++1) multiqueue virtqueues receiveq1...receiveqN; the default receiveq is unused.
+ Driver must have configured all these (
+\emph on
+param
+\emph default
++1) virtqueues beforehand.
+ For best performance for bi-directional flows (such as TCP) device should
+ detect the flow to virtqueue pair mapping on transmit and select the receive
+ virtqueue from the same virtqueue pair.
+ For uni-directional flows, or when this mapping information is missing,
+ a device-specific steering function is used.
+\change_unchanged
+
+\end_layout
+
+\begin_layout Standard
+
+\change_inserted 1986246365 1346669564
+Supported steering rules can be added and removed in the future.
+ Driver should probe for supported rules by checking ack values of the command.
+\end_layout
+
+\begin_layout Standard
+
+\change_inserted 1986246365 1346932135
+When the steering rule is modified, some packets can still be outstanding
+ in one or more of the virtqueues.
+ Device is not required to wait for these packets to be consumed before
+ delivering packets using the new streering rule.
+ Drivers modifying the steering rule at a high rate (e.g.
+ adaptively in response to changes in the workload) are recommended to complete
+ processing of the receive queue(s) utilized by the original steering before
+ processing any packets delivered by the modified steering rule.
+\end_layout
+
+\begin_layout Standard
+
+\change_deleted 1986246365 1346664095
.
+
+\change_unchanged
\end_layout
^ permalink raw reply related
* Re: [PATCH] virtio-balloon spec: provide a version of the "silent deflate" feature that works
From: Paolo Bonzini @ 2012-09-06 12:13 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: fes, aarcange, riel, kvm, linux-kernel, mikew, yinghan,
virtualization
In-Reply-To: <20120906105301.GC32325@redhat.com>
Il 06/09/2012 12:53, Michael S. Tsirkin ha scritto:
>> It is useful because it lets guests inflate the balloon aggressively,
>> and then use ballooned-out pages even in places where the guest OS
>> cannot sleep, such as kmalloc(GFP_ATOMIC).
>
> Interesting.
> Do you intend to develop a driver patch using this? I'd like to see how
> that works. Because if not, IMO it's best to wait until someone asks
> for it.
It's been two months, but Frank Swiderski's patch that triggered the
debate is exactly that
(http://permalink.gmane.org/gmane.linux.kernel/1318984). However, he
didn't check VIRTIO_BALLOON_F_MUST_TELL_HOST, so he has a bug there.
>> Currently migration works the same way for all virtio devices,
>> and assumes that features are defined only in the "positive" direction:
>> drivers request features if they want to use it, devices provide
>> features to say they support something.
>
> Well this approach is buggy. If I reread features after migration what
> do I see? Something changed right? So this is a bug. Migration should
> not change hardware.
Exactly, virtio migration currently fails if it would change hardware
due to features not supported in the destination. Except for
VIRTIO_BALLOON_F_MUST_TELL_HOST, where it does not fail because it is
defined in the wrong direction.
> Fix that in qemu, and the problem goes away without spec changes.
That would be a one-off hack, for the sole feature that was defined wrong.
>> Instead, in the case of this feature, the driver requests it before
>> relying on its lack (which is odd);
>
> Which code in driver do you refer to?
I'm talking of the code Frank should have put in the driver, but he
didn't (so he has a bug). Something like
if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_MUST_TELL_HOST))
return -ENODEV;
So it has to request the feature, and then fail if the feature is
present. That's quite backwards. Everywhere else you'll find
if (!virtio_has_feature(vblk->vdev, VIRTIO_BLK_F_SCSI))
return -ENOTTY;
BUG_ON(!virtio_has_feature(vblk->vdev, VIRTIO_BLK_F_CONFIG_WCE));
if (virtio_has_feature(vblk->vdev, VIRTIO_NET_F_CTRL_VQ)) {
/* do cool stuff */
}
etc.
>> the device provides if they do not
>> support something (which is wrong).
>
> Not support? It just seems to be asking guest to tell it about deflates.
> If guest acks the bit, we know it will. If it does not,
> it will not.
No, it is the other way round. The host ultimately decides what
features are negotiated, so it doesn't ask anything to the guest. The
_guest_ is asking the host about the need for explicit deflate.
>> You can see that this just cannot
>> provide backwards-compatibility in the device;
>
> Sorry I do not understand this meta argument.
> There should be an example where a driver and device
> fail to work together.
There's nothing that you cannot work around. Use virtio_has_feature in
the device, invert the migration feature check in the driver. Why not
just _get it right_ instead?
>> it happens to work only
>> because the feature was there in the first version of the spec.
>
> This is how we do compatiblity in virtio. If we want driver to do
> something, we add a feature and it can ack, if it does we know it will
> do what we want. Another example is network announce bit. If driver
> acks it, we know we do not need to send gratitious arp from qemu. You
> are saying it is also broken?
No, it's not broken. A reverse feature, let's call it like
VIRTIO_NET_F_HOST_MUST_SEND_GARP, would be broken.
VIRTIO_NET_F_GUEST_ANNOUNCE is a "positive" feature: if set, the host
_can_ ask the guest to send a gARP, but it may also send it itself.
Similarly if VIRTIO_BALLOON_F_SILENT_DEFLATE is set, the guest _can_ use
ballooned pages directly, but may also deflate them explicitly.
Instead, VIRTIO_NET_F_HOST_MUST_SEND_GARP would be a "negative" feature:
if set, the host _may not_ rely on the guest to send a gARP. Similarly
if VIRTIO_BALLOON_F_MUST_TELL_HOST is set, the guest _may not_ use
ballooned pages directly.
There are _no_ other negative features besides
VIRTIO_BALLOON_F_MUST_TELL_HOST in the spec, and for a good
reason---because they're broken.
(Hmm, actually we have one, VIRTIO_BLK_F_RO. It is also a bit broken,
but it is not so important because it depends on user input more than
hypervisor version).
Reasoning on migration is just another way to see if the feature is
positive. During migration, new features available on the destination
can always be masked. But if removing the feature _adds_ a capability
to the hardware, it's wrong.
> Don't fix what is not broken. We get to carry compatibility
> in both driver and host for a long time for each feature.
Sure, but better fix broken things _before_ somebody uses them.
I'm not proposing to replace VIRTIO_BLK_F_RO with VIRTIO_BLK_F_RW
because it's in wide use and it would pose compatibility problems indeed.
But since VIRTIO_BALLOON_F_MUST_TELL_HOST does not exist in any source
code, neither driver nor hypervisor, we get lucky and we can instantly
deprecate it.
> Note: adding new features adds zero value in this respect - it will not
> allow simplifying the hypervisor.
Indeed, it will add one line of code to the hypervisor to advertise the
new feature.
Paolo
^ permalink raw reply
* Re: [PATCH] virtio-balloon spec: provide a version of the "silent deflate" feature that works
From: Michael S. Tsirkin @ 2012-09-06 12:51 UTC (permalink / raw)
To: Paolo Bonzini
Cc: fes, aarcange, riel, kvm, linux-kernel, mikew, yinghan,
virtualization
In-Reply-To: <5048935A.8090308@redhat.com>
On Thu, Sep 06, 2012 at 02:13:14PM +0200, Paolo Bonzini wrote:
> Il 06/09/2012 12:53, Michael S. Tsirkin ha scritto:
> >> It is useful because it lets guests inflate the balloon aggressively,
> >> and then use ballooned-out pages even in places where the guest OS
> >> cannot sleep, such as kmalloc(GFP_ATOMIC).
> >
> > Interesting.
> > Do you intend to develop a driver patch using this? I'd like to see how
> > that works. Because if not, IMO it's best to wait until someone asks
> > for it.
>
> It's been two months, but Frank Swiderski's patch that triggered the
> debate is exactly that
> (http://permalink.gmane.org/gmane.linux.kernel/1318984). However, he
> didn't check VIRTIO_BALLOON_F_MUST_TELL_HOST, so he has a bug there.
He is using a sepate device ID though, so we do not need a feature bit.
> >> Currently migration works the same way for all virtio devices,
> >> and assumes that features are defined only in the "positive" direction:
> >> drivers request features if they want to use it, devices provide
> >> features to say they support something.
> >
> > Well this approach is buggy. If I reread features after migration what
> > do I see? Something changed right? So this is a bug. Migration should
> > not change hardware.
>
> Exactly, virtio migration currently fails if it would change hardware
> due to features not supported in the destination. Except for
> VIRTIO_BALLOON_F_MUST_TELL_HOST, where it does not fail because it is
> defined in the wrong direction.
There is nothing wrong with the direction that I can see.
The bug is that migration between backends
> > Fix that in qemu, and the problem goes away without spec changes.
>
> That would be a one-off hack, for the sole feature that was defined wrong.
Not at all. It's a fundamental bug, as long as it's unfixed talking
about migration is just useless.
> >> Instead, in the case of this feature, the driver requests it before
> >> relying on its lack (which is odd);
> >
> > Which code in driver do you refer to?
>
> I'm talking of the code Frank should have put in the driver, but he
> didn't (so he has a bug). Something like
>
> if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_MUST_TELL_HOST))
> return -ENODEV;
>
> So it has to request the feature, and then fail if the feature is
> present. That's quite backwards. Everywhere else you'll find
>
> if (!virtio_has_feature(vblk->vdev, VIRTIO_BLK_F_SCSI))
> return -ENOTTY;
>
> BUG_ON(!virtio_has_feature(vblk->vdev, VIRTIO_BLK_F_CONFIG_WCE));
This is a bug BTW - we should not crash on bad device, failing probe
is the right thing.
>
> if (virtio_has_feature(vblk->vdev, VIRTIO_NET_F_CTRL_VQ)) {
> /* do cool stuff */
> }
>
> etc.
>
See above. Frank's driver does not seem to have a bug.
> >> the device provides if they do not
> >> support something (which is wrong).
> >
> > Not support? It just seems to be asking guest to tell it about deflates.
> > If guest acks the bit, we know it will. If it does not,
> > it will not.
>
> No, it is the other way round. The host ultimately decides what
> features are negotiated, so it doesn't ask anything to the guest. The
> _guest_ is asking the host about the need for explicit deflate.
Now we are arguing about words. This is why meta arguments without
specific examples are so bad.
> >> You can see that this just cannot
> >> provide backwards-compatibility in the device;
> >
> > Sorry I do not understand this meta argument.
> > There should be an example where a driver and device
> > fail to work together.
>
> There's nothing that you cannot work around. Use virtio_has_feature in
> the device, invert the migration feature check in the driver. Why not
> just _get it right_ instead?
Exactly. Bug is in qemu, fix it _there_. What you do is a work around
in spec: you declare old configurations unsupported.
> >> it happens to work only
> >> because the feature was there in the first version of the spec.
> >
> > This is how we do compatiblity in virtio. If we want driver to do
> > something, we add a feature and it can ack, if it does we know it will
> > do what we want. Another example is network announce bit. If driver
> > acks it, we know we do not need to send gratitious arp from qemu. You
> > are saying it is also broken?
>
> No, it's not broken. A reverse feature, let's call it like
> VIRTIO_NET_F_HOST_MUST_SEND_GARP, would be broken.
>
> VIRTIO_NET_F_GUEST_ANNOUNCE is a "positive" feature: if set, the host
> _can_ ask the guest to send a gARP, but it may also send it itself.
> Similarly if VIRTIO_BALLOON_F_SILENT_DEFLATE is set, the guest _can_ use
> ballooned pages directly, but may also deflate them explicitly.
>
> Instead, VIRTIO_NET_F_HOST_MUST_SEND_GARP would be a "negative" feature:
> if set, the host _may not_ rely on the guest to send a gARP. Similarly
> if VIRTIO_BALLOON_F_MUST_TELL_HOST is set, the guest _may not_ use
> ballooned pages directly.
>
> There are _no_ other negative features besides
> VIRTIO_BALLOON_F_MUST_TELL_HOST in the spec, and for a good
> reason---because they're broken.
>
> (Hmm, actually we have one, VIRTIO_BLK_F_RO. It is also a bit broken,
> but it is not so important because it depends on user input more than
> hypervisor version).
Now that we have a specific example, we can talk.
Simply, some features do not need an ack from guest:
they just tell guest something about device.
RO is one such feature.
> Reasoning on migration is just another way to see if the feature is
> positive. During migration, new features available on the destination
> can always be masked. But if removing the feature _adds_ a capability
> to the hardware, it's wrong.
Fact is, nothing except migration seems broken. This alone should
make you realize there is a bug in qemu not in driver or protocol.
> > Don't fix what is not broken. We get to carry compatibility
> > in both driver and host for a long time for each feature.
>
> Sure, but better fix broken things _before_ somebody uses them.
>
> I'm not proposing to replace VIRTIO_BLK_F_RO with VIRTIO_BLK_F_RW
> because it's in wide use and it would pose compatibility problems indeed.
>
> But since VIRTIO_BALLOON_F_MUST_TELL_HOST does not exist in any source
> code, neither driver nor hypervisor,
> we get lucky and we can instantly
> deprecate it.
Which driver are you looking at?
grep VIRTIO_BALLOON_F_MUST_TELL_HOST drivers/virtio/*c|wc -l
2
So it does not look like we can just remove it like you did. At minimum
we will need to reserve the bit.
Yes qemu does not seem to set this bit. Need to check others e.g. kvm tool etc.
Benefit seems very small. Why bother?
> > Note: adding new features adds zero value in this respect - it will not
> > allow simplifying the hypervisor.
>
> Indeed, it will add one line of code to the hypervisor to advertise the
> new feature.
>
> Paolo
So there's no point. Migration will stil be broken until it is
fixed properly.
--
MST
^ permalink raw reply
* Re: [PATCH] virtio-balloon spec: provide a version of the "silent deflate" feature that works
From: Paolo Bonzini @ 2012-09-06 13:12 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: fes, aarcange, riel, kvm, linux-kernel, mikew, yinghan,
virtualization
In-Reply-To: <20120906125107.GA1639@redhat.com>
Il 06/09/2012 14:51, Michael S. Tsirkin ha scritto:
> On Thu, Sep 06, 2012 at 02:13:14PM +0200, Paolo Bonzini wrote:
>> Il 06/09/2012 12:53, Michael S. Tsirkin ha scritto:
>>>> It is useful because it lets guests inflate the balloon aggressively,
>>>> and then use ballooned-out pages even in places where the guest OS
>>>> cannot sleep, such as kmalloc(GFP_ATOMIC).
>>>
>>> Interesting.
>>> Do you intend to develop a driver patch using this? I'd like to see how
>>> that works. Because if not, IMO it's best to wait until someone asks
>>> for it.
>>
>> It's been two months, but Frank Swiderski's patch that triggered the
>> debate is exactly that
>> (http://permalink.gmane.org/gmane.linux.kernel/1318984). However, he
>> didn't check VIRTIO_BALLOON_F_MUST_TELL_HOST, so he has a bug there.
>
> He is using a sepate device ID though, so we do not need a feature bit.
It doesn't need to, it can work just as well with the existing device.
>> Exactly, virtio migration currently fails if it would change hardware
>> due to features not supported in the destination. Except for
>> VIRTIO_BALLOON_F_MUST_TELL_HOST, where it does not fail because it is
>> defined in the wrong direction.
>
> There is nothing wrong with the direction that I can see.
> The bug is that migration between backends
...?
>> BUG_ON(!virtio_has_feature(vblk->vdev, VIRTIO_BLK_F_CONFIG_WCE));
>
> This is a bug BTW - we should not crash on bad device, failing probe
> is the right thing.
(I knew you would say that. :) The feature has been checked elsewhere,
and this code will be unreachable if the feature was not there in the
first place).
>>>> You can see that this just cannot
>>>> provide backwards-compatibility in the device;
>>>
>>> Sorry I do not understand this meta argument.
>>> There should be an example where a driver and device
>>> fail to work together.
>>
>> There's nothing that you cannot work around. Use virtio_has_feature in
>> the device, invert the migration feature check in the driver. Why not
>> just _get it right_ instead?
>
> Exactly. Bug is in qemu, fix it _there_. What you do is a work around
> in spec: you declare old configurations unsupported.
Such old configurations do not exist, so it's fine.
>> There are _no_ other negative features besides
>> VIRTIO_BALLOON_F_MUST_TELL_HOST in the spec, and for a good
>> reason---because they're broken.
>>
>> (Hmm, actually we have one, VIRTIO_BLK_F_RO. It is also a bit broken,
>> but it is not so important because it depends on user input more than
>> hypervisor version).
>
> Now that we have a specific example, we can talk.
> Simply, some features do not need an ack from guest:
> they just tell guest something about device.
> RO is one such feature.
I don't understand how that's relevant. All features that just say
"this field is there in the configuration at that offset" need no ack
from guest, yet they're expressed as "positive" features.
>> Reasoning on migration is just another way to see if the feature is
>> positive. During migration, new features available on the destination
>> can always be masked. But if removing the feature _adds_ a capability
>> to the hardware, it's wrong.
>
> Fact is, nothing except migration seems broken. This alone should
> make you realize there is a bug in qemu not in driver or protocol.
I'm not entirely sure that you understand why migration is broken and
why it is only broken for this particular feature bit (and in a more
benign way for VIRTIO_BLK_F_RO).
>>> Don't fix what is not broken. We get to carry compatibility
>>> in both driver and host for a long time for each feature.
>>
>> Sure, but better fix broken things _before_ somebody uses them.
>>
>> I'm not proposing to replace VIRTIO_BLK_F_RO with VIRTIO_BLK_F_RW
>> because it's in wide use and it would pose compatibility problems indeed.
>>
>> But since VIRTIO_BALLOON_F_MUST_TELL_HOST does not exist in any source
>> code, neither driver nor hypervisor,
>> we get lucky and we can instantly
>> deprecate it.
>
> Which driver are you looking at?
>
> grep VIRTIO_BALLOON_F_MUST_TELL_HOST drivers/virtio/*c|wc -l
> 2
Well, I am also looking at how it is used:
static unsigned int features[] = {
VIRTIO_BALLOON_F_MUST_TELL_HOST,
VIRTIO_BALLOON_F_STATS_VQ,
};
/*
* Note that if
* virtio_has_feature(vdev, VIRTIO_BALLOON_F_MUST_TELL_HOST);
* is true, we *have* to do it in this order
*/
tell_host(vb, vb->deflate_vq);
release_pages_by_pfn(vb->pfns, vb->num_pfns);
So no, it is not used with virtio_has_feature in any way that matters.
> So it does not look like we can just remove it like you did. At minimum
> we will need to reserve the bit.
Yes, reserving the bit is necessary.
> Benefit seems very small. Why bother?
Because the problem is clear, and easily solved, and I don't like time
bombs.
>>> Note: adding new features adds zero value in this respect - it will not
>>> allow simplifying the hypervisor.
>>
>> Indeed, it will add one line of code to the hypervisor to advertise the
>> new feature.
>
> So there's no point. Migration will stil be broken until it is
> fixed properly.
Migration of VIRTIO_BALLOON_F_MUST_TELL_HOST would be broken, but
VIRTIO_BALLOON_F_MUST_TELL_HOST does not exist and will not exist in the
hypervisor, so it will not be broken.
Paolo
^ permalink raw reply
* Re: [PATCHv3] virtio-spec: virtio network device multiqueue support
From: Paolo Bonzini @ 2012-09-06 16:43 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: netdev, kvm, virtualization
In-Reply-To: <20120906120828.GA1534@redhat.com>
Il 06/09/2012 14:08, Michael S. Tsirkin ha scritto:
> +#define VIRTIO_NET_CTRL_STEERING_HOST 1
You didn't change this occurrence.
Paolo
^ permalink raw reply
* Re: [PATCHv3] virtio-spec: virtio network device multiqueue support
From: Michael S. Tsirkin @ 2012-09-06 23:16 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: netdev, kvm, virtualization
In-Reply-To: <5048D2B8.4060003@redhat.com>
On Thu, Sep 06, 2012 at 06:43:36PM +0200, Paolo Bonzini wrote:
> Il 06/09/2012 14:08, Michael S. Tsirkin ha scritto:
> > +#define VIRTIO_NET_CTRL_STEERING_HOST 1
>
> You didn't change this occurrence.
Good catch, thanks!
^ permalink raw reply
* Re: [PATCH] virtio-blk: Fix kconfig option
From: Rusty Russell @ 2012-09-06 23:40 UTC (permalink / raw)
To: Kent Overstreet, Michael S. Tsirkin; +Cc: linux-kernel, virtualization
In-Reply-To: <20120906100247.GA27355@koverstreet-glaptop>
Kent Overstreet <koverstreet@google.com> writes:
> On Thu, Sep 06, 2012 at 12:49:56PM +0300, Michael S. Tsirkin wrote:
>> On Thu, Sep 06, 2012 at 02:25:12AM -0700, Kent Overstreet wrote:
>> > Do you not understand the difference between depends an selects?
>> > Or did you not read my original mail?
Now you're getting insulting.
It's normal for options to depend on other options. Sometimes they're
directly nested (eg. E1000 depends on NETDEVICES, and it's nested under
that option), sometimes they're not (eg. E1000 depends on PCI, which is
selected elsewhere).
The fact that you are only just realizing this is not Michael's problem.
>> > Flip off everything in drivers -> virtio
>> >
>> > Now go to drivers -> block and try to turn on virtio-blk.
>> >
>> > It's not listed!
>>
>> Yes. Because you disabled all virtio backends.
>> It does not make sense to have any frontends.
>
> How's a user - or even another kernel developer who isn't familiar with
> virtio - supposed to know that?
I get annoyed that menuconfig doesn't show options whose dependencies
aren't possible, too. (I got bitten the other way: it doesn't show
dependencies which can't be disabled, and I was trying to turn KALLSYMS
off).
But as I found out just last week, the '/' key allows you to find any
option, and shows what dependencies it has, and their values.
Hope that helps,
Rusty.
^ permalink raw reply
* Re: [PATCH] virtio-balloon spec: provide a version of the "silent deflate" feature that works
From: Michael S. Tsirkin @ 2012-09-06 23:41 UTC (permalink / raw)
To: Paolo Bonzini
Cc: fes, aarcange, riel, kvm, linux-kernel, mikew, yinghan,
virtualization
In-Reply-To: <1346917610-14568-1-git-send-email-pbonzini@redhat.com>
On Thu, Sep 06, 2012 at 09:46:50AM +0200, Paolo Bonzini wrote:
> VIRTIO_BALLOON_F_MUST_TELL_HOST cannot be used properly because it is a
> "negative" feature: it tells you that silent defalte is not supported.
> Right now, QEMU refuses migration if the target does not support all the
> features that were negotiated. But then:
> - a migration from non-MUST_TELL_HOST to MUST_TELL_HOST will succeed,
> which is wrong;
It need not be wrong. It depends on how host behaves.
The right thing for qemu to do would be to assume that
since this bit was not acked it can not assume specific
guest behaviour.
Even ignoring that, looking at this at a high level: basically you have
configured two different machines with different qemu flags, and are
complaning that migration did not fail cleanly.
However
- This is still a user/management bug. You should not even try
such migration. Yes I put a sanity check there but it is
just a debugging aid. It is not indended to be exhaustive.
This is far from the only case where user error might cause
problems silently.
- Yes clean failure would be nicer, if
you want to guarantee this just teach qemu to send
all host
features and verify they match on destination.
Or more generally send machine configuration.
No need to change spec or special case this bit at all.
>
> - a migration from MUST_TELL_HOST to non-MUST_TELL_HOST will fail, which
> is useless.
It is correct. device feature bits should not change across migration.
> Add instead a new feature VIRTIO_BALLOON_F_SILENT_DEFLATE, and deprecate
> VIRTIO_BALLOON_F_MUST_TELL_HOST since it is never actually used.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> virtio-spec.lyx | 36 +++++++++++++++++++++++++++++++++---
> 1 file modificato, 33 inserzioni(+), 3 rimozioni(-)
>
> diff --git a/virtio-spec.lyx b/virtio-spec.lyx
> index 7a073f4..1a25a18 100644
> --- a/virtio-spec.lyx
> +++ b/virtio-spec.lyx
> @@ -6238,6 +6238,8 @@ bits
>
> \begin_deeper
> \begin_layout Description
> +
> +\change_deleted 1531152142 1346917221
> VIRTIO_BALLOON_F_MUST_TELL_HOST
> \begin_inset space ~
> \end_inset
> @@ -6251,6 +6253,20 @@ VIRTIO_BALLOON_F_STATS_VQ
> \end_inset
>
> (1) A virtqueue for reporting guest memory statistics is present.
> +\change_inserted 1531152142 1346917193
> +
> +\end_layout
> +
> +\begin_layout Description
> +
> +\change_inserted 1531152142 1346917219
> +VIRTIO_BALLOON_F_SILENT_DEFLATE
> +\begin_inset space ~
> +\end_inset
> +
> +(2) Host does not need to be told before pages from the balloon are used.
> +\change_unchanged
> +
> \end_layout
>
> \end_deeper
> @@ -6401,9 +6417,23 @@ The driver constructs an array of addresses of memory pages it has previously
> \end_layout
>
> \begin_layout Enumerate
> -If the VIRTIO_BALLOON_F_MUST_TELL_HOST feature is set, the guest may not
> - use these requested pages until that descriptor in the deflateq has been
> - used by the device.
> +If the VIRTIO_BALLOON_F_
> +\change_deleted 1531152142 1346917234
> +MUST_TELL_HOST
> +\change_inserted 1531152142 1346917237
> +SILENT_DEFLATE
> +\change_unchanged
> + feature is
> +\change_inserted 1531152142 1346917241
> +not
> +\change_unchanged
> +set, the guest may not use these requested pages until that descriptor in
> + the deflateq has been used by the device.
> +
> +\change_inserted 1531152142 1346917253
> + If it is set, the guest may choose to not use the deflateq at all.
> +\change_unchanged
> +
> \end_layout
>
> \begin_layout Enumerate
> --
> 1.7.11.2
^ permalink raw reply
* Re: [PATCH] virtio-balloon spec: provide a version of the "silent deflate" feature that works
From: Rusty Russell @ 2012-09-06 23:45 UTC (permalink / raw)
To: Paolo Bonzini, Michael S. Tsirkin
Cc: fes, aarcange, riel, kvm, linux-kernel, mikew, yinghan,
virtualization
In-Reply-To: <5048935A.8090308@redhat.com>
Paolo Bonzini <pbonzini@redhat.com> writes:
> Instead, VIRTIO_NET_F_HOST_MUST_SEND_GARP would be a "negative" feature:
> if set, the host _may not_ rely on the guest to send a gARP. Similarly
> if VIRTIO_BALLOON_F_MUST_TELL_HOST is set, the guest _may not_ use
> ballooned pages directly.
>
> There are _no_ other negative features besides
> VIRTIO_BALLOON_F_MUST_TELL_HOST in the spec, and for a good
> reason---because they're broken.
>
> (Hmm, actually we have one, VIRTIO_BLK_F_RO. It is also a bit broken,
> but it is not so important because it depends on user input more than
> hypervisor version).
Yes, this is the key observation, and an important lesson for the
future. Thanks!
Note that these two negative features were in the original spec, where
it's assumed that every device supports them. That's not explicitly
documented, however.
I like killing the totally unused feature.
Cheers,
Rusty.
^ permalink raw reply
* Re: [PATCH v2 2/2] virtio-ring: Allocate indirect buffers from cache when possible
From: Rusty Russell @ 2012-09-06 23:49 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: linux-kernel, avi, Sasha Levin, kvm, virtualization
In-Reply-To: <20120906084526.GE17656@redhat.com>
"Michael S. Tsirkin" <mst@redhat.com> writes:
> On Thu, Sep 06, 2012 at 05:27:23PM +0930, Rusty Russell wrote:
>> "Michael S. Tsirkin" <mst@redhat.com> writes:
>> > Yes without checksum net core always linearizes packets, so yes it is
>> > screwed.
>> > For -net, skb always allocates space for 17 frags + linear part so
>> > it seems sane to do same in virtio core, and allocate, for -net,
>> > up to max_frags + 1 from cache.
>> > We can adjust it: no _SG -> 2 otherwise 18.
>>
>> But I thought it used individual buffers these days?
>
> Yes for receive, no for transmit. That's probably why
> we should have the threshold per vq, not per device, BTW.
Can someone actually run with my histogram patch and see what the real
numbers are?
I'm not convinced that the ideal 17-buffer case actually happens as much
as we think. And if it's not happening with this netperf test, we're
testing the wrong thing.
Thanks,
Rusty.
^ permalink raw reply
* Re: [PATCH v2 2/2] virtio-ring: Allocate indirect buffers from cache when possible
From: Michael S. Tsirkin @ 2012-09-07 0:06 UTC (permalink / raw)
To: Rusty Russell; +Cc: linux-kernel, avi, Sasha Levin, kvm, virtualization
In-Reply-To: <87txvahfv3.fsf@rustcorp.com.au>
On Fri, Sep 07, 2012 at 09:19:04AM +0930, Rusty Russell wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
>
> > On Thu, Sep 06, 2012 at 05:27:23PM +0930, Rusty Russell wrote:
> >> "Michael S. Tsirkin" <mst@redhat.com> writes:
> >> > Yes without checksum net core always linearizes packets, so yes it is
> >> > screwed.
> >> > For -net, skb always allocates space for 17 frags + linear part so
> >> > it seems sane to do same in virtio core, and allocate, for -net,
> >> > up to max_frags + 1 from cache.
> >> > We can adjust it: no _SG -> 2 otherwise 18.
> >>
> >> But I thought it used individual buffers these days?
> >
> > Yes for receive, no for transmit. That's probably why
> > we should have the threshold per vq, not per device, BTW.
>
> Can someone actually run with my histogram patch and see what the real
> numbers are?
>
> I'm not convinced that the ideal 17-buffer case actually happens as much
> as we think. And if it's not happening with this netperf test, we're
> testing the wrong thing.
>
> Thanks,
> Rusty.
hope to play with it next week
^ permalink raw reply
* Re: [PATCHv3] virtio-spec: virtio network device multiqueue support
From: Sasha Levin @ 2012-09-07 0:17 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: netdev, kvm, virtualization
In-Reply-To: <20120906120828.GA1534@redhat.com>
Hi Michael,
On 09/06/2012 02:08 PM, Michael S. Tsirkin wrote:
> Add multiqueue support to virtio network device.
> Add a new feature flag VIRTIO_NET_F_MULTIQUEUE for this feature, a new
> configuration field max_virtqueue_pairs to detect supported number of
> virtqueues as well as a new command VIRTIO_NET_CTRL_STEERING to program
> packet steering.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Some comments about the change:
- "The following four read-only fields only exists if VIRTIO_NET_F_MULTIQUEUE
is set." => Should be "exist" (I think).
- "When rule is set to VIRTIO_NET_CTRL_STEERING_RX_FOLLOWS_TX packets are
steered by driver to the first (param+1) multiqueue virtqueues
transmitq1...transmitqN;" - Why param+1? I thought we ignore the default
transmit/receive in this case.
- "As selecting a specific steering ais n optimization feature" - "is an".
- It's mentioned several times that the ability to read the steering rule from
the virtio-net config is there for debug reasons. Is it really necessary? I
think it's the first time I see debug features go in as part of the spec.
- I'm slightly confused, why are there both receive and transmit steering? I
can't find a difference in the way to configure the rule for transmit and
receive. Is it a plan for the future to allow different rules for tx and rx? If
so, shouldn't we use different ctrl commands (
VIRTIO_NET_CTRL_TX_STEERING/VIRTIO_NET_CTRL_RX_STEERING)?
- "When rule is set to VIRTIO_NET_CTRL_STEERING_SINGLE all packets are steered
to the default virtqueue receveq (0);" - "receiveq (0)"
Thanks,
Sasha
^ permalink raw reply
* Re: [PATCH] virtio-blk: Fix kconfig option
From: Kent Overstreet @ 2012-09-07 0:25 UTC (permalink / raw)
To: Rusty Russell; +Cc: virtualization, linux-kernel, Michael S. Tsirkin
In-Reply-To: <87zk52hg9i.fsf@rustcorp.com.au>
On Fri, Sep 07, 2012 at 09:10:25AM +0930, Rusty Russell wrote:
> Kent Overstreet <koverstreet@google.com> writes:
>
> > On Thu, Sep 06, 2012 at 12:49:56PM +0300, Michael S. Tsirkin wrote:
> >> On Thu, Sep 06, 2012 at 02:25:12AM -0700, Kent Overstreet wrote:
> >> > Do you not understand the difference between depends an selects?
> >> > Or did you not read my original mail?
>
> Now you're getting insulting.
Yes, but at least I'm not being intentionally obtuse.
> It's normal for options to depend on other options. Sometimes they're
> directly nested (eg. E1000 depends on NETDEVICES, and it's nested under
> that option), sometimes they're not (eg. E1000 depends on PCI, which is
> selected elsewhere).
>
> The fact that you are only just realizing this is not Michael's problem.
Like I said, I'm well aware of that. The issue here isn't the
dependency, it's that it depends on something that isn't exposed
anywhere!
Think about it from the user's pov. They check what VIRTIO_BLK depends
on - just VIRTIO.
So they try to figure out how to flip on VIRTIO, or what VIRTIO even is.
See how that last step might be problematic? CONFIG_VIRTIO is not
exposed! It doesn't even seem to control anything!
Go back to your example. Checking the dependencies for E1000 would tell
you the user needs to flip on CONFIG_PCI. Done. Easy.
User checks the dependencies here and... what do _you_ expect people to
do?
Look, depending on a kconfig option that's supposed to be user
controllable but isn't exposed anywhere is flat out broken. The fact
that it's in a different submenu just makes it worse.
The problem is that VIRTIO_BLK's dependencies are not actually specified
in the kconfig. If it depends on VIRTIO_PCI, that's what the kconfig
should say. If it depends on having any of multiple virtio backends
enabled, then specify that!
depends VIRTIO_PCI || VIRTIO_WHATEVER
Or if you really want to have a fake config option that's enabled if you
have any virtio backend enabled, fix the damn comments and naming!
How is anyone supposed to know that CONFIG_VIRTIO really means "any
virtio backend?" Call it VIRTIO_ANY_BACKEND if that's what it really is.
And, if that is what you're doing with CONFIG_VIRTIO (I'm still not
sure) the comment at the top of drivers/virtio/Kconfig is _wrong_:
# Virtio always gets selected by whoever wants it.
VIRTIO
tristate
How is _anyone_ supposed to know that really means "VIRTIO gets selected
by things that provide a virtio backend?"
C'mon, you've had to debug other people's code before. What would _you_
think if you were tripped up by something like that?
> >> > Flip off everything in drivers -> virtio
> >> >
> >> > Now go to drivers -> block and try to turn on virtio-blk.
> >> >
> >> > It's not listed!
> >>
> >> Yes. Because you disabled all virtio backends.
> >> It does not make sense to have any frontends.
> >
> > How's a user - or even another kernel developer who isn't familiar with
> > virtio - supposed to know that?
>
> I get annoyed that menuconfig doesn't show options whose dependencies
> aren't possible, too. (I got bitten the other way: it doesn't show
> dependencies which can't be disabled, and I was trying to turn KALLSYMS
> off).
>
> But as I found out just last week, the '/' key allows you to find any
> option, and shows what dependencies it has, and their values.
Yep, use it all the time.
^ permalink raw reply
* Re: [PATCHv3] virtio-spec: virtio network device multiqueue support
From: Michael S. Tsirkin @ 2012-09-07 0:26 UTC (permalink / raw)
To: Sasha Levin; +Cc: netdev, kvm, virtualization
In-Reply-To: <50493D04.1090408@gmail.com>
On Fri, Sep 07, 2012 at 02:17:08AM +0200, Sasha Levin wrote:
> Hi Michael,
>
> On 09/06/2012 02:08 PM, Michael S. Tsirkin wrote:
> > Add multiqueue support to virtio network device.
> > Add a new feature flag VIRTIO_NET_F_MULTIQUEUE for this feature, a new
> > configuration field max_virtqueue_pairs to detect supported number of
> > virtqueues as well as a new command VIRTIO_NET_CTRL_STEERING to program
> > packet steering.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> Some comments about the change:
>
> - "The following four read-only fields only exists if VIRTIO_NET_F_MULTIQUEUE
> is set." => Should be "exist" (I think).
>
> - "When rule is set to VIRTIO_NET_CTRL_STEERING_RX_FOLLOWS_TX packets are
> steered by driver to the first (param+1) multiqueue virtqueues
> transmitq1...transmitqN;" - Why param+1? I thought we ignore the default
> transmit/receive in this case.
This is so that all values are valid. E.g. param=0 -> use q1.
>
> - "As selecting a specific steering ais n optimization feature" - "is an".
>
> - It's mentioned several times that the ability to read the steering rule from
> the virtio-net config is there for debug reasons. Is it really necessary? I
> think it's the first time I see debug features go in as part of the spec.
I actually think we need to work on more debug features.
> - I'm slightly confused, why are there both receive and transmit steering? I
> can't find a difference in the way to configure the rule for transmit and
> receive. Is it a plan for the future to allow different rules for tx and rx? If
> so, shouldn't we use different ctrl commands (
> VIRTIO_NET_CTRL_TX_STEERING/VIRTIO_NET_CTRL_RX_STEERING)?
No. Receive steering is done by device. Documented for driver writer's
benefit. xmit streering by driver. documented for device benefit.
They must match for tcp to work well so I doubt we will have
commands to control them separately.
> - "When rule is set to VIRTIO_NET_CTRL_STEERING_SINGLE all packets are steered
> to the default virtqueue receveq (0);" - "receiveq (0)"
>
>
>
> Thanks,
> Sasha
Thanks for the comments will address!
^ permalink raw reply
* Re: [PATCH] virtio-blk: Fix kconfig option
From: Rusty Russell @ 2012-09-07 2:57 UTC (permalink / raw)
To: Kent Overstreet; +Cc: virtualization, linux-kernel, Michael S. Tsirkin
In-Reply-To: <20120907002533.GB16360@google.com>
Kent Overstreet <koverstreet@google.com> writes:
> On Fri, Sep 07, 2012 at 09:10:25AM +0930, Rusty Russell wrote:
>> Kent Overstreet <koverstreet@google.com> writes:
>>
>> > On Thu, Sep 06, 2012 at 12:49:56PM +0300, Michael S. Tsirkin wrote:
>> >> On Thu, Sep 06, 2012 at 02:25:12AM -0700, Kent Overstreet wrote:
>> >> > Do you not understand the difference between depends an selects?
>> >> > Or did you not read my original mail?
>>
>> Now you're getting insulting.
>
> Yes, but at least I'm not being intentionally obtuse.
Insulting again. Wow.
It took me this long to understand your complaint. Perhaps I'm stupid.
Or perhaps you are terrible at explaining yourself, and it is only
through our patient and heroic efforts that we can comprehend you at
all?
> Think about it from the user's pov. They check what VIRTIO_BLK depends
> on - just VIRTIO.
>
> So they try to figure out how to flip on VIRTIO, or what VIRTIO even is.
>
> See how that last step might be problematic? CONFIG_VIRTIO is not
> exposed! It doesn't even seem to control anything!
>
> Go back to your example. Checking the dependencies for E1000 would tell
> you the user needs to flip on CONFIG_PCI. Done. Easy.
Actually, it depends on NET_VENDOR_INTEL which depends on CONFIG_PCI,
but yes, it's discoverable.
So your actual complaint is that:
1) CONFIG_VIRTIO is misleadingly documented both in comment and name.
2) It's not discoverable, since it's only selected via other things.
> And, if that is what you're doing with CONFIG_VIRTIO (I'm still not
> sure) the comment at the top of drivers/virtio/Kconfig is _wrong_:
As grep would show you, it's selected by LGUEST, S390_GUEST, RPMSG,
VIRTIO_PCI and VIRTIO_MMIO (VIRTIO_BALLOON is a cut & paste bug, already
patched by MST).
We could change every virtio device to depend on (CONFIG_LGUEST |
CONFIG_S390_GUEST | CONFIG_RPMSG | CONFIG_VIRTIO_PCI |
CONFIG_VIRTIO_MMIO), which is more discoverable but uglier. How's this
workaround?
From: Rusty Russell <rusty@rustcorp.com.au>
Subject: virtio: add help to CONFIG_VIRTIO option.
Trying to enable a virtio driver (eg CONFIG_VIRTIO_BLK) is painful
because it depends on CONFIG_VIRTIO. CONFIG_VIRTIO doesn't tell you
how to turn it on (it's selected from anything which provides a virtio
bus).
This patch at least adds some documentation, visible in menuconfig, as
a hint.
Reported-by: Kent Overstreet <koverstreet@google.com>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
--- a/drivers/virtio/Kconfig
+++ b/drivers/virtio/Kconfig
@@ -1,8 +1,10 @@
-# Virtio always gets selected by whoever wants it.
config VIRTIO
tristate
+ ---help---
+ This option is selected by any driver which implements the virtio
+ bus, such as CONFIG_VIRTIO_PCI, CONFIG_VIRTIO_MMIO, CONFIG_LGUEST,
+ CONFIG_RPMSG or CONFIG_S390_GUEST.
-# Similarly the virtio ring implementation.
config VIRTIO_RING
tristate
depends on VIRTIO
^ permalink raw reply
* Re: [PATCH] virtio-balloon spec: provide a version of the "silent deflate" feature that works
From: Michael S. Tsirkin @ 2012-09-07 5:42 UTC (permalink / raw)
To: Rusty Russell
Cc: fes, aarcange, riel, kvm, yvugenfi, linux-kernel, mikew, yinghan,
Paolo Bonzini, virtualization
In-Reply-To: <87wr06hg0l.fsf@rustcorp.com.au>
On Fri, Sep 07, 2012 at 09:15:46AM +0930, Rusty Russell wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> > Instead, VIRTIO_NET_F_HOST_MUST_SEND_GARP would be a "negative" feature:
> > if set, the host _may not_ rely on the guest to send a gARP. Similarly
> > if VIRTIO_BALLOON_F_MUST_TELL_HOST is set, the guest _may not_ use
> > ballooned pages directly.
> >
> > There are _no_ other negative features besides
> > VIRTIO_BALLOON_F_MUST_TELL_HOST in the spec, and for a good
> > reason---because they're broken.
> >
> > (Hmm, actually we have one, VIRTIO_BLK_F_RO. It is also a bit broken,
> > but it is not so important because it depends on user input more than
> > hypervisor version).
>
> Yes, this is the key observation, and an important lesson for the
> future. Thanks!
> Note that these two negative features were in the original spec, where
> it's assumed that every device supports them. That's not explicitly
> documented, however.
I'm curious what would we do for the future? I tried to imagine that _RO
was not in the original spec, so virtio-blk expects a r/w device.
Now we can not add _RW - old hypervisors do not set it, and old
drivers do not ack it.
What would a new flag with equivalent functionality be?
> I like killing the totally unused feature.
>
> Cheers,
> Rusty.
I tried verifying that it is unused. And found this:
Linux drivers currently try to ack this feature if it is there,
so do BSD drivers. Both from v1. And they tell host before leak.
So that is fine.
But looking at windows drivers:
https://github.com/YanVugenfirer/kvm-guest-drivers-windows/blob/master/Balloon/sys/balloon.c
they do *not* ack this bit, and BalloonLeak calls TellHost at the last
line.
So it looks like a bug: we should teach driver to tell host first on leak?
Yan, Vadim, can you comment please?
Also if true, looks like this bit will be useful to detect a fixed driver on
the hypervisor side - to avoid unmapping such pages? Rusty what do you
think?
--
MST
^ permalink raw reply
* Re: [RFC-v3 0/5] vhost-scsi: Add support for host virtualized target
From: Michael S. Tsirkin @ 2012-09-07 6:13 UTC (permalink / raw)
To: Nicholas A. Bellinger
Cc: Stefan Hajnoczi, kvm-devel, Jan Kiszka, qemu-devel, Zhi Yong Wu,
Anthony Liguori, target-devel, Paolo Bonzini, lf-virt,
Christoph Hellwig
In-Reply-To: <1345582331-22593-1-git-send-email-nab@linux-iscsi.org>
On Tue, Aug 21, 2012 at 08:52:06PM +0000, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
>
> Hi folks,
>
> This is the third RFC for vhost-scsi patches against mainline QEMU v1.1
> to support the upstream tcm_vhost host virtualized target driver now
> available in v3.6-rc kernel code. This series is based upon last week's
> commit 346fe0c4c0b, and is aiming for a future QEMU v1.3 merge.
>
> The patch series is available directly from:
>
> git://git.kernel.org/pub/scm/virt/kvm/nab/qemu-kvm.git vhost-scsi-merge-v3
>
> This -v3 series contains further review changes based upon feedback from
> MST, Paolo, and Blue. It also contains the changes to function against
> the changes in target-pending/master -> headed for v3.6-rc3 code.
ACK series. Paolo can you ack virtio-scsi changes please?
Nicholas, do you want me to take this through my tree?
> Changes from v3 -> v2:
>
> - Move qdev_prop_vhost_scsi + DEFINE_PROP_VHOST_SCSI defs into vhost-scsi.[c,h]
> (reported by MST)
> - Add enum vhost_scsi_vq_list for VHostSCSI->vqs[] enumeration (reported by MST)
> - Add missing braces around single like if statement to following QEMU
> style (reported by Blue Swirl)
> - Change vhost_scsi_target->vhost_wwpn to char *, in order to drop casts to
> pstrcpy in vhost_scsi_start() + vhost_scsi_stop() (reported by Blue Swirl)
> - Change VHOST_SCSI_GET_ABI_VERSION to 'int' type (MST)
> - Add vhost-scsi.h include for DEFINE_PROP_VHOST_SCSI (mst + nab)
> - Move vhost-scsi related struct members ahead of *cmd_vqs[0] within
> VirtIOSCSI definition. (paolo + nab)
> - Fix 4 byte alignment of vhost_scsi_target (MST)
> - Convert fprintf(stderr, ...) usage to -> error_report() (reported by MST)
> - Do explict memset of backend before calling VHOST_SCSI_CLEAR_ENDPOINT
> in vhost_scsi_stop() (reported by MST)
> - Add support for vhostfd passing in vhost_scsi_add() (reported by MST)
> - Move net_handle_fd_param() -> monitor_handle_fd_param() for generic
> usage by net/ + vhost-scsi (reported by MST)
> - Change vhost_scsi_add() to use monitor_handle_fd_param() (reported by MST)
>
> Changes from v1 -> v2:
>
> - Expose ABI version via VHOST_SCSI_GET_ABI_VERSION + use Rev 0 as
> starting point for v3.6-rc code (Stefan + ALiguori + nab)
> - Fix upstream qemu conflict in hw/qdev-properties.c
> - Make GET_ABI_VERSION use int (nab + mst)
> - Drop unnecessary event-notifier changes (nab)
> - Fix vhost-scsi case lables in configure (reported by paolo)
> - Convert qdev_prop_vhost_scsi to use ->get() + ->set() following
> qdev_prop_netdev (reported by paolo)
> - Fix typo in qemu-options.hx definition of vhost-scsi (reported by paolo)
> - Squash virtio-scsi: use the vhost-scsi host device from stefan (nab)
> - Fix up virtio_scsi_properties[] conflict w/ upstream qemu (nab)
> - Drop usage of to_virtio_scsi() in virtio_scsi_set_status()
> (reported by paolo)
> - Use modern VirtIOSCSIConf define in virtio-scsi.h (reported by paolo)
> - Use s->conf->vhost_scsi instead of proxyconf->vhost_scsi in
> virtio_scsi_init() (reported by paolo)
> - Only register QEMU SCSI bus is vhost-scsi is not active (reported by paolo)
> - Fix incorrect VirtIOSCSI->cmd_vqs[0] definition (nab)
>
> Thanks again to everyone who has been reviewing this series!
>
> --nab
>
> Nicholas Bellinger (2):
> monitor: Rename+move net_handle_fd_param -> monitor_handle_fd_param
> virtio-scsi: Set max_target=0 during vhost-scsi operation
>
> Stefan Hajnoczi (3):
> vhost: Pass device path to vhost_dev_init()
> vhost-scsi: add -vhost-scsi host device for use with tcm-vhost
> virtio-scsi: Add start/stop functionality for vhost-scsi
>
> configure | 10 +++
> hw/Makefile.objs | 1 +
> hw/qdev-properties.c | 41 +++++++++++
> hw/vhost-scsi.c | 190 ++++++++++++++++++++++++++++++++++++++++++++++++++
> hw/vhost-scsi.h | 62 ++++++++++++++++
> hw/vhost.c | 5 +-
> hw/vhost.h | 3 +-
> hw/vhost_net.c | 2 +-
> hw/virtio-pci.c | 2 +
> hw/virtio-scsi.c | 55 ++++++++++++++-
> hw/virtio-scsi.h | 1 +
> monitor.c | 18 +++++
> monitor.h | 1 +
> net.c | 18 -----
> net.h | 2 -
> net/socket.c | 2 +-
> net/tap.c | 4 +-
> qemu-common.h | 1 +
> qemu-config.c | 19 +++++
> qemu-options.hx | 4 +
> vl.c | 18 +++++
> 21 files changed, 431 insertions(+), 28 deletions(-)
> create mode 100644 hw/vhost-scsi.c
> create mode 100644 hw/vhost-scsi.h
>
> --
> 1.7.2.5
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox