* Re: [PATCH 5/5] virtio-scsi: introduce multiqueue support
From: Paolo Bonzini @ 2012-09-04 14:55 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: kvm, linux-scsi, linux-kernel, virtualization
In-Reply-To: <20120904144754.GO9805@redhat.com>
Il 04/09/2012 16:47, Michael S. Tsirkin ha scritto:
>> > static void virtscsi_init_vq(struct virtio_scsi_vq *virtscsi_vq,
>> > - struct virtqueue *vq)
>> > + struct virtqueue *vq, bool affinity)
>> > {
>> > spin_lock_init(&virtscsi_vq->vq_lock);
>> > virtscsi_vq->vq = vq;
>> > + if (affinity)
>> > + virtqueue_set_affinity(vq, virtqueue_get_queue_index(vq) -
>> > + VIRTIO_SCSI_VQ_BASE);
>> > }
>> >
> This means in practice if you have less virtqueues than CPUs,
> things are not going to work well, will they?
Not particularly. It could be better or worse than single queue
depending on the workload.
> Any idea what to do?
Two possibilities:
1) Add a stride argument to virtqueue_set_affinity, and make it equal to
the number of queues.
2) Make multiqueue the default in QEMU, and make the default number of
queues equal to the number of VCPUs.
I was going for (2).
Paolo
^ permalink raw reply
* Re: [PATCH 5/5] virtio-scsi: introduce multiqueue support
From: Michael S. Tsirkin @ 2012-09-04 15:03 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: kvm, linux-scsi, linux-kernel, virtualization
In-Reply-To: <5046167C.3070109@redhat.com>
On Tue, Sep 04, 2012 at 04:55:56PM +0200, Paolo Bonzini wrote:
> Il 04/09/2012 16:47, Michael S. Tsirkin ha scritto:
> >> > static void virtscsi_init_vq(struct virtio_scsi_vq *virtscsi_vq,
> >> > - struct virtqueue *vq)
> >> > + struct virtqueue *vq, bool affinity)
> >> > {
> >> > spin_lock_init(&virtscsi_vq->vq_lock);
> >> > virtscsi_vq->vq = vq;
> >> > + if (affinity)
> >> > + virtqueue_set_affinity(vq, virtqueue_get_queue_index(vq) -
> >> > + VIRTIO_SCSI_VQ_BASE);
> >> > }
> >> >
> > This means in practice if you have less virtqueues than CPUs,
> > things are not going to work well, will they?
>
> Not particularly. It could be better or worse than single queue
> depending on the workload.
Well interrupts will go to CPU different from the one
that sends commands so ...
> > Any idea what to do?
>
> Two possibilities:
>
> 1) Add a stride argument to virtqueue_set_affinity, and make it equal to
> the number of queues.
>
> 2) Make multiqueue the default in QEMU, and make the default number of
> queues equal to the number of VCPUs.
>
> I was going for (2).
>
> Paolo
3. use per target queue if less targets than cpus?
--
MST
^ permalink raw reply
* Re: [PATCH v3 2/2] virtio-ring: Allocate indirect buffers from cache when possible
From: Avi Kivity @ 2012-09-04 16:34 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: linux-kernel, Sasha Levin, kvm, virtualization
In-Reply-To: <20120831095628.GB24244@redhat.com>
On 08/31/2012 12:56 PM, Michael S. Tsirkin wrote:
> On Fri, Aug 31, 2012 at 11:36:07AM +0200, Sasha Levin wrote:
>> On 08/30/2012 03:38 PM, Michael S. Tsirkin wrote:
>> >> +static unsigned int indirect_alloc_thresh = 16;
>> > Why 16? Please make is MAX_SG + 1 this makes some sense.
>>
>> Wouldn't MAX_SG mean we always allocate from the cache? Isn't the memory waste
>> too big in this case?
>
> Sorry. I really meant MAX_SKB_FRAGS + 1. MAX_SKB_FRAGS is 17 so gets us
> threshold of 18. It is less than the size of an skb+shinfo itself so -
> does it look too big to you? Also why do you think 16 is not too big but
> 18 is? If there's a reason then I am fine with 16 too but then please
> put it in code comment near where the value is set.
>
> Yes this means virtio net always allocates from cache
> but this is a good thing, isn't it? Gets us more consistent
> performance.
kmalloc() also goes to a cache. Is there a measurable difference?
Ugh, there's an ugly loop in __find_general_cachep(), which really wants
to be replaced with fls().
--
error compiling committee.c: too many arguments to function
^ permalink raw reply
* Re: [PATCH v3 2/2] virtio-ring: Allocate indirect buffers from cache when possible
From: Avi Kivity @ 2012-09-04 16:36 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: linux-kernel, Sasha Levin, kvm, virtualization
In-Reply-To: <50462D8B.7040004@redhat.com>
On 09/04/2012 07:34 PM, Avi Kivity wrote:
> On 08/31/2012 12:56 PM, Michael S. Tsirkin wrote:
>> On Fri, Aug 31, 2012 at 11:36:07AM +0200, Sasha Levin wrote:
>>> On 08/30/2012 03:38 PM, Michael S. Tsirkin wrote:
>>> >> +static unsigned int indirect_alloc_thresh = 16;
>>> > Why 16? Please make is MAX_SG + 1 this makes some sense.
>>>
>>> Wouldn't MAX_SG mean we always allocate from the cache? Isn't the memory waste
>>> too big in this case?
>>
>> Sorry. I really meant MAX_SKB_FRAGS + 1. MAX_SKB_FRAGS is 17 so gets us
>> threshold of 18. It is less than the size of an skb+shinfo itself so -
>> does it look too big to you? Also why do you think 16 is not too big but
>> 18 is? If there's a reason then I am fine with 16 too but then please
>> put it in code comment near where the value is set.
>>
>> Yes this means virtio net always allocates from cache
>> but this is a good thing, isn't it? Gets us more consistent
>> performance.
>
> kmalloc() also goes to a cache. Is there a measurable difference?
>
> Ugh, there's an ugly loop in __find_general_cachep(), which really wants
> to be replaced with fls().
>
Actually, not, as the loop will be very short for small sizes.
--
error compiling committee.c: too many arguments to function
^ permalink raw reply
* Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation
From: Sjur Brændeland @ 2012-09-04 16:58 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Linus Walleij, linux-kernel, virtualization, Amit Shah
In-Reply-To: <20120904135022.GI9805@redhat.com>
Hi Michael,
> Exactly. Though if we just fail load it will be much less code.
>
> Generally, using a feature bit for this is a bit of a problem though:
> normally driver is expected to be able to simply ignore
> a feature bit. In this case driver is required to
> do something so a feature bit is not a good fit.
> I am not sure what the right thing to do is.
I see - so in order to avoid the binding between driver and device
there are two options I guess. Either make virtio_dev_match() or
virtcons_probe() fail. Neither of them seems like the obvious choice.
Maybe adding a check for VIRTIO_CONSOLE_F_DMA_MEM match
between device and driver in virtcons_probe() is the lesser evil?
Regards,
Sjur
^ permalink raw reply
* Re: [PATCH v3 2/2] virtio-ring: Allocate indirect buffers from cache when possible
From: Michael S. Tsirkin @ 2012-09-04 18:41 UTC (permalink / raw)
To: Avi Kivity; +Cc: linux-kernel, Sasha Levin, kvm, virtualization
In-Reply-To: <50462D8B.7040004@redhat.com>
On Tue, Sep 04, 2012 at 07:34:19PM +0300, Avi Kivity wrote:
> On 08/31/2012 12:56 PM, Michael S. Tsirkin wrote:
> > On Fri, Aug 31, 2012 at 11:36:07AM +0200, Sasha Levin wrote:
> >> On 08/30/2012 03:38 PM, Michael S. Tsirkin wrote:
> >> >> +static unsigned int indirect_alloc_thresh = 16;
> >> > Why 16? Please make is MAX_SG + 1 this makes some sense.
> >>
> >> Wouldn't MAX_SG mean we always allocate from the cache? Isn't the memory waste
> >> too big in this case?
> >
> > Sorry. I really meant MAX_SKB_FRAGS + 1. MAX_SKB_FRAGS is 17 so gets us
> > threshold of 18. It is less than the size of an skb+shinfo itself so -
> > does it look too big to you? Also why do you think 16 is not too big but
> > 18 is? If there's a reason then I am fine with 16 too but then please
> > put it in code comment near where the value is set.
> >
> > Yes this means virtio net always allocates from cache
> > but this is a good thing, isn't it? Gets us more consistent
> > performance.
>
> kmalloc() also goes to a cache. Is there a measurable difference?
Yes see 0/2 and followup discussion.
> Ugh, there's an ugly loop in __find_general_cachep(), which really wants
> to be replaced with fls().
>
> --
> error compiling committee.c: too many arguments to function
^ permalink raw reply
* Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation
From: Michael S. Tsirkin @ 2012-09-04 18:55 UTC (permalink / raw)
To: Sjur Brændeland
Cc: Linus Walleij, linux-kernel, virtualization, Amit Shah
In-Reply-To: <CAJK669Z1UYakBWartChfoeKL_eU5=GFZMGhH8R_Q-difVm0ygQ@mail.gmail.com>
On Tue, Sep 04, 2012 at 06:58:47PM +0200, Sjur Brændeland wrote:
> Hi Michael,
>
> > Exactly. Though if we just fail load it will be much less code.
> >
> > Generally, using a feature bit for this is a bit of a problem though:
> > normally driver is expected to be able to simply ignore
> > a feature bit. In this case driver is required to
> > do something so a feature bit is not a good fit.
> > I am not sure what the right thing to do is.
>
> I see - so in order to avoid the binding between driver and device
> there are two options I guess. Either make virtio_dev_match() or
> virtcons_probe() fail. Neither of them seems like the obvious choice.
>
> Maybe adding a check for VIRTIO_CONSOLE_F_DMA_MEM match
> between device and driver in virtcons_probe() is the lesser evil?
>
> Regards,
> Sjur
A simplest thing to do is change dev id. rusty?
--
MST
^ permalink raw reply
* Re: [PATCH 5/5] virtio-scsi: introduce multiqueue support
From: Nicholas A. Bellinger @ 2012-09-04 20:11 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Jens Axboe, kvm, linux-scsi, mst, linux-kernel, virtualization,
target-devel, Christoph Hellwig
In-Reply-To: <5045A3B4.2030101@redhat.com>
On Tue, 2012-09-04 at 08:46 +0200, Paolo Bonzini wrote:
> Il 04/09/2012 04:21, Nicholas A. Bellinger ha scritto:
> >> @@ -112,6 +118,9 @@ static void virtscsi_complete_cmd(struct virtio_scsi *vscsi, void *buf)
> >> struct virtio_scsi_cmd *cmd = buf;
> >> struct scsi_cmnd *sc = cmd->sc;
> >> struct virtio_scsi_cmd_resp *resp = &cmd->resp.cmd;
> >> + struct virtio_scsi_target_state *tgt = vscsi->tgt[sc->device->id];
> >> +
> >> + atomic_dec(&tgt->reqs);
> >>
> >
> > As tgt->tgt_lock is taken in virtscsi_queuecommand_multi() before the
> > atomic_inc_return(tgt->reqs) check, it seems like using atomic_dec() w/o
> > smp_mb__after_atomic_dec or tgt_lock access here is not using atomic.h
> > accessors properly, no..?
>
> No, only a single "thing" is being accessed, and there is no need to
> order the decrement with respect to preceding or subsequent accesses to
> other locations.
>
> In other words, tgt->reqs is already synchronized with itself, and that
> is enough.
>
> (Besides, on x86 smp_mb__after_atomic_dec is a nop).
>
So the implementation detail wrt to requests to the same target being
processed in FIFO ordering + only being able to change the queue when no
requests are pending helps understand this code more. Thanks for the
explanation on that bit..
However, it's still my understanding that the use of atomic_dec() in the
completion path mean that smp_mb__after_atomic_dec() is a requirement to
be proper portable atomic.hcode, no..? Otherwise tgt->regs should be
using something other than an atomic_t, right..?
> >> +static int virtscsi_queuecommand_multi(struct Scsi_Host *sh,
> >> + struct scsi_cmnd *sc)
> >> +{
> >> + struct virtio_scsi *vscsi = shost_priv(sh);
> >> + struct virtio_scsi_target_state *tgt = vscsi->tgt[sc->device->id];
> >> + unsigned long flags;
> >> + u32 queue_num;
> >> +
> >> + /* Using an atomic_t for tgt->reqs lets the virtqueue handler
> >> + * decrement it without taking the spinlock.
> >> + */
> >> + spin_lock_irqsave(&tgt->tgt_lock, flags);
> >> + if (atomic_inc_return(&tgt->reqs) == 1) {
> >> + queue_num = smp_processor_id();
> >> + while (unlikely(queue_num >= vscsi->num_queues))
> >> + queue_num -= vscsi->num_queues;
> >> + tgt->req_vq = &vscsi->req_vqs[queue_num];
> >> + }
> >> + spin_unlock_irqrestore(&tgt->tgt_lock, flags);
> >> + return virtscsi_queuecommand(vscsi, tgt, sc);
> >> +}
> >> +
> >
> > The extra memory barriers to get this right for the current approach are
> > just going to slow things down even more for virtio-scsi-mq..
>
> virtio-scsi multiqueue has a performance benefit up to 20% (for a single
> LUN) or 40% (on overall bandwidth across multiple LUNs). I doubt that a
> single memory barrier can have that much impact. :)
>
I've no doubt that this series increases the large block high bandwidth
for virtio-scsi, but historically that has always been the easier
workload to scale. ;)
> The way to go to improve performance even more is to add new virtio APIs
> for finer control of the usage of the ring. These should let us avoid
> copying the sg list and almost get rid of the tgt_lock; even though the
> locking is quite efficient in virtio-scsi (see how tgt_lock and vq_lock
> are "pipelined" so as to overlap the preparation of two requests), it
> should give a nice improvement and especially avoid a kmalloc with small
> requests. I may have some time for it next month.
>
> > Jen's approach is what we will ultimately need to re-architect in SCSI
> > core if we're ever going to move beyond the issues of legacy host_lock,
> > so I'm wondering if maybe this is the direction that virtio-scsi-mq
> > needs to go in as well..?
>
> We can see after the block layer multiqueue work goes in... I also need
> to look more closely at Jens's changes.
>
Yes, I think Jen's new approach is providing some pretty significant
gains for raw block drivers with extremly high packet (small block
random I/O) workloads, esp with hw block drivers that support genuine mq
with hw num_queues > 1.
He also has virtio-blk converted to run in num_queues=1 mode.
> Have you measured the host_lock to be a bottleneck in high-iops
> benchmarks, even for a modern driver that does not hold it in
> queuecommand? (Certainly it will become more important as the
> virtio-scsi queuecommand becomes thinner and thinner).
This is exactly why it would make such a good vehicle to re-architect
SCSI core. I'm thinking it can be the first sw LLD we attempt to get
running on an (currently) future scsi-mq prototype.
> If so, we can
> start looking at limiting host_lock usage in the fast path.
>
That would be a good incremental step for SCSI core, but I'm not sure
that that we'll be able to scale compared to blk-mq without a
new-approach for sw/hw LLDs along the lines of what Jen's is doing.
> BTW, supporting this in tcm-vhost should be quite trivial, as all the
> request queues are the same and all serialization is done in the
> virtio-scsi driver.
>
Looking forward to that too.. ;)
^ permalink raw reply
* Re: [PATCH] virtio-blk: Fix kconfig option
From: Kent Overstreet @ 2012-09-04 23:12 UTC (permalink / raw)
To: Rusty Russell; +Cc: virtualization, linux-kernel, mst
In-Reply-To: <87wr0ae26e.fsf@rustcorp.com.au>
On Mon, Sep 3, 2012 at 11:23 PM, Rusty Russell <rusty@rustcorp.com.au> 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.
Yes... VIRT_BLOCK isn't selecting it.
^ permalink raw reply
* Re: [PATCHv2] virtio-spec: virtio network device multiqueue support
From: Jason Wang @ 2012-09-05 3:34 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: netdev, kvm, virtualization
In-Reply-To: <20120903115526.GA3815@redhat.com>
On 09/03/2012 07:55 PM, Michael S. Tsirkin wrote:
> At Jason's request, I am trying to help finalize the spec for
> the new multiqueue feature.
>
> 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.
For host driven, more thought in the long term. Maybe we could add more
policy to choose the rxq such as hashing, round-robin and cpuid.
> - 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.
Not sure whether or not this can really helps. Depending on the host
scheduler, we may always see a disorder when we do the switching.
> 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.
>
> -->
>
> virtio-spec: virtio network device multiqueue support
>
> 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>
>
> --
>
> diff --git a/virtio-spec.lyx b/virtio-spec.lyx
> index 7a073f4..583debc 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 1346672138
> + 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 used for multiqueue operation.
s/can/can be/
> + 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.
> +
> +\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
> @@ -4516,6 +4643,201 @@ Note that the header will be two bytes longer for the VIRTIO_NET_F_MRG_RXBUF
> \end_layout
>
> \begin_layout Subsection*
> +
> +\change_inserted 1986246365 1346670975
> +\begin_inset CommandInset label
> +LatexCommand label
> +name "sub:Transmit-Packet-Steering"
> +
> +\end_inset
> +
> +Transmit Packet Steering
> +\end_layout
> +
Since this needs control vq, move this after the section of control vq?
> +\begin_layout Standard
> +
> +\change_inserted 1986246365 1346670592
> +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 gnerally 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 1346670727
> +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
> +
Not sure it's suitable to define the policy in the spec ( and even a not
hard requirement ). An alternative is to just define some tools such as
the queue number negotiation mechanism like modern card and just let the
driver to choose the policy, this can give both flexibility and space
for the future evolution of the device. E.g. we may have a guest
controlled flow director in the future, we can have a seperate command
to enable/disable it and a separate feature bits.
> +\change_inserted 1986246365 1346670594
> +This command accepts a single out argument in the following format:
> +\end_layout
> +
> +\begin_layout Standard
> +
> +\change_inserted 1986246365 1346670594
> +\begin_inset listings
> +inline false
> +status open
> +
> +\begin_layout Plain Layout
> +
> +\change_inserted 1986246365 1346670594
> +
> +#define VIRTIO_NET_CTRL_STEERING 4
> +\end_layout
> +
> +\begin_layout Plain Layout
> +
> +\change_inserted 1986246365 1346670594
> +
> +\end_layout
> +
> +\begin_layout Plain Layout
> +
> +\change_inserted 1986246365 1346670594
> +
> +struct virtio_net_ctrl_steering {
> +\end_layout
> +
> +\begin_layout Plain Layout
> +
> +\change_inserted 1986246365 1346671425
> +
> + u8 current_steering_rule;
> +\end_layout
> +
> +\begin_layout Plain Layout
> +
> +\change_inserted 1986246365 1346670594
> +
> + u8 reserved;
> +\end_layout
> +
> +\begin_layout Plain Layout
> +
> +\change_inserted 1986246365 1346671485
> +
> + u16 current_steering_param;
> +\end_layout
> +
> +\begin_layout Plain Layout
> +
> +\change_inserted 1986246365 1346670594
> +
> +};
> +\end_layout
> +
> +\begin_layout Plain Layout
> +
> +\change_inserted 1986246365 1346670594
> +
> +#define VIRTIO_NET_CTRL_STEERING_SINGLE 0
> +\end_layout
> +
> +\begin_layout Plain Layout
> +
> +\change_inserted 1986246365 1346670594
> +
> +#define VIRTIO_NET_CTRL_STEERING_HOST 1
> +\end_layout
> +
> +\end_inset
> +
> +
> +\end_layout
> +
> +\begin_layout Standard
> +
> +\change_inserted 1986246365 1346671803
> +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 all packets are steered to the
> + default virtqueue transmitq (1); param is unused; this is the default.
> + When
> +\emph on
> +rule
> +\emph default
> + is set to VIRTIO_NET_CTRL_STEERING_HOST packets are steered by driver to
> + the first (
> +\emph on
> +param
> +\emph default
> ++1) multiqueue virtqueues transmitq1...transmitqN; the default transmitq is
> + unused.
It's driver that select the transmitq, does the device need to know
about the transmit steering used in driver? defining the those
transmission steering commands for the deivce looks useless. Maybe we'd
better rename the subsection to "Packet Steering" and focus on the
receive part?
> + Driver must have configured all these (
> +\emph on
> +param
> +\emph default
> ++1) virtqueues beforehand.
> + For best performance, driver should detects flow to virtqueue pair mapping
> + on receive and selects the transmit virtqueue from the same virtqueue pair.
> +\end_layout
> +
> +\begin_layout Standard
> +
> +\change_inserted 1986246365 1346670762
> +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
> +
What's the advantages of using this over feature bits?
> +\begin_layout Standard
> +
> +\change_inserted 1986246365 1346671315
> +When steering rule is modified, some packets can still be outstanding in
> + one or more of the virtqueues.
> + For transmit, 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 1346671412
> +For debugging, current steering rule can also be read from the configuration
> + space.
> +\end_layout
> +
> +\begin_layout Standard
> +
> +\change_inserted 1986246365 1346670762
> +See also receive steering description
> +\begin_inset CommandInset ref
> +LatexCommand ref
> +reference "sub:Receive-Packet-Steering"
> +
> +\end_inset
> +
> +.
> +\end_layout
> +
> +\begin_layout Subsection*
> Packet Transmission Interrupt
> \end_layout
>
> @@ -4988,8 +5310,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 +5342,101 @@ 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 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 1346671818
> +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.
A question here is whether or not we need to do the seamless switching
between mq modes and sq modes. If we expect it happens rare, we could
afforad some disordering by dropping this and just do the switching by
resetting the device and negotiating the MULTIQUEUE bit or not on
demand. This can simplify the implemention.
> + When
> +\emph on
> +rule
> +\emph default
> + is set to VIRTIO_NET_CTRL_STEERING_HOST packets are steered by host using
> + a device-specific steering function 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, driver is expected to detect flow to virtqueue pair
> + mapping on receive and select the transmit virtqueue from the same virtqueue
> + pair.
> +\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 1346671151
> +When steering rule is modified, some packets can still be outstanding in
> + one or more of the virtqueues.
> + For receive, driver is 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
* Re: [PATCH] virtio-blk: Fix kconfig option
From: Asias He @ 2012-09-05 4:22 UTC (permalink / raw)
To: Rusty Russell; +Cc: Kent Overstreet, virtualization, linux-kernel, mst
In-Reply-To: <87wr0ae26e.fsf@rustcorp.com.au>
On 09/04/2012 02:23 PM, 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.
virtio_mmio selects virtio as well.
drivers/virtio/Kconfig
config VIRTIO_MMIO
select VIRTIO
config VIRTIO_PCI
select VIRTIO
How about this:
config VIRTIO_BLK
depends on EXPERIMENTAL && (VIRTIO_PCI || VIRTIO_MMIO)
--
Asias
^ permalink raw reply
* Re: [PATCHv2] virtio-spec: virtio network device multiqueue support
From: Michael S. Tsirkin @ 2012-09-05 5:34 UTC (permalink / raw)
To: Jason Wang; +Cc: netdev, kvm, virtualization
In-Reply-To: <5046C837.7040609@redhat.com>
On Wed, Sep 05, 2012 at 11:34:15AM +0800, Jason Wang wrote:
> On 09/03/2012 07:55 PM, Michael S. Tsirkin wrote:
> >At Jason's request, I am trying to help finalize the spec for
> >the new multiqueue feature.
> >
> >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.
>
> For host driven, more thought in the long term. Maybe we could add
> more policy to choose the rxq such as hashing, round-robin and
> cpuid.
As we discussed off-list, different guests may need wildly
different strategies. For example different queues for
different qos priorities might make a lot of sense.
So for now I'll remove the host-driven option and
add _GUEST (or maybe better name is _RX_FOLLOWS_TX)
rule which records the queue number on packet transmit and
uses that on receive.
> >- 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.
>
> Not sure whether or not this can really helps. Depending on the host
> scheduler, we may always see a disorder when we do the switching.
Since guest handles one queue at a time during switch,
won't this mean host reorders packets even with a single queue?
> >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.
> >
> >-->
> >
> >virtio-spec: virtio network device multiqueue support
> >
> >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>
> >
> >--
> >
> >diff --git a/virtio-spec.lyx b/virtio-spec.lyx
> >index 7a073f4..583debc 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 1346672138
> >+ 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 used for multiqueue operation.
>
> s/can/can be/
> >+ 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.
> >+
> >+\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
> >@@ -4516,6 +4643,201 @@ Note that the header will be two bytes longer for the VIRTIO_NET_F_MRG_RXBUF
> > \end_layout
> >
> > \begin_layout Subsection*
> >+
> >+\change_inserted 1986246365 1346670975
> >+\begin_inset CommandInset label
> >+LatexCommand label
> >+name "sub:Transmit-Packet-Steering"
> >+
> >+\end_inset
> >+
> >+Transmit Packet Steering
> >+\end_layout
> >+
>
> Since this needs control vq, move this after the section of control vq?
I don't mind but this is also transmit related.
> >+\begin_layout Standard
> >+
> >+\change_inserted 1986246365 1346670592
> >+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 gnerally 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 1346670727
> >+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
> >+
>
> Not sure it's suitable to define the policy in the spec ( and even a
> not hard requirement ). An alternative is to just define some tools
> such as the queue number negotiation mechanism like modern card and
> just let the driver to choose the policy, this can give both
> flexibility and space for the future evolution of the device. E.g.
> we may have a guest controlled flow director in the future, we can
> have a seperate command to enable/disable it and a separate feature
> bits.
Problem with such approaches is, if we make them very generic they
will be slow (interpreter) or hard to implement (jit compiler).
I think rx follows tx above solves this neatly without
a lot of complexity.
> >+\change_inserted 1986246365 1346670594
> >+This command accepts a single out argument in the following format:
> >+\end_layout
> >+
> >+\begin_layout Standard
> >+
> >+\change_inserted 1986246365 1346670594
> >+\begin_inset listings
> >+inline false
> >+status open
> >+
> >+\begin_layout Plain Layout
> >+
> >+\change_inserted 1986246365 1346670594
> >+
> >+#define VIRTIO_NET_CTRL_STEERING 4
> >+\end_layout
> >+
> >+\begin_layout Plain Layout
> >+
> >+\change_inserted 1986246365 1346670594
> >+
> >+\end_layout
> >+
> >+\begin_layout Plain Layout
> >+
> >+\change_inserted 1986246365 1346670594
> >+
> >+struct virtio_net_ctrl_steering {
> >+\end_layout
> >+
> >+\begin_layout Plain Layout
> >+
> >+\change_inserted 1986246365 1346671425
> >+
> >+ u8 current_steering_rule;
> >+\end_layout
> >+
> >+\begin_layout Plain Layout
> >+
> >+\change_inserted 1986246365 1346670594
> >+
> >+ u8 reserved;
> >+\end_layout
> >+
> >+\begin_layout Plain Layout
> >+
> >+\change_inserted 1986246365 1346671485
> >+
> >+ u16 current_steering_param;
> >+\end_layout
> >+
> >+\begin_layout Plain Layout
> >+
> >+\change_inserted 1986246365 1346670594
> >+
> >+};
> >+\end_layout
> >+
> >+\begin_layout Plain Layout
> >+
> >+\change_inserted 1986246365 1346670594
> >+
> >+#define VIRTIO_NET_CTRL_STEERING_SINGLE 0
> >+\end_layout
> >+
> >+\begin_layout Plain Layout
> >+
> >+\change_inserted 1986246365 1346670594
> >+
> >+#define VIRTIO_NET_CTRL_STEERING_HOST 1
> >+\end_layout
> >+
> >+\end_inset
> >+
> >+
> >+\end_layout
> >+
> >+\begin_layout Standard
> >+
> >+\change_inserted 1986246365 1346671803
> >+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 all packets are steered to the
> >+ default virtqueue transmitq (1); param is unused; this is the default.
> >+ When
> >+\emph on
> >+rule
> >+\emph default
> >+ is set to VIRTIO_NET_CTRL_STEERING_HOST packets are steered by driver to
> >+ the first (
> >+\emph on
> >+param
> >+\emph default
> >++1) multiqueue virtqueues transmitq1...transmitqN; the default transmitq is
> >+ unused.
>
> It's driver that select the transmitq, does the device need to know
> about the transmit steering used in driver? defining the those
> transmission steering commands for the deivce looks useless. Maybe
> we'd better rename the subsection to "Packet Steering" and focus on
> the receive part?
Fow rx follows tx device needs to know how many tx queues
will be used.
Also I think it's good to be explicit and write up what
we expect driver to do rather than leave it to
drivers to interpret the spec any way they like.
> >+ Driver must have configured all these (
> >+\emph on
> >+param
> >+\emph default
> >++1) virtqueues beforehand.
> >+ For best performance, driver should detects flow to virtqueue pair mapping
> >+ on receive and selects the transmit virtqueue from the same virtqueue pair.
> >+\end_layout
> >+
> >+\begin_layout Standard
> >+
> >+\change_inserted 1986246365 1346670762
> >+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
> >+
>
> What's the advantages of using this over feature bits?
Feature bits are negotiated at startup. Experience shows
there might be no ideal steering rule for all workloads
so switching them at runtime is needed.
> >+\begin_layout Standard
> >+
> >+\change_inserted 1986246365 1346671315
> >+When steering rule is modified, some packets can still be outstanding in
> >+ one or more of the virtqueues.
> >+ For transmit, 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 1346671412
> >+For debugging, current steering rule can also be read from the configuration
> >+ space.
> >+\end_layout
> >+
> >+\begin_layout Standard
> >+
> >+\change_inserted 1986246365 1346670762
> >+See also receive steering description
> >+\begin_inset CommandInset ref
> >+LatexCommand ref
> >+reference "sub:Receive-Packet-Steering"
> >+
> >+\end_inset
> >+
> >+.
> >+\end_layout
> >+
> >+\begin_layout Subsection*
> > Packet Transmission Interrupt
> > \end_layout
> >
> >@@ -4988,8 +5310,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 +5342,101 @@ 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 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 1346671818
> >+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.
>
> A question here is whether or not we need to do the seamless
> switching between mq modes and sq modes. If we expect it happens
> rare, we could afforad some disordering by dropping this and just do
> the switching by resetting the device and negotiating the MULTIQUEUE
> bit or not on demand.
So far your own performance results show there is no
ideal mode. And to me, this makes a lot of sense.
If you reset on switch this is very slow, so this basically means you
will forever rely on guest admin to know what the right thing to do is.
I agree it's a sensible thing to do in v1 but long term
an adaptive strategy requires that mode switch is lightweight.
> This can simplify the implemention.
It only seems like that. That's much more work for a driver to do, and
lots of races to avoid. Just using vqs negotiated at startup is exactly
what all drivers currently do - it is much easier to do correctly.
> >+ When
> >+\emph on
> >+rule
> >+\emph default
> >+ is set to VIRTIO_NET_CTRL_STEERING_HOST packets are steered by host using
> >+ a device-specific steering function 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, driver is expected to detect flow to virtqueue pair
> >+ mapping on receive and select the transmit virtqueue from the same virtqueue
> >+ pair.
> >+\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 1346671151
> >+When steering rule is modified, some packets can still be outstanding in
> >+ one or more of the virtqueues.
> >+ For receive, driver is 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
* Re: [PATCH] virtio-blk: Fix kconfig option
From: Michael S. Tsirkin @ 2012-09-05 5:48 UTC (permalink / raw)
To: Asias He; +Cc: Kent Overstreet, linux-kernel, virtualization
In-Reply-To: <5046D3A2.3070108@redhat.com>
On Wed, Sep 05, 2012 at 12:22:58PM +0800, Asias He wrote:
> On 09/04/2012 02:23 PM, 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.
>
> virtio_mmio selects virtio as well.
>
> drivers/virtio/Kconfig
> config VIRTIO_MMIO
> select VIRTIO
> config VIRTIO_PCI
> select VIRTIO
>
> How about this:
> config VIRTIO_BLK
> depends on EXPERIMENTAL && (VIRTIO_PCI || VIRTIO_MMIO)
Isn't this exactly what depends on VIRTIO achieves?
> --
> Asias
^ permalink raw reply
* Re: [PATCH] virtio-blk: Fix kconfig option
From: Asias He @ 2012-09-05 5:54 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Kent Overstreet, linux-kernel, virtualization
In-Reply-To: <20120905054830.GB7517@redhat.com>
On 09/05/2012 01:48 PM, Michael S. Tsirkin wrote:
> On Wed, Sep 05, 2012 at 12:22:58PM +0800, Asias He wrote:
>> On 09/04/2012 02:23 PM, 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.
>>
>> virtio_mmio selects virtio as well.
>>
>> drivers/virtio/Kconfig
>> config VIRTIO_MMIO
>> select VIRTIO
>> config VIRTIO_PCI
>> select VIRTIO
>>
>> How about this:
>> config VIRTIO_BLK
>> depends on EXPERIMENTAL && (VIRTIO_PCI || VIRTIO_MMIO)
>
> Isn't this exactly what depends on VIRTIO achieves?
Yes. But this gives the information that virtio_blk depends on one of
the virtio transport: virito_pci or virtio_mmio.
--
Asias
^ permalink raw reply
* Re: [PATCH] virtio-blk: Fix kconfig option
From: Michael S. Tsirkin @ 2012-09-05 6:04 UTC (permalink / raw)
To: Asias He; +Cc: Kent Overstreet, linux-kernel, virtualization
In-Reply-To: <5046E8FF.9020504@redhat.com>
On Wed, Sep 05, 2012 at 01:54:07PM +0800, Asias He wrote:
> On 09/05/2012 01:48 PM, Michael S. Tsirkin wrote:
> > On Wed, Sep 05, 2012 at 12:22:58PM +0800, Asias He wrote:
> >> On 09/04/2012 02:23 PM, 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.
> >>
> >> virtio_mmio selects virtio as well.
> >>
> >> drivers/virtio/Kconfig
> >> config VIRTIO_MMIO
> >> select VIRTIO
> >> config VIRTIO_PCI
> >> select VIRTIO
> >>
> >> How about this:
> >> config VIRTIO_BLK
> >> depends on EXPERIMENTAL && (VIRTIO_PCI || VIRTIO_MMIO)
> >
> > Isn't this exactly what depends on VIRTIO achieves?
>
> Yes. But this gives the information that virtio_blk depends on one of
> the virtio transport: virito_pci or virtio_mmio.
It can work with any transport.
> --
> Asias
^ permalink raw reply
* Re: [PATCH] virtio-blk: Fix kconfig option
From: Michael S. Tsirkin @ 2012-09-05 6:12 UTC (permalink / raw)
To: Rusty Russell; +Cc: Kent Overstreet, linux-kernel, virtualization
In-Reply-To: <87wr0ae26e.fsf@rustcorp.com.au>
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.
>
> Cheers,
> Rusty.
Actually balloon selects VIRTIO, I think it's a bug.
Also isn't it time we dropped the experimental tag?
Leaving it in for now.
-->
virtio-balloon: dependency fix
Devices should depend on virtio, not select it. It's supposed to be
selected by the particular driver, e.g. VIRTIO_PCI.
Make balloon depend on VIRTIO and EXPERIMENTAL
(to match description).
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
--
diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
index f38b17a..564bb64 100644
--- a/drivers/virtio/Kconfig
+++ b/drivers/virtio/Kconfig
@@ -27,8 +27,7 @@ config VIRTIO_PCI
config VIRTIO_BALLOON
tristate "Virtio balloon driver (EXPERIMENTAL)"
- select VIRTIO
- select VIRTIO_RING
+ depends on EXPERIMENTAL && VIRTIO
---help---
This driver supports increasing and decreasing the amount
of memory within a KVM guest.
^ permalink raw reply related
* Re: [PATCH 5/5] virtio-scsi: introduce multiqueue support
From: Paolo Bonzini @ 2012-09-05 7:03 UTC (permalink / raw)
To: Nicholas A. Bellinger
Cc: Jens Axboe, kvm, linux-scsi, mst, linux-kernel, virtualization,
target-devel, Christoph Hellwig
In-Reply-To: <1346789466.4162.181.camel@haakon2.linux-iscsi.org>
Il 04/09/2012 22:11, Nicholas A. Bellinger ha scritto:
>>> As tgt->tgt_lock is taken in virtscsi_queuecommand_multi() before the
>>> atomic_inc_return(tgt->reqs) check, it seems like using atomic_dec() w/o
>>> smp_mb__after_atomic_dec or tgt_lock access here is not using atomic.h
>>> accessors properly, no..?
>>
>> No, only a single "thing" is being accessed, and there is no need to
>> order the decrement with respect to preceding or subsequent accesses to
>> other locations.
>>
>> In other words, tgt->reqs is already synchronized with itself, and that
>> is enough.
>>
>
> However, it's still my understanding that the use of atomic_dec() in the
> completion path mean that smp_mb__after_atomic_dec() is a requirement to
> be proper portable atomic.hcode, no..? Otherwise tgt->regs should be
> using something other than an atomic_t, right..?
Memory barriers aren't _always_ requested, only when you need to order
accesses to multiple locations.
In this case, there is no other location that the
queuecommand/completion handlers needs to synchronize against, so no
barrier is required. You can see plenty of atomic_inc/atomic_dec in the
code without a barrier afterwards (the typical case is the opposite as
in this patch: a refcount increment needs no barrier, a refcount
decrement uses atomic_dec_return).
>> virtio-scsi multiqueue has a performance benefit up to 20% (for a single
>> LUN) or 40% (on overall bandwidth across multiple LUNs). I doubt that a
>> single memory barrier can have that much impact. :)
>>
>
> I've no doubt that this series increases the large block high bandwidth
> for virtio-scsi, but historically that has always been the easier
> workload to scale. ;)
This is with a mixed workload (random 4k-64k) and tmpfs backend on the host.
> Yes, I think Jen's new approach is providing some pretty significant
> gains for raw block drivers with extremly high packet (small block
> random I/O) workloads, esp with hw block drivers that support genuine mq
> with hw num_queues > 1.
I need to look into it, to understand how the queue steering here can be
adapted to his code.
>> Have you measured the host_lock to be a bottleneck in high-iops
>> benchmarks, even for a modern driver that does not hold it in
>> queuecommand? (Certainly it will become more important as the
>> virtio-scsi queuecommand becomes thinner and thinner).
>
> This is exactly why it would make such a good vehicle to re-architect
> SCSI core. I'm thinking it can be the first sw LLD we attempt to get
> running on an (currently) future scsi-mq prototype.
Agreed.
Paolo
^ permalink raw reply
* [patch] virtio-blk: fix NULL checking in virtblk_alloc_req()
From: Dan Carpenter @ 2012-09-05 12:32 UTC (permalink / raw)
To: Asias He; +Cc: kernel-janitors, virtualization, Michael S. Tsirkin
Smatch complains about the inconsistent NULL checking here. Fix it to
return NULL on failure.
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
This is only needed in linux-next.
diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 2edfb5c..457db0c 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -90,10 +90,11 @@ static inline struct virtblk_req *virtblk_alloc_req(struct virtio_blk *vblk,
struct virtblk_req *vbr;
vbr = mempool_alloc(vblk->pool, gfp_mask);
- if (vbr && use_bio)
- sg_init_table(vbr->sg, vblk->sg_elems);
+ if (!vbr)
+ return NULL;
- vbr->vblk = vblk;
+ if (use_bio)
+ sg_init_table(vbr->sg, vblk->sg_elems);
return vbr;
}
^ permalink raw reply related
* Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation
From: Sjur Brændeland @ 2012-09-05 13:00 UTC (permalink / raw)
To: Rusty Russell, Michael S. Tsirkin
Cc: Linus Walleij, linux-kernel, virtualization, Amit Shah,
Sjur Brændeland
In-Reply-To: <87zk56e2xc.fsf@rustcorp.com.au>
> The driver certainly shouldn't offer VIRTIO_CONSOLE_F_DMA_MEM if you
> don't have DMA!
OK, so the feature table could be done like this:
static unsigned int features[] = {
VIRTIO_CONSOLE_F_SIZE,
VIRTIO_CONSOLE_F_MULTIPORT,
#if VIRTIO_CONSOLE_HAS_DMA
VIRTIO_CONSOLE_F_DMA_MEM,
#endif
}
If the device then asks for VIRTIO_CONSOLE_F_DMA_MEM
when DMA is not supported, virtio will do BUG_ON() from
virtio_check_driver_offered_feature().
Is this acceptable or should we add a check in virtcons_probe()
and let the probing fail instead?
E.g:
/* Refuse to bind if F_DMA_MEM request cannot be met */
if (!VIRTIO_CONSOLE_HAS_DMA &&
(vdev->config->get_features(vdev) & (1 << VIRTIO_CONSOLE_F_DMA_MEM))){
dev_err(&vdev->dev,
"DMA_MEM requested, but arch does not support DMA\n");
err = -EINVAL;
goto fail;
}
Regards,
Sjur
^ permalink raw reply
* Re: [patch] virtio-blk: fix NULL checking in virtblk_alloc_req()
From: Michael S. Tsirkin @ 2012-09-05 13:11 UTC (permalink / raw)
To: Dan Carpenter; +Cc: kernel-janitors, virtualization
In-Reply-To: <20120905123252.GE6128@elgon.mountain>
On Wed, Sep 05, 2012 at 03:32:53PM +0300, Dan Carpenter wrote:
> Smatch complains about the inconsistent NULL checking here. Fix it to
> return NULL on failure.
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
ACK
> ---
> This is only needed in linux-next.
Yes upstream is OK.
linux-next picks up stuff from rusty's patch queue so
presumably this can be folded into patch that
triggered this.
>
> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index 2edfb5c..457db0c 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -90,10 +90,11 @@ static inline struct virtblk_req *virtblk_alloc_req(struct virtio_blk *vblk,
> struct virtblk_req *vbr;
>
> vbr = mempool_alloc(vblk->pool, gfp_mask);
> - if (vbr && use_bio)
> - sg_init_table(vbr->sg, vblk->sg_elems);
> + if (!vbr)
> + return NULL;
>
> - vbr->vblk = vblk;
Smatch is right to complain: on memory allocation
failure this will dereference NULL.
> + if (use_bio)
> + sg_init_table(vbr->sg, vblk->sg_elems);
>
> return vbr;
> }
^ permalink raw reply
* Re: [PATCH v3 2/2] virtio-ring: Allocate indirect buffers from cache when possible
From: Avi Kivity @ 2012-09-05 14:21 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: linux-kernel, Sasha Levin, kvm, virtualization
In-Reply-To: <20120904184122.GA3602@redhat.com>
On 09/04/2012 09:41 PM, Michael S. Tsirkin wrote:
> On Tue, Sep 04, 2012 at 07:34:19PM +0300, Avi Kivity wrote:
>> On 08/31/2012 12:56 PM, Michael S. Tsirkin wrote:
>> > On Fri, Aug 31, 2012 at 11:36:07AM +0200, Sasha Levin wrote:
>> >> On 08/30/2012 03:38 PM, Michael S. Tsirkin wrote:
>> >> >> +static unsigned int indirect_alloc_thresh = 16;
>> >> > Why 16? Please make is MAX_SG + 1 this makes some sense.
>> >>
>> >> Wouldn't MAX_SG mean we always allocate from the cache? Isn't the memory waste
>> >> too big in this case?
>> >
>> > Sorry. I really meant MAX_SKB_FRAGS + 1. MAX_SKB_FRAGS is 17 so gets us
>> > threshold of 18. It is less than the size of an skb+shinfo itself so -
>> > does it look too big to you? Also why do you think 16 is not too big but
>> > 18 is? If there's a reason then I am fine with 16 too but then please
>> > put it in code comment near where the value is set.
>> >
>> > Yes this means virtio net always allocates from cache
>> > but this is a good thing, isn't it? Gets us more consistent
>> > performance.
>>
>> kmalloc() also goes to a cache. Is there a measurable difference?
>
> Yes see 0/2 and followup discussion.
I don't see 0/2, looks like this was not threaded properly. What was
the subject line?
--
error compiling committee.c: too many arguments to function
^ permalink raw reply
* Re: [PATCH v3 2/2] virtio-ring: Allocate indirect buffers from cache when possible
From: Michael S. Tsirkin @ 2012-09-05 14:27 UTC (permalink / raw)
To: Avi Kivity; +Cc: linux-kernel, Sasha Levin, kvm, virtualization
In-Reply-To: <50475FD8.1090109@redhat.com>
On Wed, Sep 05, 2012 at 05:21:12PM +0300, Avi Kivity wrote:
> On 09/04/2012 09:41 PM, Michael S. Tsirkin wrote:
> > On Tue, Sep 04, 2012 at 07:34:19PM +0300, Avi Kivity wrote:
> >> On 08/31/2012 12:56 PM, Michael S. Tsirkin wrote:
> >> > On Fri, Aug 31, 2012 at 11:36:07AM +0200, Sasha Levin wrote:
> >> >> On 08/30/2012 03:38 PM, Michael S. Tsirkin wrote:
> >> >> >> +static unsigned int indirect_alloc_thresh = 16;
> >> >> > Why 16? Please make is MAX_SG + 1 this makes some sense.
> >> >>
> >> >> Wouldn't MAX_SG mean we always allocate from the cache? Isn't the memory waste
> >> >> too big in this case?
> >> >
> >> > Sorry. I really meant MAX_SKB_FRAGS + 1. MAX_SKB_FRAGS is 17 so gets us
> >> > threshold of 18. It is less than the size of an skb+shinfo itself so -
> >> > does it look too big to you? Also why do you think 16 is not too big but
> >> > 18 is? If there's a reason then I am fine with 16 too but then please
> >> > put it in code comment near where the value is set.
> >> >
> >> > Yes this means virtio net always allocates from cache
> >> > but this is a good thing, isn't it? Gets us more consistent
> >> > performance.
> >>
> >> kmalloc() also goes to a cache. Is there a measurable difference?
> >
> > Yes see 0/2 and followup discussion.
>
> I don't see 0/2, looks like this was not threaded properly. What was
> the subject line?
My mistake, there is no 0/2, the resolts where in the followup thread
of the previous version:
[PATCH v2 2/2] virtio-ring: Allocate indirect buffers from cache when possible
Sasha, could you please accompany the next version
with a cover letter 0/2) including performance results?
>
> --
> error compiling committee.c: too many arguments to function
^ permalink raw reply
* Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation
From: Michael S. Tsirkin @ 2012-09-05 14:35 UTC (permalink / raw)
To: Sjur Brændeland
Cc: Linus Walleij, linux-kernel, virtualization, Amit Shah,
Sjur Brændeland
In-Reply-To: <CAJK669bUXHqaDUZDNZ9i85pRKENUMS_RXw4z7BsSq7Hiw=OOfg@mail.gmail.com>
On Wed, Sep 05, 2012 at 03:00:20PM +0200, Sjur Brændeland wrote:
> > The driver certainly shouldn't offer VIRTIO_CONSOLE_F_DMA_MEM if you
> > don't have DMA!
>
> OK, so the feature table could be done like this:
>
> static unsigned int features[] = {
> VIRTIO_CONSOLE_F_SIZE,
> VIRTIO_CONSOLE_F_MULTIPORT,
> #if VIRTIO_CONSOLE_HAS_DMA
> VIRTIO_CONSOLE_F_DMA_MEM,
> #endif
> }
>
> If the device then asks for VIRTIO_CONSOLE_F_DMA_MEM
> when DMA is not supported, virtio will do BUG_ON() from
> virtio_check_driver_offered_feature().
>
> Is this acceptable or should we add a check in virtcons_probe()
> and let the probing fail instead?
>
> E.g:
> /* Refuse to bind if F_DMA_MEM request cannot be met */
> if (!VIRTIO_CONSOLE_HAS_DMA &&
> (vdev->config->get_features(vdev) & (1 << VIRTIO_CONSOLE_F_DMA_MEM))){
> dev_err(&vdev->dev,
> "DMA_MEM requested, but arch does not support DMA\n");
> err = -EINVAL;
> goto fail;
> }
>
> Regards,
> Sjur
Failing probe would be cleaner. But there is still a problem:
old driver will happily bind to that device and then
fail to work, right?
virtio pci has revision id for this, but remoteproc doesn't
seem to have anything similar. Or did I miss it? If not -
we probably need to use a different
device id, and not a feature bit.
--
MST
^ permalink raw reply
* Re: [PATCHv2] x86info: dump kvm cpuid's
From: Michael S. Tsirkin @ 2012-09-05 17:33 UTC (permalink / raw)
To: davej; +Cc: xen-devel, kvm, pv-drivers, virtualization, devel
In-Reply-To: <20120430143835.GA10190@redhat.com>
On Mon, Apr 30, 2012 at 05:38:35PM +0300, Michael S. Tsirkin wrote:
> The following makes 'x86info -r' dump hypervisor leaf cpu ids
> (for kvm this is signature+features) when running in a vm.
>
> On the guest we see the signature and the features:
> eax in: 0x40000000, eax = 00000000 ebx = 4b4d564b ecx = 564b4d56 edx = 0000004d
> eax in: 0x40000001, eax = 0100007b ebx = 00000000 ecx = 00000000 edx = 00000000
>
> Hypervisor flag is checked to avoid output changes when not
> running on a VM.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> Changes from v1:
> Make work on non KVM hypervisors (only KVM was tested).
> Avi Kivity said kvm will in the future report
> max HV leaf in eax. For now it reports eax = 0
> so add a work around for that.
Ping.
Davej, any comments?
Would be nice to have this in.
> ---
>
> diff --git a/identify.c b/identify.c
> index 33f35de..a4a3763 100644
> --- a/identify.c
> +++ b/identify.c
> @@ -9,8 +9,8 @@
>
> void get_cpu_info_basics(struct cpudata *cpu)
> {
> - unsigned int maxi, maxei, vendor, address_bits;
> - unsigned int eax;
> + unsigned int maxi, maxei, maxhv, vendor, address_bits;
> + unsigned int eax, ebx, ecx;
>
> cpuid(cpu->number, 0, &maxi, &vendor, NULL, NULL);
> maxi &= 0xffff; /* The high-order word is non-zero on some Cyrix CPUs */
> @@ -19,7 +19,7 @@ void get_cpu_info_basics(struct cpudata *cpu)
> return;
>
> /* Everything that supports cpuid supports these. */
> - cpuid(cpu->number, 1, &eax, NULL, NULL, NULL);
> + cpuid(cpu->number, 1, &eax, &ebx, &ecx, NULL);
> cpu->stepping = eax & 0xf;
> cpu->model = (eax >> 4) & 0xf;
> cpu->family = (eax >> 8) & 0xf;
> @@ -29,6 +29,19 @@ void get_cpu_info_basics(struct cpudata *cpu)
>
> cpuid(cpu->number, 0xC0000000, &maxei, NULL, NULL, NULL);
> cpu->maxei2 = maxei;
> + if (ecx & 0x80000000) {
> + cpuid(cpu->number, 0x40000000, &maxhv, NULL, NULL, NULL);
> + /*
> + * KVM up to linux 3.4 reports 0 as the max hypervisor leaf,
> + * where it really means 0x40000001.
> + * Most (all?) hypervisors have at least one CPUID besides
> + * the vendor ID so assume that.
> + */
> + cpu->maxhv = maxhv ? maxhv : 0x40000001;
> + } else {
> + /* Suppress hypervisor cpuid unless running on a hypervisor */
> + cpu->maxhv = 0;
> + }
>
> cpuid(cpu->number, 0x80000008,&address_bits, NULL, NULL, NULL);
> cpu->phyaddr_bits = address_bits & 0xFF;
> diff --git a/x86info.c b/x86info.c
> index 22c4734..80cae36 100644
> --- a/x86info.c
> +++ b/x86info.c
> @@ -44,6 +44,10 @@ static void display_detailed_info(struct cpudata *cpu)
>
> if (cpu->maxei2 >=0xC0000000)
> dump_raw_cpuid(cpu->number, 0xC0000000, cpu->maxei2);
> +
> + if (cpu->maxhv >= 0x40000000)
> + dump_raw_cpuid(cpu->number, 0x40000000, cpu->maxhv);
> +
> }
>
> if (show_cacheinfo) {
> diff --git a/x86info.h b/x86info.h
> index 7d2a455..c4f5d81 100644
> --- a/x86info.h
> +++ b/x86info.h
> @@ -84,7 +84,7 @@ struct cpudata {
> unsigned int cachesize_trace;
> unsigned int phyaddr_bits;
> unsigned int viraddr_bits;
> - unsigned int cpuid_level, maxei, maxei2;
> + unsigned int cpuid_level, maxei, maxei2, maxhv;
> char name[CPU_NAME_LEN];
> enum connector connector;
> unsigned int flags_ecx;
^ permalink raw reply
* Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation
From: Sjur Brændeland @ 2012-09-05 18:15 UTC (permalink / raw)
To: Michael S. Tsirkin, Rusty Russell, Ohad Ben-Cohen
Cc: Amit Shah, Linus Walleij, Sjur Brændeland, linux-kernel,
virtualization
In-Reply-To: <20120905143558.GB10925@redhat.com>
Hi Michael.
>> If the device then asks for VIRTIO_CONSOLE_F_DMA_MEM
>> when DMA is not supported, virtio will do BUG_ON() from
>> virtio_check_driver_offered_feature().
>>
>> Is this acceptable or should we add a check in virtcons_probe()
>> and let the probing fail instead?
>>
>> E.g:
>> /* Refuse to bind if F_DMA_MEM request cannot be met */
>> if (!VIRTIO_CONSOLE_HAS_DMA &&
>> (vdev->config->get_features(vdev) & (1 << VIRTIO_CONSOLE_F_DMA_MEM))){
>> dev_err(&vdev->dev,
>> "DMA_MEM requested, but arch does not support DMA\n");
>> err = -EINVAL;
>> goto fail;
>> }
>>
>> Regards,
>> Sjur
>
> Failing probe would be cleaner. But there is still a problem:
> old driver will happily bind to that device and then
> fail to work, right?
Not just fail to work, the kernel will panic on the BUG_ON().
Remoteproc gets the virtio configuration from firmware loaded
from user space. So this type of problem might be triggered
for other virtio drivers as well.
> virtio pci has revision id for this, but remoteproc doesn't
> seem to have anything similar. Or did I miss it?
No there are currently no sanity check of
virtio type and feature bits in remoteproc.
One option may be to add this...
> If not -
> we probably need to use a different
> device id, and not a feature bit.
But if I create a new virtio console type, remoteproc
could still call the existing virtio_console with random
bad feature bits, causing kernel panic.
Even if we fix this particular problem, the general problem
still exists: bogus virtio declarations in remoteproc's firmware
may cause BUG_ON(). (Note the fundamental difference
between visualizations and remoteproc. For remoteproc
the virtio configuration comes from binaries loaded from
user space).
So maybe we should look for a more generic solution, e.g.
changing virtio probe functionality so that devices with
bad feature bits will not trigger BUG_ON(), but rather refuse
to bind the driver.
Regards,
Sjur
^ 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