* Re: [Xen-devel] [patch 3/3] xen/privcmd: remove const modifier from declaration
From: Jan Beulich @ 2012-09-10 9:03 UTC (permalink / raw)
To: Andres Lagar-Cavilla, Dan Carpenter
Cc: Jeremy Fitzhardinge, xen-devel, kernel-janitors,
Konrad Rzeszutek Wilk, virtualization
In-Reply-To: <20120908095808.GC608@elgon.mountain>
>>> On 08.09.12 at 11:58, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> When we use this pointer, we cast away the const modifier and modify the
> data. I think it was an accident to declare it as const.
NAK - the const is very valid here, as the v2 interface (as opposed
to the v1 one) does _not_ modify this array (or if it does, it's a
bug). This is a guarantee made to user mode, so it should also be
expressed that way in the interface.
But of course the cast used before this patch isn't right either, as
it indeed inappropriately discards the qualifier. Afaiu this was done
to simplify the internal workings of the code, but I don't think it's
desirable to sacrifice type safety for implementation simplicity.
Jan
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> diff --git a/include/xen/privcmd.h b/include/xen/privcmd.h
> index a853168..58ed953 100644
> --- a/include/xen/privcmd.h
> +++ b/include/xen/privcmd.h
> @@ -69,7 +69,7 @@ struct privcmd_mmapbatch_v2 {
> unsigned int num; /* number of pages to populate */
> domid_t dom; /* target domain */
> __u64 addr; /* virtual address */
> - const xen_pfn_t __user *arr; /* array of mfns */
> + xen_pfn_t __user *arr; /* array of mfns */
> int __user *err; /* array of error codes */
> };
>
> diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
> index 0ce006a..fceb83e 100644
> --- a/drivers/xen/privcmd.c
> +++ b/drivers/xen/privcmd.c
> @@ -389,7 +389,7 @@ static long privcmd_ioctl_mmap_batch(void __user *udata,
> int version)
>
> if (state.global_error && (version == 1)) {
> /* Write back errors in second pass. */
> - state.user_mfn = (xen_pfn_t *)m.arr;
> + state.user_mfn = m.arr;
> state.err = err_array;
> ret = traverse_pages(m.num, sizeof(xen_pfn_t),
> &pagelist, mmap_return_errors_v1, &state);
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
^ permalink raw reply
* Re: [RFCv2 1/2] virtio_console: Add support for DMA memory allocation
From: Rusty Russell @ 2012-09-10 7:53 UTC (permalink / raw)
To: Michael S . Tsirkin
Cc: Sjur Brændeland, Linus Walleij, linux-kernel, virtualization,
Amit Shah, Sjur Brændeland
In-Reply-To: <1347001066-13521-1-git-send-email-sjur.brandeland@stericsson.com>
sjur.brandeland@stericsson.com writes:
> From: Sjur Brændeland <sjur.brandeland@stericsson.com>
>
> Add feature VIRTIO_CONSOLE_F_DMA_MEM. If the architecture has
> DMA support and this feature bit is set, the virtio data buffers
> will be allocated from DMA memory. If the device requests
> the feature VIRTIO_CONSOLE_F_DMA_MEM, but the architecture
> don't support DMA the driver's probe function will fail.
>
> This is needed for using virtio_console from the remoteproc
> framework.
Sorry for the back and forth, I've been pondering MST's points.
If we make a new dma-multiport device (eg. ID 11), how ugly is the code?
It would be a virtio console with DMA buffers and no console, just the
multiport stuff. This would have no impact on the current spec for
virtio console.
Thanks,
Rusty.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH] virtio-balloon spec: provide a version of the "silent deflate" feature that works
From: Paolo Bonzini @ 2012-09-10 6:55 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: fes, aarcange, riel, kvm, yvugenfi, linux-kernel, mikew, yinghan,
virtualization
In-Reply-To: <20120910064705.GB17949@redhat.com>
Il 10/09/2012 08:47, Michael S. Tsirkin ha scritto:
> On Mon, Sep 10, 2012 at 08:38:09AM +0200, Paolo Bonzini wrote:
>> Il 10/09/2012 08:03, Michael S. Tsirkin ha scritto:
>>> On Mon, Sep 10, 2012 at 07:50:13AM +0200, Paolo Bonzini wrote:
>>>> Il 09/09/2012 00:22, Michael S. Tsirkin ha scritto:
>>>>>> Almost. One is "the guest, if really needed, can tell the host of
>>>>>> pages". If not negotiated, and the host does not support it, the host
>>>>>> must break the guest (e.g. fail to offer any virtqueues).
>>>>>
>>>>> There is no way in spec to break the guest.
>>>>> You can not fail to offer virtqueues.
>>>>
>>>> You can always return 0 for the first queue.
>>>
>>> I don't think guest drivers recover gracefully from this.
>>> Do they?
>>
>> No, that's the point ("break the guest" is really "break the driver").
>
> You can just stop VM then. No need for a side channel.
Keeping the VM running, just with no balloon driver is preferrable.
>>>>>> The other is "the guest, though, would prefer not to do so". It is
>>>>>> different because the guest can proceed in a fallback mode even if the
>>>>>> host doesn't offer it.
>>>>>
>>>>> I think I get what your proposed SILENT means what I do not get
>>>>> is the motivation. It looks like a premature optimization to me.
>>>>
>>>> The motivation is to let the driver choose between two behaviors: the
>>>> current one where ballooning is only done on request, and a more
>>>> aggressive one.
>>>
>>> Yes but why is being silent any good? Optimization?
>>> Any data to show that it will help some workload?
>>
>> Idle guests can move cache pages to the balloon. You can overcommit
>> more aggressively, because the host can madvise away a lot more memory.
>
> IMHO this feature needs more thought. E.g. how will this work with assignment?
Revert to normal cooperative ballooning.
> If we build something let's build it in a way that plays nicely
> with other features.
Yes, that's the point of SILENT. :)
Paolo
^ permalink raw reply
* Re: [PATCH 4/5] virtio-scsi: Add start/stop functionality for vhost-scsi
From: Michael S. Tsirkin @ 2012-09-10 6:49 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Stefan Hajnoczi, kvm-devel, Jan Kiszka, qemu-devel, lf-virt,
Anthony Liguori, target-devel, Zhi Yong Wu, Christoph Hellwig
In-Reply-To: <504D8BC7.8070900@redhat.com>
On Mon, Sep 10, 2012 at 08:42:15AM +0200, Paolo Bonzini wrote:
> Il 10/09/2012 08:24, Michael S. Tsirkin ha scritto:
> >> > I chose the backend name because, ideally, there would be no other
> >> > difference. QEMU _could_ implement all the goodies in vhost-scsi (such
> >> > as reservations or ALUA), it just doesn't do that yet.
> >> >
> >> > Paolo
> > Then why do you say "It is used completely differently from
> > virtio-scsi-pci"?
>
> It is configured differently (and I haven't seen a proposal yet for how
> to bridge the two), it does not interoperate, it has right now a
> different set of features.
>
> The "does not interoperate" bit is particularly important. Say QEMU
> were to implement persistent reservations (right now only a vhost-scsi
> feature). Then QEMU and vhost-scsi PR would not be interchangeable, a
> reservation made by QEMU would not be visible in vhost and vice versa.
So this is backend stuff, right?
> > Isn't it just a different backend?
> >
> > If yes then it should be a backend option, like it is
> > for virtio-net.
>
> You mean a -drive option?
Yes.
> That would mean adding the logic to configure
> vhost-scsi to the QEMU block layer, that's a completely different project...
>
> Paolo
This is an implementation detail. You can make it -drive option
but still have all the actual logic outside block layer.
All you need in block is option parsing code.
Please take a look at how -net does this:
we did *not* add all logic to qemu net layer.
^ permalink raw reply
* Re: [PATCH] virtio-balloon spec: provide a version of the "silent deflate" feature that works
From: Michael S. Tsirkin @ 2012-09-10 6:47 UTC (permalink / raw)
To: Paolo Bonzini
Cc: fes, aarcange, riel, kvm, yvugenfi, linux-kernel, mikew, yinghan,
virtualization
In-Reply-To: <504D8AD1.6050006@redhat.com>
On Mon, Sep 10, 2012 at 08:38:09AM +0200, Paolo Bonzini wrote:
> Il 10/09/2012 08:03, Michael S. Tsirkin ha scritto:
> > On Mon, Sep 10, 2012 at 07:50:13AM +0200, Paolo Bonzini wrote:
> >> Il 09/09/2012 00:22, Michael S. Tsirkin ha scritto:
> >>>> Almost. One is "the guest, if really needed, can tell the host of
> >>>> pages". If not negotiated, and the host does not support it, the host
> >>>> must break the guest (e.g. fail to offer any virtqueues).
> >>>
> >>> There is no way in spec to break the guest.
> >>> You can not fail to offer virtqueues.
> >>
> >> You can always return 0 for the first queue.
> >
> > I don't think guest drivers recover gracefully from this.
> > Do they?
>
> No, that's the point ("break the guest" is really "break the driver").
You can just stop VM then. No need for a side channel.
...
> >>>> The other is "the guest, though, would prefer not to do so". It is
> >>>> different because the guest can proceed in a fallback mode even if the
> >>>> host doesn't offer it.
> >>>
> >>> I think I get what your proposed SILENT means what I do not get
> >>> is the motivation. It looks like a premature optimization to me.
> >>
> >> The motivation is to let the driver choose between two behaviors: the
> >> current one where ballooning is only done on request, and a more
> >> aggressive one.
> >
> > Yes but why is being silent any good? Optimization?
> > Any data to show that it will help some workload?
>
> Idle guests can move cache pages to the balloon. You can overcommit
> more aggressively, because the host can madvise away a lot more memory.
IMHO this feature needs more thought. E.g. how will this work with assignment?
If we build something let's build it in a way that plays nicely
with other features.
> >>> OK so TELL says *when* to notify host, SILENT if set allows guest
> >>> to skip leak notifications? In this case TELL should just be ignored
> >>> when SILENT is set.
> >>
> >> Yeah, that was my first idea. However, there are existing drivers that
> >> ignore SILENT, so that would not be 100% exact.
> >
> > Not sure I follow the logic.
> > They don't ack SILENT so that would be 100% exact.
>
> Hmm, then I'm not sure I follow yours. We agreed that delaying
> notifications or skipping them is really the same thing, right?
>
> I think we're just stuck in a linguistic problem, with "must not" being
> wrong and "does not have to" being too verbose. Calling it
> VIRTIO_BALLOON_F_SILENT_DEFLATE was a workaround for this, but perhaps
> it adds more confusion.
>
> Paolo
Looks like it does :)
^ permalink raw reply
* Re: [PATCH 4/5] virtio-scsi: Add start/stop functionality for vhost-scsi
From: Paolo Bonzini @ 2012-09-10 6:42 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Stefan Hajnoczi, kvm-devel, Jan Kiszka, qemu-devel, lf-virt,
Anthony Liguori, target-devel, Zhi Yong Wu, Christoph Hellwig
In-Reply-To: <20120910062437.GD16819@redhat.com>
Il 10/09/2012 08:24, Michael S. Tsirkin ha scritto:
>> > I chose the backend name because, ideally, there would be no other
>> > difference. QEMU _could_ implement all the goodies in vhost-scsi (such
>> > as reservations or ALUA), it just doesn't do that yet.
>> >
>> > Paolo
> Then why do you say "It is used completely differently from
> virtio-scsi-pci"?
It is configured differently (and I haven't seen a proposal yet for how
to bridge the two), it does not interoperate, it has right now a
different set of features.
The "does not interoperate" bit is particularly important. Say QEMU
were to implement persistent reservations (right now only a vhost-scsi
feature). Then QEMU and vhost-scsi PR would not be interchangeable, a
reservation made by QEMU would not be visible in vhost and vice versa.
> Isn't it just a different backend?
>
> If yes then it should be a backend option, like it is
> for virtio-net.
You mean a -drive option? That would mean adding the logic to configure
vhost-scsi to the QEMU block layer, that's a completely different project...
Paolo
^ permalink raw reply
* Re: [PATCH] virtio-balloon spec: provide a version of the "silent deflate" feature that works
From: Paolo Bonzini @ 2012-09-10 6:38 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: fes, aarcange, riel, kvm, yvugenfi, linux-kernel, mikew, yinghan,
virtualization
In-Reply-To: <20120910060359.GB16819@redhat.com>
Il 10/09/2012 08:03, Michael S. Tsirkin ha scritto:
> On Mon, Sep 10, 2012 at 07:50:13AM +0200, Paolo Bonzini wrote:
>> Il 09/09/2012 00:22, Michael S. Tsirkin ha scritto:
>>>> Almost. One is "the guest, if really needed, can tell the host of
>>>> pages". If not negotiated, and the host does not support it, the host
>>>> must break the guest (e.g. fail to offer any virtqueues).
>>>
>>> There is no way in spec to break the guest.
>>> You can not fail to offer virtqueues.
>>
>> You can always return 0 for the first queue.
>
> I don't think guest drivers recover gracefully from this.
> Do they?
No, that's the point ("break the guest" is really "break the driver").
>>> Besides, there is no guarantee that virtqueue setup
>>> happens after feature negotiation.
>>
>> It is the only way that makes sense though (unless the guest would write
>> 0 for its features). Should we change that?
>
> I do think it would be nice to add a generic way for device to
> notify guest about an internal failure.
> This can only happen after DRIVER_OK status is written though,
> and since existing drivers do not expect such failure, it might
> be too late.
Agreed.
>>>> The other is "the guest, though, would prefer not to do so". It is
>>>> different because the guest can proceed in a fallback mode even if the
>>>> host doesn't offer it.
>>>
>>> I think I get what your proposed SILENT means what I do not get
>>> is the motivation. It looks like a premature optimization to me.
>>
>> The motivation is to let the driver choose between two behaviors: the
>> current one where ballooning is only done on request, and a more
>> aggressive one.
>
> Yes but why is being silent any good? Optimization?
> Any data to show that it will help some workload?
Idle guests can move cache pages to the balloon. You can overcommit
more aggressively, because the host can madvise away a lot more memory.
>>> OK so TELL says *when* to notify host, SILENT if set allows guest
>>> to skip leak notifications? In this case TELL should just be ignored
>>> when SILENT is set.
>>
>> Yeah, that was my first idea. However, there are existing drivers that
>> ignore SILENT, so that would not be 100% exact.
>
> Not sure I follow the logic.
> They don't ack SILENT so that would be 100% exact.
Hmm, then I'm not sure I follow yours. We agreed that delaying
notifications or skipping them is really the same thing, right?
I think we're just stuck in a linguistic problem, with "must not" being
wrong and "does not have to" being too verbose. Calling it
VIRTIO_BALLOON_F_SILENT_DEFLATE was a workaround for this, but perhaps
it adds more confusion.
Paolo
^ permalink raw reply
* Re: [PATCHv4] virtio-spec: virtio network device multiqueue support
From: Michael S. Tsirkin @ 2012-09-10 6:33 UTC (permalink / raw)
To: Rusty Russell
Cc: kvm, netdev, rick.jones2, virtualization, levinsasha928, pbonzini,
Tom Herbert
In-Reply-To: <20120910062738.GA17331@redhat.com>
On Mon, Sep 10, 2012 at 09:27:38AM +0300, Michael S. Tsirkin wrote:
> On Mon, Sep 10, 2012 at 09:16:29AM +0300, Michael S. Tsirkin wrote:
> > On Mon, Sep 10, 2012 at 11:42:25AM +0930, Rusty Russell wrote:
> > > OK, I read the spec (pasted below for easy of reading), but I'm still
> > > confused over how this will work.
> > >
> > > I thought normal net drivers have the hardware provide an rxhash for
> > > each packet, and we map that to CPU to queue the packet on[1]. We hope
> > > that the receiving process migrates to that CPU, so xmit queue
> > > matches.
> >
> > This ony works sometimes. For example it's common to pin netperf to a
> > cpu to get consistent performance. Proper hardware must obey what
> > applications want it to do, not the other way around.
> >
> > > For virtio this would mean a new per-packet rxhash value, right?
> > >
> > > Why are we doing something different? What am I missing?
> > >
> > > Thanks,
> > > Rusty.
> > > [1] Everything I Know About Networking I Learned From LWN:
> > > https://lwn.net/Articles/362339/
> >
> > I think you missed this:
> >
> > Some network interfaces can help with the distribution of incoming
> > packets; they have multiple receive queues and multiple interrupt lines.
> > Others, though, are equipped with a single queue, meaning that the
> > driver for that hardware must deal with all incoming packets in a
> > single, serialized stream. Parallelizing such a stream requires some
> > intelligence on the part of the host operating system.
> >
> > In other words RPS is a hack to speed up networking on cheapo
> > hardware, this is one of the reasons it is off by default.
> > Good hardware has multiple receive queues.
> > We can implement a good one so we do not need RPS.
> >
> > Also not all guest OS-es support RPS.
> >
> > Does this clarify?
>
> I would like to add that on many processors, sending
> IPCs between guest CPUs requires exits on sending *and*
> receiving path, making it very expensive.
A final addition: what you suggest above would be
"TX follows RX", right?
It is in anticipation of something like that, that I made
steering programming so generic.
I think TX follows RX is more immediately useful for reasons above
but we can add both to spec and let drivers and devices
decide what they want to support.
Pls let me know.
> > > ---
> > > Transmit Packet Steering
> > >
> > > 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 ru
le is not followed.
> > >
> > > 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).
> > >
> > > This command accepts a single out argument in the following format:
> > >
> > > #define VIRTIO_NET_CTRL_STEERING 4
> > >
> > > The field rule specifies the function used to select transmit virtqueue for a given packet; the field param makes it possible to pass an extra parameter if appropriate. When rule 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 rule is set to VIRTIO_NET_CTRL_STEERING_RX_FOLLOWS_TX packets are steered by driver to the first N=(param+1) multiqueue virtqueues transmitq1...transmitqN; the default transmitq is unused. Driver must have configured all these (param+1) virtqueues beforehand.
> > >
> > > 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 is an 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.
> > >
> > > 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.
> > >
> > > For debugging, the current steering rule can also be read from the configuration space.
> > >
> > > Receive Packet Steering
> > >
> > > 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[sub:Transmit-Packet-Steering].
> > >
> > > The field rule specifies the function used to select receive virtqueue for a given packet; the field param makes it possible to pass an extra parameter if appropriate. When rule is set to VIRTIO_NET_CTRL_STEERING_SINGLE all packets are steered to the default virtqueue receiveq (0); param is unused; this is the default. When rule is set to VIRTIO_NET_CTRL_STEERING_RX_FOLLOWS_TX packets are steered by host to the first N=(param+1) multiqueue virtqueues receiveq1...receiveqN; the default receiveq is unused. Driver must have configured all these (param+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 fl
ows, or when this mapping information is missing, a device-specific steering function is used.
> > >
> > > Supported steering rules can be added and removed in the future. Driver should probe for supported rules by checking ack values of the command.
> > >
> > > 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.
^ permalink raw reply
* Re: [PATCHv4] virtio-spec: virtio network device multiqueue support
From: Michael S. Tsirkin @ 2012-09-10 6:27 UTC (permalink / raw)
To: Rusty Russell
Cc: kvm, netdev, rick.jones2, virtualization, levinsasha928, pbonzini,
Tom Herbert
In-Reply-To: <20120910061629.GC16819@redhat.com>
On Mon, Sep 10, 2012 at 09:16:29AM +0300, Michael S. Tsirkin wrote:
> On Mon, Sep 10, 2012 at 11:42:25AM +0930, Rusty Russell wrote:
> > OK, I read the spec (pasted below for easy of reading), but I'm still
> > confused over how this will work.
> >
> > I thought normal net drivers have the hardware provide an rxhash for
> > each packet, and we map that to CPU to queue the packet on[1]. We hope
> > that the receiving process migrates to that CPU, so xmit queue
> > matches.
>
> This ony works sometimes. For example it's common to pin netperf to a
> cpu to get consistent performance. Proper hardware must obey what
> applications want it to do, not the other way around.
>
> > For virtio this would mean a new per-packet rxhash value, right?
> >
> > Why are we doing something different? What am I missing?
> >
> > Thanks,
> > Rusty.
> > [1] Everything I Know About Networking I Learned From LWN:
> > https://lwn.net/Articles/362339/
>
> I think you missed this:
>
> Some network interfaces can help with the distribution of incoming
> packets; they have multiple receive queues and multiple interrupt lines.
> Others, though, are equipped with a single queue, meaning that the
> driver for that hardware must deal with all incoming packets in a
> single, serialized stream. Parallelizing such a stream requires some
> intelligence on the part of the host operating system.
>
> In other words RPS is a hack to speed up networking on cheapo
> hardware, this is one of the reasons it is off by default.
> Good hardware has multiple receive queues.
> We can implement a good one so we do not need RPS.
>
> Also not all guest OS-es support RPS.
>
> Does this clarify?
I would like to add that on many processors, sending
IPCs between guest CPUs requires exits on sending *and*
receiving path, making it very expensive.
> > ---
> > Transmit Packet Steering
> >
> > 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.
> >
> > 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).
> >
> > This command accepts a single out argument in the following format:
> >
> > #define VIRTIO_NET_CTRL_STEERING 4
> >
> > The field rule specifies the function used to select transmit virtqueue for a given packet; the field param makes it possible to pass an extra parameter if appropriate. When rule 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 rule is set to VIRTIO_NET_CTRL_STEERING_RX_FOLLOWS_TX packets are steered by driver to the first N=(param+1) multiqueue virtqueues transmitq1...transmitqN; the default transmitq is unused. Driver must have configured all these (param+1) virtqueues beforehand.
> >
> > 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 is an 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.
> >
> > 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.
> >
> > For debugging, the current steering rule can also be read from the configuration space.
> >
> > Receive Packet Steering
> >
> > 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[sub:Transmit-Packet-Steering].
> >
> > The field rule specifies the function used to select receive virtqueue for a given packet; the field param makes it possible to pass an extra parameter if appropriate. When rule is set to VIRTIO_NET_CTRL_STEERING_SINGLE all packets are steered to the default virtqueue receiveq (0); param is unused; this is the default. When rule is set to VIRTIO_NET_CTRL_STEERING_RX_FOLLOWS_TX packets are steered by host to the first N=(param+1) multiqueue virtqueues receiveq1...receiveqN; the default receiveq is unused. Driver must have configured all these (param+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 flow
s, or when this mapping information is missing, a device-specific steering function is used.
> >
> > Supported steering rules can be added and removed in the future. Driver should probe for supported rules by checking ack values of the command.
> >
> > 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.
^ permalink raw reply
* Re: [PATCH 4/5] virtio-scsi: Add start/stop functionality for vhost-scsi
From: Michael S. Tsirkin @ 2012-09-10 6:24 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Stefan Hajnoczi, kvm-devel, Jan Kiszka, qemu-devel, lf-virt,
Anthony Liguori, target-devel, Zhi Yong Wu, Christoph Hellwig
In-Reply-To: <504D85D6.2090703@redhat.com>
On Mon, Sep 10, 2012 at 08:16:54AM +0200, Paolo Bonzini wrote:
> Il 09/09/2012 00:40, Michael S. Tsirkin ha scritto:
> > On Fri, Sep 07, 2012 at 06:00:50PM +0200, Paolo Bonzini wrote:
> >> Il 07/09/2012 08:48, Nicholas A. Bellinger ha scritto:
> >>> Cc: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
> >>> Cc: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
> >>> Cc: Michael S. Tsirkin <mst@redhat.com>
> >>> Cc: Paolo Bonzini <pbonzini@redhat.com>
> >>> Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> >>> ---
> >>> hw/virtio-pci.c | 2 ++
> >>> hw/virtio-scsi.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
> >>> hw/virtio-scsi.h | 1 +
> >>> 3 files changed, 52 insertions(+), 0 deletions(-)
> >>
> >> Please create a completely separate device vhost-scsi-pci instead (or
> >> virtio-scsi-tcm-pci, or something like that). It is used completely
> >> differently from virtio-scsi-pci, it does not make sense to conflate the
> >> two.
> >
> > Ideally the name would say how it is different, not what backend it
> > uses. Any good suggestions?
>
> I chose the backend name because, ideally, there would be no other
> difference. QEMU _could_ implement all the goodies in vhost-scsi (such
> as reservations or ALUA), it just doesn't do that yet.
>
> Paolo
Then why do you say "It is used completely differently from
virtio-scsi-pci"? Isn't it just a different backend?
If yes then it should be a backend option, like it is
for virtio-net.
--
MST
^ permalink raw reply
* Re: [PATCH 4/5] virtio-scsi: Add start/stop functionality for vhost-scsi
From: Paolo Bonzini @ 2012-09-10 6:16 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Stefan Hajnoczi, kvm-devel, Jan Kiszka, qemu-devel, lf-virt,
Anthony Liguori, target-devel, Zhi Yong Wu, Christoph Hellwig
In-Reply-To: <20120908224032.GC20588@redhat.com>
Il 09/09/2012 00:40, Michael S. Tsirkin ha scritto:
> On Fri, Sep 07, 2012 at 06:00:50PM +0200, Paolo Bonzini wrote:
>> Il 07/09/2012 08:48, Nicholas A. Bellinger ha scritto:
>>> Cc: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
>>> Cc: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
>>> Cc: Michael S. Tsirkin <mst@redhat.com>
>>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>>> Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
>>> ---
>>> hw/virtio-pci.c | 2 ++
>>> hw/virtio-scsi.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
>>> hw/virtio-scsi.h | 1 +
>>> 3 files changed, 52 insertions(+), 0 deletions(-)
>>
>> Please create a completely separate device vhost-scsi-pci instead (or
>> virtio-scsi-tcm-pci, or something like that). It is used completely
>> differently from virtio-scsi-pci, it does not make sense to conflate the
>> two.
>
> Ideally the name would say how it is different, not what backend it
> uses. Any good suggestions?
I chose the backend name because, ideally, there would be no other
difference. QEMU _could_ implement all the goodies in vhost-scsi (such
as reservations or ALUA), it just doesn't do that yet.
Paolo
^ permalink raw reply
* Re: [PATCHv4] virtio-spec: virtio network device multiqueue support
From: Michael S. Tsirkin @ 2012-09-10 6:16 UTC (permalink / raw)
To: Rusty Russell
Cc: kvm, netdev, rick.jones2, virtualization, levinsasha928, pbonzini,
Tom Herbert
In-Reply-To: <878vcifwxi.fsf@rustcorp.com.au>
On Mon, Sep 10, 2012 at 11:42:25AM +0930, Rusty Russell wrote:
> OK, I read the spec (pasted below for easy of reading), but I'm still
> confused over how this will work.
>
> I thought normal net drivers have the hardware provide an rxhash for
> each packet, and we map that to CPU to queue the packet on[1]. We hope
> that the receiving process migrates to that CPU, so xmit queue
> matches.
This ony works sometimes. For example it's common to pin netperf to a
cpu to get consistent performance. Proper hardware must obey what
applications want it to do, not the other way around.
> For virtio this would mean a new per-packet rxhash value, right?
>
> Why are we doing something different? What am I missing?
>
> Thanks,
> Rusty.
> [1] Everything I Know About Networking I Learned From LWN:
> https://lwn.net/Articles/362339/
I think you missed this:
Some network interfaces can help with the distribution of incoming
packets; they have multiple receive queues and multiple interrupt lines.
Others, though, are equipped with a single queue, meaning that the
driver for that hardware must deal with all incoming packets in a
single, serialized stream. Parallelizing such a stream requires some
intelligence on the part of the host operating system.
In other words RPS is a hack to speed up networking on cheapo
hardware, this is one of the reasons it is off by default.
Good hardware has multiple receive queues.
We can implement a good one so we do not need RPS.
Also not all guest OS-es support RPS.
Does this clarify?
> ---
> Transmit Packet Steering
>
> 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 i
s not followed.
>
> 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).
>
> This command accepts a single out argument in the following format:
>
> #define VIRTIO_NET_CTRL_STEERING 4
>
> The field rule specifies the function used to select transmit virtqueue for a given packet; the field param makes it possible to pass an extra parameter if appropriate. When rule 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 rule is set to VIRTIO_NET_CTRL_STEERING_RX_FOLLOWS_TX packets are steered by driver to the first N=(param+1) multiqueue virtqueues transmitq1...transmitqN; the default transmitq is unused. Driver must have configured all these (param+1) virtqueues beforehand.
>
> 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 is an 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.
>
> 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.
>
> For debugging, the current steering rule can also be read from the configuration space.
>
> Receive Packet Steering
>
> 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[sub:Transmit-Packet-Steering].
>
> The field rule specifies the function used to select receive virtqueue for a given packet; the field param makes it possible to pass an extra parameter if appropriate. When rule is set to VIRTIO_NET_CTRL_STEERING_SINGLE all packets are steered to the default virtqueue receiveq (0); param is unused; this is the default. When rule is set to VIRTIO_NET_CTRL_STEERING_RX_FOLLOWS_TX packets are steered by host to the first N=(param+1) multiqueue virtqueues receiveq1...receiveqN; the default receiveq is unused. Driver must have configured all these (param+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.
>
> Supported steering rules can be added and removed in the future. Driver should probe for supported rules by checking ack values of the command.
>
> 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.
^ permalink raw reply
* Re: [PATCH] virtio-spec: balloon: MUST_TELL_HOST is optional
From: Paolo Bonzini @ 2012-09-10 6:15 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: virtualization
In-Reply-To: <20120909113650.GA2775@redhat.com>
Il 09/09/2012 13:36, Michael S. Tsirkin ha scritto:
> \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_MUST_TELL_HOST feature is
> +\change_inserted 1986246365 1347188540
> +negotiated
> +\change_deleted 1986246365 1347188542
> +set
> +\change_unchanged
> +, the guest may not use these requested pages until that descriptor in the
> + deflateq has been used by the device.
> \end_layout
Yeah, that's ok.
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-10 6:03 UTC (permalink / raw)
To: Paolo Bonzini
Cc: fes, aarcange, riel, kvm, yvugenfi, linux-kernel, mikew, yinghan,
virtualization
In-Reply-To: <504D7F95.9070700@redhat.com>
On Mon, Sep 10, 2012 at 07:50:13AM +0200, Paolo Bonzini wrote:
> Il 09/09/2012 00:22, Michael S. Tsirkin ha scritto:
> >> Almost. One is "the guest, if really needed, can tell the host of
> >> pages". If not negotiated, and the host does not support it, the host
> >> must break the guest (e.g. fail to offer any virtqueues).
> >
> > There is no way in spec to break the guest.
> > You can not fail to offer virtqueues.
>
> You can always return 0 for the first queue.
I don't think guest drivers recover gracefully from this.
Do they?
> > Besides, there is no guarantee that virtqueue setup
> > happens after feature negotiation.
>
> It is the only way that makes sense though (unless the guest would write
> 0 for its features).
> Should we change that?
Not sure. This was not always the case. Further
setup can fail with e.g ENOMEM and
driver could retry with a set of more conservative features.
I do think it would be nice to add a generic way for device to
notify guest about an internal failure.
This can only happen after DRIVER_OK status is written though,
and since existing drivers do not expect such failure, it might
be too late.
> >> The other is "the guest, though, would prefer not to do so". It is
> >> different because the guest can proceed in a fallback mode even if the
> >> host doesn't offer it.
> >
> > I think I get what your proposed SILENT means what I do not get
> > is the motivation. It looks like a premature optimization to me.
>
> The motivation is to let the driver choose between two behaviors: the
> current one where ballooning is only done on request, and a more
> aggressive one.
Yes but why is being silent any good? Optimization?
Any data to show that it will help some workload?
...
> > OK so TELL says *when* to notify host, SILENT if set allows guest
> > to skip leak notifications? In this case TELL should just be ignored
> > when SILENT is set.
>
> Yeah, that was my first idea. However, there are existing drivers that
> ignore SILENT, so that would not be 100% exact.
Not sure I follow the logic.
They don't ack SILENT so that would be 100% exact.
--
MST
^ permalink raw reply
* Re: [PATCH] virtio-balloon spec: provide a version of the "silent deflate" feature that works
From: Michael S. Tsirkin @ 2012-09-10 5:51 UTC (permalink / raw)
To: Rusty Russell
Cc: fes, aarcange, riel, kvm, yvugenfi, linux-kernel, mikew, yinghan,
Paolo Bonzini, virtualization
In-Reply-To: <87fw6qfya7.fsf@rustcorp.com.au>
On Mon, Sep 10, 2012 at 11:13:12AM +0930, Rusty Russell wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> > On Sat, Sep 08, 2012 at 02:36:00PM +0930, Rusty Russell wrote:
> >> "Michael S. Tsirkin" <mst@redhat.com> writes:
> >> > On Fri, Sep 07, 2012 at 04:09:50PM +0930, Rusty Russell wrote:
> >> >> > 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?
> >> >>
> >> >> So, feature is unimplemented in qemu, and broken in drivers. I starting
> >> >> to share Paolo's dislike of it.
> >> >
> >> > What is broken in drivers?
> >>
> >> Because supporting the feature is *not* optional for a driver.
> >>
> >> If the device said MUST_TELL_HOST, it meant that the driver *had* to
> >> tell the host before it touched the page, otherwise Bad Things might
> >> happen. It was in the original spec precisely to allow devices to
> >> actually *remove* pages.
> >>
> >> Noone ever noticed the windows driver didn't support it, because qemu
> >> never requires MUST_TELL_HOST.
> >>
> >> So in practice, it's now an optional feature. Since no device used it
> >> anyway, we're better off discarding it than trying to fix it.
> >
> > I trust you this was not the intent. But it seems to be
> > the intent in spec, because almost all features are optional.
> >
> > And so windows driver authors interpreted it
> > this way. And it is *useful* like this. See below.
>
> ...
>
> > Suggested use is for device assignment which needs all guest memory
> > locked. hypervisor can unlock pages in balloon but guest must wait for
> > hypervisor to lock them back before use.
> >
> > when a hypervisor implements this,
> > this will work with linux guests but not windows
> > guests and the existing bit fits exactly the purpose.
>
> If a hypervisor needs this, and the guest doesn't support it, then
> the hypervisor can only abandon the balloon device. That's not my
> definition of "optional".
>
> >> > Do we really know there are no hypervisors implementing it?
> >>
> >> As much as can be known. Qemu doesn't, lkvm doesn't.
> >
> > But we can add it in qemu and it will be a useful feature.
> >
> >> > As I said above drivers do have support.
> >>
> >> Not the windows drivers. So it's optional, thus removing it will likely
> >> harm noone.
> >>
> >> Cheers,
> >> Rusty.
> >
> > I think the issue is that kvm always locked all guest memory
> > for assignment. This restriction is removed
> > with vfio which has separate page tables.
> > Now that vfio is upstream and work on qemu integration
> > is being worked on, we might finally see people using this bit
> > to allow memory overcommit with device assignment.
>
> That was left-field.... so you're saying some guest might pull a page
> from the balloon and DMA to it, and the vfio device needs to know in
> advance that it's going to do it?
>
> But what will we do if the guest doesn't ack the bit?
> ie. I don't think it can really be optional.
>
> Cheers,
> Rusty.
We have several options:
1. No memory overcommit feature. This is always the case ATM!
2. Do not hot-plug assigned device.
3. Hot-unplug assigned device.
4. Some assigned devices can be able to handle memory errors
e.g. using ATS. Limit hotplug to these.
> I don't think it can really be optional.
It is optional *for the device*.
But I don't insist on my patch. I am merely saying that
1. The bit is useful for host to detect guests
which can't combine memory overcommit with device assignment,
and this set of guests is not empty.
2. The fact that this bit is not optional for drivers is not well documented.
The only hint seems the use of words "feature is set" as
opposed to "feature is negoticated" as with other features.
The spec intended
"feature is set in Device Features bits". Drivers interpreted this
as "feature is set in Guest Features bits".
Hard to blame them, let us give them time to address this.
--
MST
^ permalink raw reply
* Re: [PATCH] virtio-balloon spec: provide a version of the "silent deflate" feature that works
From: Paolo Bonzini @ 2012-09-10 5:51 UTC (permalink / raw)
To: Rusty Russell
Cc: fes, aarcange, riel, kvm, Michael S. Tsirkin, yvugenfi,
linux-kernel, mikew, yinghan, virtualization
In-Reply-To: <87fw6qfya7.fsf@rustcorp.com.au>
Il 10/09/2012 03:43, Rusty Russell ha scritto:
>> Now that vfio is upstream and work on qemu integration
>> is being worked on, we might finally see people using this bit
>> to allow memory overcommit with device assignment.
> That was left-field.... so you're saying some guest might pull a page
> from the balloon and DMA to it, and the vfio device needs to know in
> advance that it's going to do it?
Not the vfio device, but the page needs to be pinned with mlock so the
effect is the same.
> But what will we do if the guest doesn't ack the bit?
Just pin the whole memory.
Paolo
> ie. I don't think it can really be optional.
^ permalink raw reply
* Re: [PATCH] virtio-balloon spec: provide a version of the "silent deflate" feature that works
From: Paolo Bonzini @ 2012-09-10 5:50 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: fes, aarcange, riel, kvm, yvugenfi, linux-kernel, mikew, yinghan,
virtualization
In-Reply-To: <20120908222221.GA20588@redhat.com>
Il 09/09/2012 00:22, Michael S. Tsirkin ha scritto:
>> Almost. One is "the guest, if really needed, can tell the host of
>> pages". If not negotiated, and the host does not support it, the host
>> must break the guest (e.g. fail to offer any virtqueues).
>
> There is no way in spec to break the guest.
> You can not fail to offer virtqueues.
You can always return 0 for the first queue.
> Besides, there is no guarantee that virtqueue setup
> happens after feature negotiation.
It is the only way that makes sense though (unless the guest would write
0 for its features). Should we change that?
>> The other is "the guest, though, would prefer not to do so". It is
>> different because the guest can proceed in a fallback mode even if the
>> host doesn't offer it.
>
> I think I get what your proposed SILENT means what I do not get
> is the motivation. It looks like a premature optimization to me.
The motivation is to let the driver choose between two behaviors: the
current one where ballooning is only done on request, and a more
aggressive one.
> The spec is pretty clear that if guest acks feature it
> is a contract that dictates behaviour.
> If it doesn't it is either ignored or just informative
> depending on feature.
>
>> You could negotiate VIRTIO_BLK_F_TOPOLOGY
>> and end up never reading the fields; you could negotiate
>> VIRTIO_NET_F_GUEST_ANNOUNCE and never send a guest announcement.
>
> Block example is just informative. It does not need to be
> negotiated even to be used. But last example is wrong.
> If you ack GUEST_ANNOUNCE hypervisor assumes guest will
> announce self, if guest does not do it this break migration.
It is wrong indeed, sorry.
Better example: the driver can negotiate VIRTIO_NET_F_CTRL_RX and never
set promiscuous mode. The device has to obey if it does.
Similarly, if you set VIRTIO_BALLOON_F_SILENT_DEFLATE and only do chatty
deflate later, that's fine. If you do silent deflate, and the device
negotiated the feature, it has to work.
>> Delaying or avoiding is the same in the end. The spec says it well: "In
>> this case, deflation advice is merely a courtesy".
>
> So it looks like we don't need a new bit to leak in atomic ctx.
> Just do not ack MUST_TELL_HOST and delay telling host to a wq.
> IMO we should not add random stuff to spec like this just because it
> seems like a good idea.
But this way you have to choose all-or-none. If the host cannot do
silent deflate, you cannot balloon anymore, not even in the normal
"cooperative" mode.
> OK so TELL says *when* to notify host, SILENT if set allows guest
> to skip leak notifications? In this case TELL should just be ignored
> when SILENT is set.
Yeah, that was my first idea. However, there are existing drivers that
ignore SILENT, so that would not be 100% exact.
> IMHO, renaming is fine since there is confusion.
> But WILL_TELL is also not all that clear imho.
> I think the confusion is that TELL_HOST seems to
> imply we can avoid telling host at all.
> How about being explicit?
>
> VIRTIO_BALLOON_F_HOST_ACK_BEFORE_DEFLATE
Makes sense.
Paolo
^ permalink raw reply
* Re: [PATCH] virtio: console: fix error handling in init() function
From: Rusty Russell @ 2012-09-10 2:19 UTC (permalink / raw)
To: Amit Shah, Alexey Khoroshilov
Cc: ldv-project, Greg Kroah-Hartman, linux-kernel, Arnd Bergmann,
virtualization
In-Reply-To: <20120909201947.GF16985@amit.redhat.com>
Amit Shah <amit.shah@redhat.com> writes:
> On (Sat) 01 Sep 2012 [23:49:37], Alexey Khoroshilov wrote:
>> If register_virtio_driver() fails, virtio-ports class is not destroyed.
>> The patch adds error handling of register_virtio_driver().
>>
>> Found by Linux Driver Verification project (linuxtesting.org).
>>
>> Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
>
> Acked-by: Amit Shah <amit.shah@redhat.com>
>
> Rusty, please pick this up.
Thanks for the CC.
Applied,
Rusty.
^ permalink raw reply
* Re: [PATCHv4] virtio-spec: virtio network device multiqueue support
From: Rusty Russell @ 2012-09-10 2:12 UTC (permalink / raw)
To: Michael S. Tsirkin, kvm, virtualization, netdev
Cc: pbonzini, rick.jones2, levinsasha928, Tom Herbert
In-Reply-To: <20120909130308.GA3471@redhat.com>
OK, I read the spec (pasted below for easy of reading), but I'm still
confused over how this will work.
I thought normal net drivers have the hardware provide an rxhash for
each packet, and we map that to CPU to queue the packet on[1]. We hope
that the receiving process migrates to that CPU, so xmit queue
matches.
For virtio this would mean a new per-packet rxhash value, right?
Why are we doing something different? What am I missing?
Thanks,
Rusty.
[1] Everything I Know About Networking I Learned From LWN:
https://lwn.net/Articles/362339/
---
Transmit Packet Steering
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.
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).
This command accepts a single out argument in the following format:
#define VIRTIO_NET_CTRL_STEERING 4
The field rule specifies the function used to select transmit virtqueue for a given packet; the field param makes it possible to pass an extra parameter if appropriate. When rule 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 rule is set to VIRTIO_NET_CTRL_STEERING_RX_FOLLOWS_TX packets are steered by driver to the first N=(param+1) multiqueue virtqueues transmitq1...transmitqN; the default transmitq is unused. Driver must have configured all these (param+1) virtqueues beforehand.
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 is an 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.
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.
For debugging, the current steering rule can also be read from the configuration space.
Receive Packet Steering
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[sub:Transmit-Packet-Steering].
The field rule specifies the function used to select receive virtqueue for a given packet; the field param makes it possible to pass an extra parameter if appropriate. When rule is set to VIRTIO_NET_CTRL_STEERING_SINGLE all packets are steered to the default virtqueue receiveq (0); param is unused; this is the default. When rule is set to VIRTIO_NET_CTRL_STEERING_RX_FOLLOWS_TX packets are steered by host to the first N=(param+1) multiqueue virtqueues receiveq1...receiveqN; the default receiveq is unused. Driver must have configured all these (param+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, o
r when this mapping information is missing, a device-specific steering function is used.
Supported steering rules can be added and removed in the future. Driver should probe for supported rules by checking ack values of the command.
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.
^ permalink raw reply
* Re: [PATCH] virtio-balloon spec: provide a version of the "silent deflate" feature that works
From: Rusty Russell @ 2012-09-10 1:43 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: fes, aarcange, riel, kvm, yvugenfi, linux-kernel, mikew, yinghan,
Paolo Bonzini, virtualization
In-Reply-To: <20120908223724.GB20588@redhat.com>
"Michael S. Tsirkin" <mst@redhat.com> writes:
> On Sat, Sep 08, 2012 at 02:36:00PM +0930, Rusty Russell wrote:
>> "Michael S. Tsirkin" <mst@redhat.com> writes:
>> > On Fri, Sep 07, 2012 at 04:09:50PM +0930, Rusty Russell wrote:
>> >> > 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?
>> >>
>> >> So, feature is unimplemented in qemu, and broken in drivers. I starting
>> >> to share Paolo's dislike of it.
>> >
>> > What is broken in drivers?
>>
>> Because supporting the feature is *not* optional for a driver.
>>
>> If the device said MUST_TELL_HOST, it meant that the driver *had* to
>> tell the host before it touched the page, otherwise Bad Things might
>> happen. It was in the original spec precisely to allow devices to
>> actually *remove* pages.
>>
>> Noone ever noticed the windows driver didn't support it, because qemu
>> never requires MUST_TELL_HOST.
>>
>> So in practice, it's now an optional feature. Since no device used it
>> anyway, we're better off discarding it than trying to fix it.
>
> I trust you this was not the intent. But it seems to be
> the intent in spec, because almost all features are optional.
>
> And so windows driver authors interpreted it
> this way. And it is *useful* like this. See below.
...
> Suggested use is for device assignment which needs all guest memory
> locked. hypervisor can unlock pages in balloon but guest must wait for
> hypervisor to lock them back before use.
>
> when a hypervisor implements this,
> this will work with linux guests but not windows
> guests and the existing bit fits exactly the purpose.
If a hypervisor needs this, and the guest doesn't support it, then
the hypervisor can only abandon the balloon device. That's not my
definition of "optional".
>> > Do we really know there are no hypervisors implementing it?
>>
>> As much as can be known. Qemu doesn't, lkvm doesn't.
>
> But we can add it in qemu and it will be a useful feature.
>
>> > As I said above drivers do have support.
>>
>> Not the windows drivers. So it's optional, thus removing it will likely
>> harm noone.
>>
>> Cheers,
>> Rusty.
>
> I think the issue is that kvm always locked all guest memory
> for assignment. This restriction is removed
> with vfio which has separate page tables.
> Now that vfio is upstream and work on qemu integration
> is being worked on, we might finally see people using this bit
> to allow memory overcommit with device assignment.
That was left-field.... so you're saying some guest might pull a page
from the balloon and DMA to it, and the vfio device needs to know in
advance that it's going to do it?
But what will we do if the guest doesn't ack the bit?
ie. I don't think it can really be optional.
Cheers,
Rusty.
^ permalink raw reply
* Re: [PATCH] virtio-spec: fix thinko in multiport documentation
From: Amit Shah @ 2012-09-09 20:58 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: virtualization
In-Reply-To: <20120909110837.GA2395@redhat.com>
On (Sun) 09 Sep 2012 [14:08:37], Michael S. Tsirkin wrote:
> spec numbers ports port0, port 1 etc, then goes on to say
> "ports 2 onwards only if MULTIPORT is set".
> An obvious thinko, should be port 1 onwards.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Acked-by: Amit Shah <amit.shah@redhat.com>
Amit
^ permalink raw reply
* Re: [PATCH] virtio-spec: serial: english tweak
From: Amit Shah @ 2012-09-09 20:58 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: virtualization
In-Reply-To: <20120909121116.GA2972@redhat.com>
On (Sun) 09 Sep 2012 [15:11:17], Michael S. Tsirkin wrote:
> "A number of virtqueues are created" seems clearer
> than "the number of virtqueues": it's
> virtqueues that are created not the number.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Acked-by: Amit Shah <amit.shah@redhat.com>
Amit
^ permalink raw reply
* Re: [PATCH] virtio: console: fix error handling in init() function
From: Amit Shah @ 2012-09-09 20:19 UTC (permalink / raw)
To: Alexey Khoroshilov
Cc: Arnd Bergmann, ldv-project, Greg Kroah-Hartman, linux-kernel,
virtualization
In-Reply-To: <1346528977-23066-1-git-send-email-khoroshilov@ispras.ru>
Hi Alexey,
On (Sat) 01 Sep 2012 [23:49:37], Alexey Khoroshilov wrote:
> If register_virtio_driver() fails, virtio-ports class is not destroyed.
> The patch adds error handling of register_virtio_driver().
>
> Found by Linux Driver Verification project (linuxtesting.org).
>
> Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
> ---
> drivers/char/virtio_console.c | 12 +++++++++++-
> 1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index cdf2f54..060a672 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -1941,7 +1941,17 @@ static int __init init(void)
> INIT_LIST_HEAD(&pdrvdata.consoles);
> INIT_LIST_HEAD(&pdrvdata.portdevs);
>
> - return register_virtio_driver(&virtio_console);
> + err = register_virtio_driver(&virtio_console);
> + if (err < 0) {
> + pr_err("Error %d registering virtio driver\n", err);
> + goto free;
> + }
> + return 0;
> +free:
> + if (pdrvdata.debugfs_dir)
> + debugfs_remove_recursive(pdrvdata.debugfs_dir);
> + class_destroy(pdrvdata.class);
> + return err;
> }
>
> static void __exit fini(void)
Patch looks good.
Acked-by: Amit Shah <amit.shah@redhat.com>
Rusty, please pick this up.
Thanks,
Amit
^ permalink raw reply
* Re: [patch 3/3] xen/privcmd: remove const modifier from declaration
From: Andres Lagar-Cavilla @ 2012-09-09 19:50 UTC (permalink / raw)
To: Dan Carpenter
Cc: Jeremy Fitzhardinge, xen-devel, Konrad Rzeszutek Wilk,
Andres Lagar-Cavilla, kernel-janitors, virtualization
In-Reply-To: <20120908095808.GC608@elgon.mountain>
On Sep 8, 2012, at 5:58 AM, Dan Carpenter wrote:
> When we use this pointer, we cast away the const modifier and modify the
> data. I think it was an accident to declare it as const.
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Acked-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
Thanks for the careful review & hardening.
Andres
>
> diff --git a/include/xen/privcmd.h b/include/xen/privcmd.h
> index a853168..58ed953 100644
> --- a/include/xen/privcmd.h
> +++ b/include/xen/privcmd.h
> @@ -69,7 +69,7 @@ struct privcmd_mmapbatch_v2 {
> unsigned int num; /* number of pages to populate */
> domid_t dom; /* target domain */
> __u64 addr; /* virtual address */
> - const xen_pfn_t __user *arr; /* array of mfns */
> + xen_pfn_t __user *arr; /* array of mfns */
> int __user *err; /* array of error codes */
> };
>
> diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
> index 0ce006a..fceb83e 100644
> --- a/drivers/xen/privcmd.c
> +++ b/drivers/xen/privcmd.c
> @@ -389,7 +389,7 @@ static long privcmd_ioctl_mmap_batch(void __user *udata, int version)
>
> if (state.global_error && (version == 1)) {
> /* Write back errors in second pass. */
> - state.user_mfn = (xen_pfn_t *)m.arr;
> + state.user_mfn = m.arr;
> state.err = err_array;
> ret = traverse_pages(m.num, sizeof(xen_pfn_t),
> &pagelist, mmap_return_errors_v1, &state);
^ permalink raw reply
* Re: [patch 2/3] xen/privcmd: return -EFAULT on error
From: Andres Lagar-Cavilla @ 2012-09-09 19:49 UTC (permalink / raw)
To: Dan Carpenter
Cc: Jeremy Fitzhardinge, xen-devel, Konrad Rzeszutek Wilk,
Andres Lagar-Cavilla, kernel-janitors, virtualization
In-Reply-To: <20120908095735.GB608@elgon.mountain>
On Sep 8, 2012, at 5:57 AM, Dan Carpenter wrote:
> __copy_to_user() returns the number of bytes remaining to be copied but
> we want to return a negative error code here.
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Acked-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
>
> diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
> index fdff8f9..0ce006a 100644
> --- a/drivers/xen/privcmd.c
> +++ b/drivers/xen/privcmd.c
> @@ -393,8 +393,11 @@ static long privcmd_ioctl_mmap_batch(void __user *udata, int version)
> state.err = err_array;
> ret = traverse_pages(m.num, sizeof(xen_pfn_t),
> &pagelist, mmap_return_errors_v1, &state);
> - } else if (version == 2)
> + } else if (version == 2) {
> ret = __copy_to_user(m.err, err_array, m.num * sizeof(int));
> + if (ret)
> + ret = -EFAULT;
> + }
>
> /* If we have not had any EFAULT-like global errors then set the global
> * error to -ENOENT if necessary. */
^ 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