18 Dec, 2012
2 commits
-
Now noone relies on this behavior, we simplify virtqueue_add_buf() so it
return 0 or -errno.Signed-off-by: Rusty Russell
Acked-by: Michael S. Tsirkin -
They're generic concepts, so hoist them. This also avoids accessor
functions (though kept around for merge with DaveM's net tree).This goes even further than Jason Wang's 17bb6d4088 patch
("virtio-ring: move queue_index to vring_virtqueue") which moved the
queue_index from the specific transport.Acked-by: Michael S. Tsirkin
Signed-off-by: Rusty Russell
22 Oct, 2012
1 commit
-
Virtio devices may attempt to add descriptors to a virtqueue from atomic
context using GFP_ATOMIC allocation. This is problematic because such
allocations can fall outside of the lowmem mapping, causing virt_to_phys
to report bogus physical addresses which are subsequently passed to
userspace via the buffers for the virtual device.This patch masks out __GFP_HIGH and __GFP_HIGHMEM from the requested
flags when allocating descriptors for a virtqueue. If an atomic
allocation is requested and later fails, we will return -ENOSPC which
will be handled by the driver.Cc: stable@kernel.org
Cc: Sasha Levin
Signed-off-by: Will Deacon
Signed-off-by: Rusty Russell
28 Sep, 2012
1 commit
-
Instead of storing the queue index in transport-specific virtio structs,
this patch moves them to vring_virtqueue and introduces an helper to get
the value. This lets drivers simplify their management and tracing of
virtqueues.Signed-off-by: Jason Wang
Signed-off-by: Paolo Bonzini
Signed-off-by: Rusty Russell
28 Jan, 2012
2 commits
-
Use virtio_mb() to make sure the available index to be exposed before
checking the the avail event. Otherwise we may get stale value of
avail event in guest and never kick the host after.Note: this fixes a bug introduced by ee7cd8981e15bcb365fc762afe3fc47b8242f630.
Signed-off-by: Jason Wang
Acked-by: Michael S. Tsirkin
Signed-off-by: Rusty Russell
Cc: stable@kernel.org -
Note: this fixes a bug introduced recently in
7b21e34fd1c272e3a8c3846168f2f6287a4cd72b.Signed-off-by: Jason Wang
Acked-by: Michael S. Tsirkin
Signed-off-by: Rusty Russell
12 Jan, 2012
7 commits
-
Under the existing #ifdef DEBUG, check that they don't have more than
1/10 of a second between an add_buf() and a
virtqueue_notify()/virtqueue_kick_prepare() call.We could get false positives on a really busy system, but good for
development.Signed-off-by: Rusty Russell
-
A virtio driver does virtqueue_add_buf() multiple times before finally
calling virtqueue_kick(); previously we only exposed the added buffers
in the virtqueue_kick() call. This means we don't need a memory
barrier in virtqueue_add_buf(), but it reduces concurrency as the
device (ie. host) can't see the buffers until the kick.In the unusual (but now possible) case where a driver does add_buf()
and get_buf() without doing a kick, we do need to insert one before
our counter wraps. Otherwise we could wrap num_added, and later on
not realize that we have passed the marker where we should have
kicked.Signed-off-by: Rusty Russell
-
Since we know vq->vring.num is a power of 2, modulus is lazy (it's asserted
in vring_new_virtqueue()).Signed-off-by: Rusty Russell
-
Based on patch by Christoph for virtio_blk speedup:
Split virtqueue_kick to be able to do the actual notification
outside the lock protecting the virtqueue. This patch was
originally done by Stefan Hajnoczi, but I can't find the
original one anymore and had to recreated it from memory.
Pointers to the original or corrections for the commit message
are welcome.Stefan's patch was here:
https://github.com/stefanha/linux/commit/a6d06644e3a58e57a774e77d7dc34c4a5a2e7496
http://www.spinics.net/lists/linux-virtualization/msg14616.htmlThird time's the charm!
Signed-off-by: Rusty Russell
-
Remove wrapper functions. This makes the allocation type explicit in
all callers; I used GPF_KERNEL where it seemed obvious, left it at
GFP_ATOMIC otherwise.Signed-off-by: Rusty Russell
Reviewed-by: Christoph Hellwig -
The old documentation is left over from when we used a structure with
strategy pointers.And move the documentation to the C file as per kernel practice.
Though I disagree...Signed-off-by: Rusty Russell
Reviewed-by: Christoph Hellwig -
We were cheating with our barriers; using the smp ones rather than the
real device ones. That was fine, until rpmsg came along, which is
used to talk to a real device (a non-SMP CPU).Unfortunately, just putting back the real barriers (reverting
d57ed95d) causes a performance regression on virtio-pci. In
particular, Amos reports netbench's TCP_RR over virtio_net CPU
utilization increased up to 35% while throughput went down by up to
14%.By comparison, this branch is in the noise.
Reference: https://lkml.org/lkml/2011/12/11/22
Signed-off-by: Rusty Russell
01 Nov, 2011
1 commit
-
Up to now, the module.h header was as hard to keep out as
sunlight. But we are cleaning that up. Fix the virtio users
who simply expect module.h to be there in every C file.Signed-off-by: Paul Gortmaker
24 Oct, 2011
1 commit
-
Add support for reporting ring sizes via ethtool -g to the virtio_net
driver.Signed-off-by: Rick Jones
Acked-by: Rusty Russell
Acked-by: Michael S. Tsirkin
Signed-off-by: David S. Miller
30 May, 2011
2 commits
-
Add an API that tells the other side that callbacks
should be delayed until a lot of work has been done.
Implement using the new event_idx feature.Note: it might seem advantageous to let the drivers
ask for a callback after a specific capacity has
been reached. However, as a single head can
free many entries in the descriptor table,
we don't really have a clue about capacity
until get_buf is called. The API is the simplest
to implement at the moment, we'll see what kind of
hints drivers can pass when there's more than one
user of the feature.Signed-off-by: Michael S. Tsirkin
Signed-off-by: Rusty Russell -
Support for the new event idx feature:
1. When enabling interrupts, publish the current avail index
value to the host to get interrupts on the next update.
2. Use the new avail_event feature to reduce the number
of exits from the guest.Simple test with the simulator:
[virtio]# time ./virtio_test
spurious wakeus: 0x7real 0m0.169s
user 0m0.140s
sys 0m0.019s
[virtio]# time ./virtio_test --no-event-idx
spurious wakeus: 0x11real 0m0.649s
user 0m0.295s
sys 0m0.335sSigned-off-by: Michael S. Tsirkin
Signed-off-by: Rusty Russell
21 Apr, 2011
1 commit
-
When detaching a buffer from a vq, the avail.idx value should be
decremented as well.This was noticed by hot-unplugging a virtio console port and then
plugging in a new one on the same number (re-using the vqs which were
just 'disowned'). qemu reported'Guest moved used index from 0 to 256'
when any IO was attempted on the new port.
CC: stable@kernel.org
Reported-by: juzhang
Signed-off-by: Amit Shah
Signed-off-by: Rusty Russell
24 Nov, 2010
1 commit
-
We can't rely on indirect buffers for capacity
calculations because they need a memory allocation
which might fail. In particular, virtio_net can get
into this situation under stress, and it drops packets
and performs badly.So return the number of buffers we can guarantee users.
Signed-off-by: Michael S. Tsirkin
Signed-off-by: Rusty Russell
Reported-By: Krishna Kumar2
26 Jul, 2010
1 commit
-
virtio ring was changed to return an error code on OOM,
but one caller was missed and still checks for vq->vring.num.
The fix is just to check for
Signed-off-by: Michael S. Tsirkin
Signed-off-by: Rusty Russell
Tested-by: Chris Mason
Cc: stable@kernel.org # .34.x
Signed-off-by: Linus Torvalds
23 Jun, 2010
1 commit
-
add_buf returns ring size on out of memory,
this is not what devices expect.Signed-off-by: Michael S. Tsirkin
Acked-by: Amit Shah
Signed-off-by: Rusty Russell
Cc: stable@kernel.org # .34.x
19 May, 2010
2 commits
-
Add an add_buf variant that gets gfp parameter. Use that
to allocate indirect buffers.Signed-off-by: Michael S. Tsirkin
Signed-off-by: Rusty Russell -
We have a single virtqueue_ops implementation,
and it seems unlikely we'll get another one
at this point. So let's remove an unnecessary
level of indirection: it would be very easy to
re-add it if another implementation surfaces.Signed-off-by: Michael S. Tsirkin
Signed-off-by: Rusty Russell
30 Mar, 2010
1 commit
-
…it slab.h inclusion from percpu.h
percpu.h is included by sched.h and module.h and thus ends up being
included when building most .c files. percpu.h includes slab.h which
in turn includes gfp.h making everything defined by the two files
universally available and complicating inclusion dependencies.percpu.h -> slab.h dependency is about to be removed. Prepare for
this change by updating users of gfp and slab facilities include those
headers directly instead of assuming availability. As this conversion
needs to touch large number of source files, the following script is
used as the basis of conversion.http://userweb.kernel.org/~tj/misc/slabh-sweep.py
The script does the followings.
* Scan files for gfp and slab usages and update includes such that
only the necessary includes are there. ie. if only gfp is used,
gfp.h, if slab is used, slab.h.* When the script inserts a new include, it looks at the include
blocks and try to put the new include such that its order conforms
to its surrounding. It's put in the include block which contains
core kernel includes, in the same order that the rest are ordered -
alphabetical, Christmas tree, rev-Xmas-tree or at the end if there
doesn't seem to be any matching order.* If the script can't find a place to put a new include (mostly
because the file doesn't have fitting include block), it prints out
an error message indicating which .h file needs to be added to the
file.The conversion was done in the following steps.
1. The initial automatic conversion of all .c files updated slightly
over 4000 files, deleting around 700 includes and adding ~480 gfp.h
and ~3000 slab.h inclusions. The script emitted errors for ~400
files.2. Each error was manually checked. Some didn't need the inclusion,
some needed manual addition while adding it to implementation .h or
embedding .c file was more appropriate for others. This step added
inclusions to around 150 files.3. The script was run again and the output was compared to the edits
from #2 to make sure no file was left behind.4. Several build tests were done and a couple of problems were fixed.
e.g. lib/decompress_*.c used malloc/free() wrappers around slab
APIs requiring slab.h to be added manually.5. The script was run on all .h files but without automatically
editing them as sprinkling gfp.h and slab.h inclusions around .h
files could easily lead to inclusion dependency hell. Most gfp.h
inclusion directives were ignored as stuff from gfp.h was usually
wildly available and often used in preprocessor macros. Each
slab.h inclusion directive was examined and added manually as
necessary.6. percpu.h was updated not to include slab.h.
7. Build test were done on the following configurations and failures
were fixed. CONFIG_GCOV_KERNEL was turned off for all tests (as my
distributed build env didn't work with gcov compiles) and a few
more options had to be turned off depending on archs to make things
build (like ipr on powerpc/64 which failed due to missing writeq).* x86 and x86_64 UP and SMP allmodconfig and a custom test config.
* powerpc and powerpc64 SMP allmodconfig
* sparc and sparc64 SMP allmodconfig
* ia64 SMP allmodconfig
* s390 SMP allmodconfig
* alpha SMP allmodconfig
* um on x86_64 SMP allmodconfig8. percpu.h modifications were reverted so that it could be applied as
a separate patch and serve as bisection point.Given the fact that I had only a couple of failures from tests on step
6, I'm fairly confident about the coverage of this conversion patch.
If there is a breakage, it's likely to be something in one of the arch
headers which should be easily discoverable easily on most builds of
the specific arch.Signed-off-by: Tejun Heo <tj@kernel.org>
Guess-its-ok-by: Christoph Lameter <cl@linux-foundation.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Lee Schermerhorn <Lee.Schermerhorn@hp.com>
24 Feb, 2010
4 commits
-
vq operations depend on vq->data[i] being NULL to figure out if the vq
entry is in use (since the previous patch).We have to initialize them to NULL to ensure we don't work with junk
data and trigger false BUG_ONs.Signed-off-by: Amit Shah
Signed-off-by: Rusty Russell
Cc: Shirley Ma -
There's currently no way for a virtio driver to ask for unused
buffers, so it has to keep a list itself to reclaim them at shutdown.
This is redundant, since virtio_ring stores that information. So
add a new hook to do this.Signed-off-by: Shirley Ma
Signed-off-by: Amit Shah
Signed-off-by: Rusty Russell -
virtio is communicating with a virtual "device" that actually runs on
another host processor. Thus SMP barriers can be used to control
memory access ordering.Where possible, we should use SMP barriers which are more lightweight than
mandatory barriers, because mandatory barriers also control MMIO effects on
accesses through relaxed memory I/O windows (which virtio does not use)
(compare specifically smp_rmb and rmb on x86_64).We can't just use smp_mb and friends though, because
we must force memory ordering even if guest is UP since host could be
running on another CPU, but SMP barriers are defined to barrier() in
that configuration. So, for UP fall back to mandatory barriers instead.Signed-off-by: Michael S. Tsirkin
Signed-off-by: Rusty Russell -
With DEBUG defined, we add an ->in_use flag to detect if the caller
invokes two virtio methods in parallel. The barriers attempt to ensure
timely update of the ->in_use flag.But they're voodoo: if we need these barriers it implies that the
calling code doesn't have sufficient synchronization to ensure the
code paths aren't invoked at the same time anyway, and we want to
detect it.Also, adding barriers changes timing, so turning on debug has more
chance of hiding real problems.Thanks to MST for drawing my attention to this code...
CC: Michael S. Tsirkin
Signed-off-by: Rusty Russell
29 Oct, 2009
1 commit
-
On SMP guests, reads from the ring might bypass used index reads. This
causes guest crashes because host writes to used index to signal ring
data readiness. Fix this by inserting rmb before used ring reads.Signed-off-by: Michael S. Tsirkin
Signed-off-by: Rusty Russell
Cc: stable@kernel.org
23 Sep, 2009
1 commit
-
This API change means that virtio_net can tell how much capacity
remains for buffers. It's necessarily fuzzy, since
VIRTIO_RING_F_INDIRECT_DESC means we can fit any number of descriptors
in one, *if* we can kmalloc.Signed-off-by: Rusty Russell
Cc: Dinesh Subhraveti
12 Jun, 2009
2 commits
-
Add a new feature flag for indirect ring entries. These are ring
entries which point to a table of buffer descriptors.The idea here is to increase the ring capacity by allowing a larger
effective ring size whereby the ring size dictates the number of
requests that may be outstanding, rather than the size of those
requests.This should be most effective in the case of block I/O where we can
potentially benefit by concurrently dispatching a large number of
large requests. Even in the simple case of single segment block
requests, this results in a threefold increase in ring capacity.Signed-off-by: Mark McLoughlin
Signed-off-by: Rusty Russell -
Add a linked list of all virtqueues for a virtio device: this helps for
debugging and is also needed for upcoming interface change.Also, add a "name" field for clearer debug messages.
Signed-off-by: Rusty Russell
30 Mar, 2009
2 commits
-
Impact: cleanup
Roel Kluin drew attention to these macros with his patch: here I
neaten them a little further:
1) Add a comment on what START_USE and END_USE are checking,
2) Brackets around _vq in BAD_RING,
3) Neaten formatting for START_USE so it's less than 80 cols.Signed-off-by: Rusty Russell
-
Impact: cleanup
fix BAD_RING, START_US and END_USE macros
When these macros aren't called with a variable named vq as first
argument, this would result in a build failure.Signed-off-by: Roel Kluin
Signed-off-by: Rusty Russell
30 Dec, 2008
1 commit
-
This allows each virtio user to hand in the alignment appropriate to
their virtio_ring structures.Signed-off-by: Rusty Russell
Acked-by: Christian Borntraeger
25 Jul, 2008
2 commits
-
To prepare for virtio_ring transport feature bits, hook in a call in
all the users to manipulate them. This currently just clears all the
bits, since it doesn't understand any features.Signed-off-by: Rusty Russell
-
We force notification when the ring is full, even if the host has
indicated it doesn't want to know. This seemed like a good idea at
the time: if we fill the transmit ring, we should tell the host
immediately.Unfortunately this logic also applies to the receiving ring, which is
refilled constantly. We should introduce real notification thesholds
to replace this logic. Meanwhile, removing the logic altogether breaks
the heuristics which KVM uses, so we use a hack: only notify if there are
outgoing parts of the new buffer.Here are the number of exits with lguest's crappy network implementation:
Before:
network xmit 7859051 recv 236420
After:
network xmit 7858610 recv 118136Signed-off-by: Rusty Russell
30 May, 2008
2 commits
-
virtio allows drivers to suppress callbacks (ie. interrupts) for
efficiency (no locking, it's just an optimization).There's a similar mechanism for the host to suppress notifications
coming from the guest: in that case, we ignore the suppression if the
ring is completely full.It turns out that life is simpler if the host similarly ignores
callback suppression when the ring is completely empty: the network
driver wants to free up old packets in a timely manner, and otherwise
has to use a timer to poll.We have to remove the code which ignores interrupts when the driver
has disabled them (again, it had no locking and hence was unreliable
anyway).Signed-off-by: Rusty Russell
-
Hello Rusty,
seems that we still have a problem with virtio_net and the enable_cb callback.
During a long running network stress tests with virtio and got the following
oops:------------[ cut here ]------------
kernel BUG at drivers/virtio/virtio_ring.c:230!
illegal operation: 0001 [#1] SMP
Modules linked in:
CPU: 0 Not tainted 2.6.26-rc2-kvm-00436-gc94c08b-dirty #34
Process netserver (pid: 2582, task: 000000000fbc4c68, ksp: 000000000f42b990)
Krnl PSW : 0704c00180000000 00000000002d0ec8 (vring_enable_cb+0x1c/0x60)
R:0 T:1 IO:1 EX:1 Key:0 M:1 W:0 P:0 AS:3 CC:0 PM:0 EA:3
Krnl GPRS: 0000000000000000 0000000000000000 000000000ef3d000 0000000010009800
0000000000000000 0000000000419ce0 0000000000000080 000000000000007b
000000000adb5538 000000000ef40900 000000000ef40000 000000000ef40920
0000000000000000 0000000000000005 000000000029c1b0 000000000fea7d18
Krnl Code: 00000000002d0ebc: a7110001 tmll %r1,1
00000000002d0ec0: a7740004 brc 7,2d0ec8
00000000002d0ec4: a7f40001 brc 15,2d0ec6
>00000000002d0ec8: a517fffe nill %r1,65534
00000000002d0ecc: 40103000 sth %r1,0(%r3)
00000000002d0ed0: 07f0 bcr 15,%r0
00000000002d0ed2: e31020380004 lg %r1,56(%r2)
00000000002d0ed8: a7480000 lhi %r4,0
Call Trace:
([] virtnet_poll+0x290/0x3b8)
[] net_rx_action+0x9c/0x1b8
[] __do_softirq+0x74/0x108
[] do_softirq+0x92/0xac
[] irq_exit+0x72/0xc8
[] do_extint+0xe2/0x104
[] ext_no_vtime+0x16/0x1a
Last Breaking-Event-Address:
[] vring_enable_cb+0x18/0x60I looked into the virtio_net code for some time and I think the following
scenario happened. Please look at virtnet_poll:
[...]
/* Out of packets? */
if (received < budget) {
netif_rx_complete(vi->dev, napi);
if (unlikely(!vi->rvq->vq_ops->enable_cb(vi->rvq))
&& napi_schedule_prep(napi)) {
vi->rvq->vq_ops->disable_cb(vi->rvq);
__netif_rx_schedule(vi->dev, napi);
goto again;
}
}If an interrupt arrives after netif_rx_complete, a second poll routine can run
on a different cpu. The second check for napi_schedule_prep would prevent any
harm in the network stack, but we have called enable_cb possibly after the
disable_cb in skb_recv_done.static void skb_recv_done(struct virtqueue *rvq)
{
struct virtnet_info *vi = rvq->vdev->priv;
/* Schedule NAPI, Suppress further interrupts if successful. */
if (netif_rx_schedule_prep(vi->dev, &vi->napi)) {
rvq->vq_ops->disable_cb(rvq);
__netif_rx_schedule(vi->dev, &vi->napi);
}
}That means that the second poll routine runs with interrupts enabled, which is
ok, since we can handle additional interrupts. The problem is now that the
second poll routine might also call enable_cb, triggering the BUG.The only solution I can come up with, is to remove the BUG statement in
enable_cb - similar to disable_cb. Opinions or better ideas where the oops
could come from?Signed-off-by: Christian Borntraeger
Signed-off-by: Rusty Russell
02 May, 2008
1 commit
-
A corrupt virtqueue (caused by the other end screwing up) can have
strange results such as a driver spinning: just bail when we try to
get a buffer from a known-broken queue.Signed-off-by: Rusty Russell