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
08 Apr, 2008
1 commit
-
The 'disable_cb' callback is designed as an optimization to tell the host
we don't need callbacks now. As it is not reliable, the debug check is
overzealous: it can happen on two CPUs at the same time. Document this.Even if it were reliable, the virtio_net driver doesn't disable
callbacks on transmit so the START_USE/END_USE debugging reentrance
protection can be easily tripped even on UP.Thanks to Balaji Rao for the bug report and testing.
Signed-off-by: Rusty Russell
CC: Balaji Rao
Signed-off-by: Linus Torvalds
17 Mar, 2008
1 commit
-
There is a race in virtio_net, dealing with disabling/enabling the callback.
I saw the following oops:kernel BUG at /space/kvm/drivers/virtio/virtio_ring.c:218!
illegal operation: 0001 [#1] SMP
Modules linked in: sunrpc dm_mod
CPU: 2 Not tainted 2.6.25-rc1zlive-host-10623-gd358142-dirty #99
Process swapper (pid: 0, task: 000000000f85a610, ksp: 000000000f873c60)
Krnl PSW : 0404300180000000 00000000002b81a6 (vring_disable_cb+0x16/0x20)
R:0 T:1 IO:0 EX:0 Key:0 M:1 W:0 P:0 AS:0 CC:3 PM:0 EA:3
Krnl GPRS: 0000000000000001 0000000000000001 0000000010005800 0000000000000001
000000000f3a0900 000000000f85a610 0000000000000000 0000000000000000
0000000000000000 000000000f870000 0000000000000000 0000000000001237
000000000f3a0920 000000000010ff74 00000000002846f6 000000000fa0bcd8
Krnl Code: 00000000002b819a: a7110001 tmll %r1,1
00000000002b819e: a7840004 brc 8,2b81a6
00000000002b81a2: a7f40001 brc 15,2b81a4
>00000000002b81a6: a51b0001 oill %r1,1
00000000002b81aa: 40102000 sth %r1,0(%r2)
00000000002b81ae: 07fe bcr 15,%r14
00000000002b81b0: eb7ff0380024 stmg %r7,%r15,56(%r15)
00000000002b81b6: a7f13e00 tmll %r15,15872
Call Trace:
([] 0xfa0bcd0)
[] vring_interrupt+0x5c/0x6c
[] do_extint+0xb8/0xf0
[] ext_no_vtime+0x16/0x1a
[] cpu_idle+0x1c2/0x1e0The problem can be triggered with a high amount of host->guest traffic.
I think its the following race:poll says netif_rx_complete
poll calls enable_cb
enable_cb opens the interrupt mask
a new packet comes, an interrupt is triggered----\
enable_cb sees that there is more work |
enable_cb disables the interrupt |
. V
. interrupt is delivered
. skb_recv_done does atomic napi test, ok
some waiting disable_cb is called->check fails->bang!
.
poll would do napi check
poll would do disable_cbThe fix is to let enable_cb not disable the interrupt again, but expect the
caller to do the cleanup if it returns false. In that case, the interrupt is
only disabled, if the napi test_set_bit was successful.Signed-off-by: Christian Borntraeger
Signed-off-by: Rusty Russell (cleaned up doco)
04 Feb, 2008
4 commits
-
This is needed for the virtio PCI device to be compiled as a module.
Signed-off-by: Anthony Liguori
Signed-off-by: Rusty Russell -
Simple cleanup.
Signed-off-by: Rusty Russell
-
Anthony Liguori found double interrupt suppression in the virtio_net
driver, triggered by two skb_recv_done's in a row. This is because
virtio_ring's interrupt suppression is a best-effort optimization: it
contains no synchronization so the host can miss it and still send
interrupts.But it's certainly nicer for virtio users if calling disable_cb
actually disables callbacks, so we check for the race in the interrupt
routine.Note: SMP guests might require syncronization here, but since
disable_cb is actually called from interrupt context, there has to be
some form of synchronization before the next same interrupt handler is
called (Linux guarantees that the same device's irq handler will never
run simultanously on multiple CPUs).Signed-off-by: Rusty Russell
-
A reset function solves three problems:
1) It allows us to renegotiate features, eg. if we want to upgrade a
guest driver without rebooting the guest.2) It gives us a clean way of shutting down virtqueues: after a reset,
we know that the buffers won't be used by the host, and3) It helps the guest recover from messed-up drivers.
So we remove the ->shutdown hook, and the only way we now remove
feature bits is via reset.We leave it to the driver to do the reset before it deletes queues:
the balloon driver, for example, needs to chat to the host in its
remove function.Signed-off-by: Rusty Russell