From 2ad3cfbac58d0a6c6e65aafd9e0e757ca3d35292 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Fri, 30 May 2008 15:09:41 -0500 Subject: virtio: bus_id for devices should contain 'virtio' Chris Lalancette points out that virtio.c sets all device names to '0', '1', etc, which looks silly in /proc/interrupts. We change this from '%d' to 'virtio%d'. Signed-off-by: Rusty Russell Cc: Christian Borntraeger Cc: Martin Schwidefsky Cc: Carsten Otte Cc: Heiko Carstens Cc: Chris Lalancette Cc: Anthony Liguori --- drivers/virtio/virtio.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'drivers/virtio') diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c index 13866789b356..59918dfc3cb7 100644 --- a/drivers/virtio/virtio.c +++ b/drivers/virtio/virtio.c @@ -166,7 +166,7 @@ int register_virtio_device(struct virtio_device *dev) int err; dev->dev.bus = &virtio_bus; - sprintf(dev->dev.bus_id, "%u", dev->index); + sprintf(dev->dev.bus_id, "virtio%u", dev->index); /* We always start by resetting the device, in case a previous * driver messed it up. This also tests that code path a little. */ -- cgit v1.2.3 From 5610bd1524332fe7d651eb56cc780e32763a2ac3 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Fri, 30 May 2008 15:09:42 -0500 Subject: virtio: virtio_pci should not set bus_id. The common virtio code sets the bus_id, overriding anything virtio_pci sets anyway. Signed-off-by: Rusty Russell Cc: Christian Borntraeger Cc: Martin Schwidefsky Cc: Carsten Otte Cc: Heiko Carstens Cc: Chris Lalancette Cc: Anthony Liguori --- drivers/virtio/virtio_pci.c | 1 - 1 file changed, 1 deletion(-) (limited to 'drivers/virtio') diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c index 27e9fc9117cd..2913c2f309f0 100644 --- a/drivers/virtio/virtio_pci.c +++ b/drivers/virtio/virtio_pci.c @@ -325,7 +325,6 @@ static int __devinit virtio_pci_probe(struct pci_dev *pci_dev, if (vp_dev == NULL) return -ENOMEM; - snprintf(vp_dev->vdev.dev.bus_id, BUS_ID_SIZE, "virtio%d", dev_index); vp_dev->vdev.index = dev_index; dev_index++; -- cgit v1.2.3 From b769f579081943f14e0ff03b7b0bd3a11cf14625 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Fri, 30 May 2008 15:09:42 -0500 Subject: virtio: set device index in common code. Anthony Liguori points out that three different transports use the virtio code, but each one keeps its own counter to set the virtio_device's index field. In theory (though not in current practice) this means that names could be duplicated, and that risk grows as more transports are created. So we move the selection of the unique virtio_device.index into the common code in virtio.c, which has the side-benefit of removing duplicate code. The only complexity is that lguest and S/390 use the index to uniquely identify the device in case of catastrophic failure before register_virtio_device() is called: now we use the offset within the descriptor page as a unique identifier for the printks. Signed-off-by: Rusty Russell Cc: Christian Borntraeger Cc: Martin Schwidefsky Cc: Carsten Otte Cc: Heiko Carstens Cc: Chris Lalancette Cc: Anthony Liguori --- drivers/virtio/virtio.c | 6 ++++++ drivers/virtio/virtio_pci.c | 6 ------ 2 files changed, 6 insertions(+), 6 deletions(-) (limited to 'drivers/virtio') diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c index 59918dfc3cb7..0f3c2bb7bf35 100644 --- a/drivers/virtio/virtio.c +++ b/drivers/virtio/virtio.c @@ -2,6 +2,9 @@ #include #include +/* Unique numbering for virtio devices. */ +static unsigned int dev_index; + static ssize_t device_show(struct device *_d, struct device_attribute *attr, char *buf) { @@ -166,6 +169,9 @@ int register_virtio_device(struct virtio_device *dev) int err; dev->dev.bus = &virtio_bus; + + /* Assign a unique device index and hence name. */ + dev->index = dev_index++; sprintf(dev->dev.bus_id, "virtio%u", dev->index); /* We always start by resetting the device, in case a previous diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c index 2913c2f309f0..eae7236310e4 100644 --- a/drivers/virtio/virtio_pci.c +++ b/drivers/virtio/virtio_pci.c @@ -78,9 +78,6 @@ static struct device virtio_pci_root = { .bus_id = "virtio-pci", }; -/* Unique numbering for devices under the kvm root */ -static unsigned int dev_index; - /* Convert a generic virtio device to our structure */ static struct virtio_pci_device *to_vp_device(struct virtio_device *vdev) { @@ -325,9 +322,6 @@ static int __devinit virtio_pci_probe(struct pci_dev *pci_dev, if (vp_dev == NULL) return -ENOMEM; - vp_dev->vdev.index = dev_index; - dev_index++; - vp_dev->vdev.dev.parent = &virtio_pci_root; vp_dev->vdev.config = &virtio_pci_config_ops; vp_dev->pci_dev = pci_dev; -- cgit v1.2.3 From 52a3a05f3ab82655ffa4c9bf6835565c98a3c2e5 Mon Sep 17 00:00:00 2001 From: Christian Borntraeger Date: Mon, 26 May 2008 11:29:27 +0200 Subject: virtio_net: another race with virtio_net and enable_cb 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: ([<000000000029c0fc>] virtnet_poll+0x290/0x3b8) [<0000000000333fb8>] net_rx_action+0x9c/0x1b8 [<00000000001394bc>] __do_softirq+0x74/0x108 [<000000000010d16a>] do_softirq+0x92/0xac [<0000000000139826>] irq_exit+0x72/0xc8 [<000000000010a7b6>] do_extint+0xe2/0x104 [<0000000000110508>] ext_no_vtime+0x16/0x1a Last Breaking-Event-Address: [<00000000002d0ec4>] vring_enable_cb+0x18/0x60 I 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 --- drivers/virtio/virtio_ring.c | 1 - 1 file changed, 1 deletion(-) (limited to 'drivers/virtio') diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c index 937a49d6772c..96d2567a7df8 100644 --- a/drivers/virtio/virtio_ring.c +++ b/drivers/virtio/virtio_ring.c @@ -227,7 +227,6 @@ static bool vring_enable_cb(struct virtqueue *_vq) struct vring_virtqueue *vq = to_vvq(_vq); START_USE(vq); - BUG_ON(!(vq->vring.avail->flags & VRING_AVAIL_F_NO_INTERRUPT)); /* We optimistically turn back on interrupts, then check if there was * more to do. */ -- cgit v1.2.3 From b4f68be6c5d507afdcd74f5be3df0b1209cda503 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Fri, 30 May 2008 15:09:45 -0500 Subject: virtio: force callback on empty. 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 --- drivers/virtio/virtio_ring.c | 7 ------- 1 file changed, 7 deletions(-) (limited to 'drivers/virtio') diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c index 96d2567a7df8..72bf8bc09014 100644 --- a/drivers/virtio/virtio_ring.c +++ b/drivers/virtio/virtio_ring.c @@ -253,13 +253,6 @@ irqreturn_t vring_interrupt(int irq, void *_vq) if (unlikely(vq->broken)) return IRQ_HANDLED; - /* Other side may have missed us turning off the interrupt, - * but we should preserve disable semantic for virtio users. */ - if (unlikely(vq->vring.avail->flags & VRING_AVAIL_F_NO_INTERRUPT)) { - pr_debug("virtqueue interrupt after disable for %p\n", vq); - return IRQ_HANDLED; - } - pr_debug("virtqueue callback for %p (%p)\n", vq, vq->vq.callback); if (vq->vq.callback) vq->vq.callback(&vq->vq); -- cgit v1.2.3 From b92dea67cc66970cda6b5b11895d08e35b4618e7 Mon Sep 17 00:00:00 2001 From: Mark McLoughlin Date: Sun, 15 Jun 2008 23:20:50 +1000 Subject: virtio: Complete feature negotation before updating status lguest (in rusty's use-tun-ringfd patch) assumes that the guest has updated its feature bits before setting its status to VIRTIO_CONFIG_S_DRIVER_OK. That's pretty reasonable, so let's make it so. Signed-off-by: Mark McLoughlin Signed-off-by: Rusty Russell Signed-off-by: Linus Torvalds --- drivers/virtio/virtio.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'drivers/virtio') diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c index 0f3c2bb7bf35..7084e7e146c0 100644 --- a/drivers/virtio/virtio.c +++ b/drivers/virtio/virtio.c @@ -124,9 +124,9 @@ static int virtio_dev_probe(struct device *_d) if (err) add_status(dev, VIRTIO_CONFIG_S_FAILED); else { - add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK); /* They should never have set feature bits beyond 32 */ dev->config->set_features(dev, dev->features[0]); + add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK); } return err; } -- cgit v1.2.3