* potential race in virtio ring?
From: Michael S. Tsirkin @ 2010-06-14 13:59 UTC (permalink / raw)
To: virtualization, Rusty Russell, Jiri Pirko, Shirley Ma, netdev
Hi!
I was going over the vring code and noticed, that
the ring has this check:
irqreturn_t vring_interrupt(int irq, void *_vq)
{
struct vring_virtqueue *vq = to_vvq(_vq);
if (!more_used(vq)) {
pr_debug("virtqueue interrupt with no work for %p\n", vq);
return IRQ_NONE;
static inline bool more_used(const struct vring_virtqueue *vq)
{
return vq->last_used_idx != vq->vring.used->idx;
}
My concern is that with virtio net, more_used is called
on a CPU different from the one that polls the vq.
This might mean that last_used_idx value might be stale.
Could this lead to a missed interrupt?
Thanks,
--
MST
^ permalink raw reply
* Re: [PATCH for-2.6.35] virtio_net: fix oom handling on tx
From: Michael S. Tsirkin @ 2010-06-10 19:03 UTC (permalink / raw)
To: Stephen Hemminger
Cc: Shirley Ma, Jiri Pirko, netdev, linux-kernel, virtualization,
Sridhar Samudrala
In-Reply-To: <20100610104653.1aed2ecc@nehalam>
On Thu, Jun 10, 2010 at 10:46:53AM -0700, Stephen Hemminger wrote:
> On Thu, 10 Jun 2010 10:17:07 -0700
> Sridhar Samudrala <sri@us.ibm.com> wrote:
>
> > On Thu, 2010-06-10 at 18:20 +0300, Michael S. Tsirkin wrote:
> > > virtio net will never try to overflow the TX ring, so the only reason
> > > add_buf may fail is out of memory. Thus, we can not stop the
> > > device until some request completes - there's no guarantee anything
> > > at all is outstanding.
> > >
> > > Make the error message clearer as well: error here does not
> > > indicate queue full.
> > >
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > ---
> > > drivers/net/virtio_net.c | 15 ++++++++-------
> > > 1 files changed, 8 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > index 85615a3..e48a06f 100644
> > > --- a/drivers/net/virtio_net.c
> > > +++ b/drivers/net/virtio_net.c
> > > @@ -563,7 +563,6 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
> > > struct virtnet_info *vi = netdev_priv(dev);
> > > int capacity;
> > >
> > > -again:
> > > /* Free up any pending old buffers before queueing new ones. */
> > > free_old_xmit_skbs(vi);
> > >
> > > @@ -572,12 +571,14 @@ again:
> > >
> > > /* This can happen with OOM and indirect buffers. */
> > > if (unlikely(capacity < 0)) {
> > > - netif_stop_queue(dev);
> > > - dev_warn(&dev->dev, "Unexpected full queue\n");
> > > - if (unlikely(!virtqueue_enable_cb(vi->svq))) {
> > > - virtqueue_disable_cb(vi->svq);
> > > - netif_start_queue(dev);
> > > - goto again;
> > > + if (net_ratelimit()) {
> > > + if (likely(capacity == -ENOMEM))
> > > + dev_warn(&dev->dev,
> > > + "TX queue failure: out of memory\n");
> > > + else
> > > + dev_warn(&dev->dev,
> > > + "Unexpected TX queue failure: %d\n",
> > > + capacity);
> > > }
> > > return NETDEV_TX_BUSY;
> > > }
> >
> > It is not clear to me how xmit_skb() can return -ENOMEM.
> > xmit_skb() calls virtqueue_add_buf_gfp() which can return -ENOSPC.
> > Even vring_add_indirect() doesn't return -ENOMEM on kmalloc failure.
>
> It makes more sense to have the device increment tx_droppped,
> and return NETDEV_TX_OK. Skip the message (or make it a pr_debug()).
> Network devices do not guarantee packet delivery, and if out of
> resources then holding more data in the
> queue is going to hurt not help the situation.
>
> --
Well, I only keep the existing behaviour around. The changes you propose
would be 2.6.36 material.
I have it on my todo list to look for a way to test performance under
GFP_ATOMIC failure scenario. Any suggestions?
--
MST
^ permalink raw reply
* Re: [PATCH for-2.6.35] virtio_net: fix oom handling on tx
From: Michael S. Tsirkin @ 2010-06-10 18:57 UTC (permalink / raw)
To: Sridhar Samudrala
Cc: Shirley Ma, Jiri Pirko, netdev, linux-kernel, virtualization
In-Reply-To: <1276190227.22064.19.camel@w-sridhar.beaverton.ibm.com>
On Thu, Jun 10, 2010 at 10:17:07AM -0700, Sridhar Samudrala wrote:
> On Thu, 2010-06-10 at 18:20 +0300, Michael S. Tsirkin wrote:
> > virtio net will never try to overflow the TX ring, so the only reason
> > add_buf may fail is out of memory. Thus, we can not stop the
> > device until some request completes - there's no guarantee anything
> > at all is outstanding.
> >
> > Make the error message clearer as well: error here does not
> > indicate queue full.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> > drivers/net/virtio_net.c | 15 ++++++++-------
> > 1 files changed, 8 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index 85615a3..e48a06f 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -563,7 +563,6 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
> > struct virtnet_info *vi = netdev_priv(dev);
> > int capacity;
> >
> > -again:
> > /* Free up any pending old buffers before queueing new ones. */
> > free_old_xmit_skbs(vi);
> >
> > @@ -572,12 +571,14 @@ again:
> >
> > /* This can happen with OOM and indirect buffers. */
> > if (unlikely(capacity < 0)) {
> > - netif_stop_queue(dev);
> > - dev_warn(&dev->dev, "Unexpected full queue\n");
> > - if (unlikely(!virtqueue_enable_cb(vi->svq))) {
> > - virtqueue_disable_cb(vi->svq);
> > - netif_start_queue(dev);
> > - goto again;
> > + if (net_ratelimit()) {
> > + if (likely(capacity == -ENOMEM))
> > + dev_warn(&dev->dev,
> > + "TX queue failure: out of memory\n");
> > + else
> > + dev_warn(&dev->dev,
> > + "Unexpected TX queue failure: %d\n",
> > + capacity);
> > }
> > return NETDEV_TX_BUSY;
> > }
>
> It is not clear to me how xmit_skb() can return -ENOMEM.
> xmit_skb() calls virtqueue_add_buf_gfp() which can return -ENOSPC.
> Even vring_add_indirect() doesn't return -ENOMEM on kmalloc failure.
>
> Thanks
> Sridhar
A separate patch fixes vring_add_indirect to return -ENOMEM.
-ENOSPC really means ring is full so nothing to do
and no need to retry.
--
MST
^ permalink raw reply
* Re: [PATCH for-2.6.35] virtio_net: fix oom handling on tx
From: Stephen Hemminger @ 2010-06-10 17:46 UTC (permalink / raw)
To: Sridhar Samudrala
Cc: Shirley Ma, Jiri Pirko, netdev, linux-kernel, virtualization,
Michael S. Tsirkin
In-Reply-To: <1276190227.22064.19.camel@w-sridhar.beaverton.ibm.com>
On Thu, 10 Jun 2010 10:17:07 -0700
Sridhar Samudrala <sri@us.ibm.com> wrote:
> On Thu, 2010-06-10 at 18:20 +0300, Michael S. Tsirkin wrote:
> > virtio net will never try to overflow the TX ring, so the only reason
> > add_buf may fail is out of memory. Thus, we can not stop the
> > device until some request completes - there's no guarantee anything
> > at all is outstanding.
> >
> > Make the error message clearer as well: error here does not
> > indicate queue full.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> > drivers/net/virtio_net.c | 15 ++++++++-------
> > 1 files changed, 8 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index 85615a3..e48a06f 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -563,7 +563,6 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
> > struct virtnet_info *vi = netdev_priv(dev);
> > int capacity;
> >
> > -again:
> > /* Free up any pending old buffers before queueing new ones. */
> > free_old_xmit_skbs(vi);
> >
> > @@ -572,12 +571,14 @@ again:
> >
> > /* This can happen with OOM and indirect buffers. */
> > if (unlikely(capacity < 0)) {
> > - netif_stop_queue(dev);
> > - dev_warn(&dev->dev, "Unexpected full queue\n");
> > - if (unlikely(!virtqueue_enable_cb(vi->svq))) {
> > - virtqueue_disable_cb(vi->svq);
> > - netif_start_queue(dev);
> > - goto again;
> > + if (net_ratelimit()) {
> > + if (likely(capacity == -ENOMEM))
> > + dev_warn(&dev->dev,
> > + "TX queue failure: out of memory\n");
> > + else
> > + dev_warn(&dev->dev,
> > + "Unexpected TX queue failure: %d\n",
> > + capacity);
> > }
> > return NETDEV_TX_BUSY;
> > }
>
> It is not clear to me how xmit_skb() can return -ENOMEM.
> xmit_skb() calls virtqueue_add_buf_gfp() which can return -ENOSPC.
> Even vring_add_indirect() doesn't return -ENOMEM on kmalloc failure.
It makes more sense to have the device increment tx_droppped,
and return NETDEV_TX_OK. Skip the message (or make it a pr_debug()).
Network devices do not guarantee packet delivery, and if out of
resources then holding more data in the
queue is going to hurt not help the situation.
--
^ permalink raw reply
* Re: [PATCH for-2.6.35] virtio_net: fix oom handling on tx
From: Sridhar Samudrala @ 2010-06-10 17:17 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Shirley Ma, Jiri Pirko, netdev, linux-kernel, virtualization
In-Reply-To: <20100610152041.GA3480@redhat.com>
On Thu, 2010-06-10 at 18:20 +0300, Michael S. Tsirkin wrote:
> virtio net will never try to overflow the TX ring, so the only reason
> add_buf may fail is out of memory. Thus, we can not stop the
> device until some request completes - there's no guarantee anything
> at all is outstanding.
>
> Make the error message clearer as well: error here does not
> indicate queue full.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> drivers/net/virtio_net.c | 15 ++++++++-------
> 1 files changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 85615a3..e48a06f 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -563,7 +563,6 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
> struct virtnet_info *vi = netdev_priv(dev);
> int capacity;
>
> -again:
> /* Free up any pending old buffers before queueing new ones. */
> free_old_xmit_skbs(vi);
>
> @@ -572,12 +571,14 @@ again:
>
> /* This can happen with OOM and indirect buffers. */
> if (unlikely(capacity < 0)) {
> - netif_stop_queue(dev);
> - dev_warn(&dev->dev, "Unexpected full queue\n");
> - if (unlikely(!virtqueue_enable_cb(vi->svq))) {
> - virtqueue_disable_cb(vi->svq);
> - netif_start_queue(dev);
> - goto again;
> + if (net_ratelimit()) {
> + if (likely(capacity == -ENOMEM))
> + dev_warn(&dev->dev,
> + "TX queue failure: out of memory\n");
> + else
> + dev_warn(&dev->dev,
> + "Unexpected TX queue failure: %d\n",
> + capacity);
> }
> return NETDEV_TX_BUSY;
> }
It is not clear to me how xmit_skb() can return -ENOMEM.
xmit_skb() calls virtqueue_add_buf_gfp() which can return -ENOSPC.
Even vring_add_indirect() doesn't return -ENOMEM on kmalloc failure.
Thanks
Sridhar
^ permalink raw reply
* Re: [PATCH] virtio_net: indicate oom when addbuf returns failure
From: Bruce Rogers @ 2010-06-10 15:46 UTC (permalink / raw)
To: Michael S. Tsirkin, Rusty Russell
Cc: Herbert Xu, netdev, stable, virtualization
In-Reply-To: <20100606201258.GA21196@redhat.com>
>>> On 6/6/2010 at 02:13 PM, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Fri, Jun 04, 2010 at 10:28:56AM +0930, Rusty Russell wrote:
>> This patch is a subset of an already upstream patch, but this portion
>> is useful in earlier releases.
>>
>> Please consider for the 2.6.32 and 2.6.33 stable trees.
>>
>> If the add_buf operation fails, indicate failure to the caller.
>>
>> Signed-off-by: Bruce Rogers <brogers@novell.com>
>> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
>
> Actually this code looks strange:
> Note that add_buf inicates out of memory
> condition with a positive return value, and ring full
> (which is not an error!) with -ENOSPC.
>
> So it seems that this patch (and upstream code) will fill
> the ring and then end up setting oom = true and rescheduling the work
> forever. And I suspect I actually saw this at some point
> on one of my systems: observed BW would drop
> with high CPU usage until reboot.
> Can't reproduce it now anymore ..
>
Thanks for looking into this.
We've decided not to use this patch, since it is not a part of the
solution we need. The upstream patch from whence it came at first glance seemed useful, but is problematic as you point out.
We've retested without that patch and are still getting good results.
Bruce
^ permalink raw reply
* Re: [PATCH for-2.6.35] virtio-pci: disable msi at startup
From: Jesse Barnes @ 2010-06-10 15:34 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Anthony Liguori, linux-pci, Matt Carlson, virtualization,
Rafael J. Wysocki, Bjorn, Randy, Kenji Kaneshige, Tejun Heo,
David S. Miller, linux-kernel, Helgaas
In-Reply-To: <20100610152252.GA3510@redhat.com>
On Thu, 10 Jun 2010 18:22:52 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> virtio-pci resets the device at startup by writing to the status
> register, but this does not clear the pci config space,
> specifically msi enable status which affects register
> layout.
>
> This breaks things like kdump when they try to use e.g. virtio-blk.
>
> Fix by forcing msi off at startup. Since pci.c already has
> a routine to do this, we export and use it instead of duplicating code.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> Tested-by: Vivek Goyal <vgoyal@redhat.com>
> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> Cc: linux-pci@vger.kernel.org
> ---
Yeah, looks fine.
Acked-by: Jesse Barnes <jbarnes@virtuousgeek.org>
--
Jesse Barnes, Intel Open Source Technology Center
^ permalink raw reply
* [PATCH for-2.6.35] virtio-pci: disable msi at startup
From: Michael S. Tsirkin @ 2010-06-10 15:22 UTC (permalink / raw)
To: virtualization
Cc: Anthony Liguori, Michael S. Tsirkin, linux-pci, Matt Carlson,
linux-kernel, Jesse Barnes, Rafael J. Wysocki, Kenji Kaneshige,
Tejun Heo, David S. Miller, Bjorn Helgaas
virtio-pci resets the device at startup by writing to the status
register, but this does not clear the pci config space,
specifically msi enable status which affects register
layout.
This breaks things like kdump when they try to use e.g. virtio-blk.
Fix by forcing msi off at startup. Since pci.c already has
a routine to do this, we export and use it instead of duplicating code.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Tested-by: Vivek Goyal <vgoyal@redhat.com>
Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
Cc: linux-pci@vger.kernel.org
---
Jesse, could you ack adding the pci export please?
Rusty, please consider this patch for 2.6.35.
drivers/pci/pci.c | 1 +
drivers/virtio/virtio_pci.c | 3 +++
include/linux/pci.h | 4 ++++
3 files changed, 8 insertions(+), 0 deletions(-)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 60f30e7..740fb4e 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -2292,6 +2292,7 @@ void pci_msi_off(struct pci_dev *dev)
pci_write_config_word(dev, pos + PCI_MSIX_FLAGS, control);
}
}
+EXPORT_SYMBOL_GPL(pci_msi_off);
#ifndef HAVE_ARCH_PCI_SET_DMA_MAX_SEGMENT_SIZE
int pci_set_dma_max_seg_size(struct pci_dev *dev, unsigned int size)
diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
index 95896f3..ef8d9d5 100644
--- a/drivers/virtio/virtio_pci.c
+++ b/drivers/virtio/virtio_pci.c
@@ -636,6 +636,9 @@ static int __devinit virtio_pci_probe(struct pci_dev *pci_dev,
INIT_LIST_HEAD(&vp_dev->virtqueues);
spin_lock_init(&vp_dev->lock);
+ /* Disable MSI/MSIX to bring device to a known good state. */
+ pci_msi_off(pci_dev);
+
/* enable the device */
err = pci_enable_device(pci_dev);
if (err)
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 7cb0084..31d8a12 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -780,7 +780,11 @@ int __must_check pci_set_mwi(struct pci_dev *dev);
int pci_try_set_mwi(struct pci_dev *dev);
void pci_clear_mwi(struct pci_dev *dev);
void pci_intx(struct pci_dev *dev, int enable);
+#ifdef CONFIG_PCI_MSI
void pci_msi_off(struct pci_dev *dev);
+#else
+static inline void pci_msi_off(struct pci_dev *dev) {}
+#endif
int pci_set_dma_max_seg_size(struct pci_dev *dev, unsigned int size);
int pci_set_dma_seg_boundary(struct pci_dev *dev, unsigned long mask);
int pcix_get_max_mmrbc(struct pci_dev *dev);
--
1.7.1.12.g42b7f
^ permalink raw reply related
* [PATCH for-2.6.35] virtio_net: fix oom handling on tx
From: Michael S. Tsirkin @ 2010-06-10 15:20 UTC (permalink / raw)
To: virtualization, Rusty Russell, Michael S. Tsirkin, Jiri Pirko,
Shirley
virtio net will never try to overflow the TX ring, so the only reason
add_buf may fail is out of memory. Thus, we can not stop the
device until some request completes - there's no guarantee anything
at all is outstanding.
Make the error message clearer as well: error here does not
indicate queue full.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
drivers/net/virtio_net.c | 15 ++++++++-------
1 files changed, 8 insertions(+), 7 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 85615a3..e48a06f 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -563,7 +563,6 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
struct virtnet_info *vi = netdev_priv(dev);
int capacity;
-again:
/* Free up any pending old buffers before queueing new ones. */
free_old_xmit_skbs(vi);
@@ -572,12 +571,14 @@ again:
/* This can happen with OOM and indirect buffers. */
if (unlikely(capacity < 0)) {
- netif_stop_queue(dev);
- dev_warn(&dev->dev, "Unexpected full queue\n");
- if (unlikely(!virtqueue_enable_cb(vi->svq))) {
- virtqueue_disable_cb(vi->svq);
- netif_start_queue(dev);
- goto again;
+ if (net_ratelimit()) {
+ if (likely(capacity == -ENOMEM))
+ dev_warn(&dev->dev,
+ "TX queue failure: out of memory\n");
+ else
+ dev_warn(&dev->dev,
+ "Unexpected TX queue failure: %d\n",
+ capacity);
}
return NETDEV_TX_BUSY;
}
--
1.7.1.12.g42b7f
^ permalink raw reply related
* [PATCH for-2.6.35] virtio_net: do not reschedule rx refill forever
From: Michael S. Tsirkin @ 2010-06-10 15:18 UTC (permalink / raw)
To: Rusty Russell, Michael S. Tsirkin, Jiri Pirko, Shirley Ma, netdev
We currently fill all of RX ring, then add_buf
returns ENOSPC, which gets mis-detected as an out of
memory condition and causes us to reschedule the work,
and so on forever. Fix this by oom = err == -ENOMEM;
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
Rusty, please review the following patch for 2.6.35.
drivers/net/virtio_net.c | 7 +++----
1 files changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 06c30df..85615a3 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -416,7 +416,7 @@ static int add_recvbuf_mergeable(struct virtnet_info *vi, gfp_t gfp)
static bool try_fill_recv(struct virtnet_info *vi, gfp_t gfp)
{
int err;
- bool oom = false;
+ bool oom;
do {
if (vi->mergeable_rx_bufs)
@@ -426,10 +426,9 @@ static bool try_fill_recv(struct virtnet_info *vi, gfp_t gfp)
else
err = add_recvbuf_small(vi, gfp);
- if (err < 0) {
- oom = true;
+ oom = err == -ENOMEM;
+ if (err < 0)
break;
- }
++vi->num;
} while (err > 0);
if (unlikely(vi->num > vi->max))
--
1.7.1.12.g42b7f
^ permalink raw reply related
* [PATCH for-2.6.35] virtio: return ENOMEM on out of memory
From: Michael S. Tsirkin @ 2010-06-10 15:16 UTC (permalink / raw)
To: Rusty Russell, Michael S. Tsirkin, Amit Shah, Mark McLoughlin,
Tejun Heo <tj>
add_buf returns ring size on out of memory,
this is not what devices expect.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
Please consider this patch for 2.6.35.
drivers/virtio/virtio_ring.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index ed845b7..dd35b34 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -119,7 +119,7 @@ static int vring_add_indirect(struct vring_virtqueue *vq,
desc = kmalloc((out + in) * sizeof(struct vring_desc), gfp);
if (!desc)
- return vq->vring.num;
+ return -ENOMEM;
/* Transfer entries from the sg list into the indirect page */
for (i = 0; i < out; i++) {
--
1.7.1.12.g42b7f
^ permalink raw reply related
* CloudComp2010 Deadline Extended to June 14 (2nd International Conference on Cloud Computing)
From: Ming Zhao @ 2010-06-09 13:58 UTC (permalink / raw)
To: tcpp-announce, performance, infodir_sigarch, vsigplan-l,
publicity, micro_publicity, hpc-announ
Update:
-------
* Paper submission deadline extended to: June 14, 2010
========================================================================
Call for Papers
The 2nd International Conference on Cloud Computing
(CloudComp2010)
Barcelona, Spain, October 25 - 28, 2010
========================================================================
Conference Scope:
-----------------
Cloud Computing is an emerging computing paradigm envisioned to change
all IT landscape facets including technology, business, services and
human resources. It is a consumer/delivery model that offers IT
capabilities as services, billed based on usage. Many such cloud
services can be envisioned, but the main ones are IaaS (Infrastructure-
as-a-Service), PaaS (Platform-as-a-Service), and SaaS (Software-as-a-
Service). The underlying cloud architecture includes a pool of
virtualized compute, storage and networking resources that can be
aggregated and launched as platforms to run workloads and satisfy their
Service-Level Agreement (SLA). Cloud architectures also include
provisions to best guarantee service delivery for clients and at the
same time optimize efficiency of resources of providers. Examples of
provisions include, but not limited to, elasticity through scaling
resources up/down to track workload behavior, extensive monitoring,
failure mitigation, and energy optimizations. The two main technologies
enabling clouds are: (i) Virtualization, the foundation of clouds; and
(ii) Manageability (autonomics), the command and control of clouds.
CloudComp is intended to bring together researchers, developers, and
industry professionals to discuss clouds, cloud computing and related
ecosystems support. To that end, papers are solicited from all cloud
related areas, including, but not limited to:
1. Cloud architectures and provisions to optimize providers'
environments while guaranteeing clients' SLAs.
2. Programming models, applications and middleware suitable for dynamic
cloud environments.
3. End-to-end techniques for autonomic management of cloud resources
including monitoring, asset management, process automation and
others.
4. New cloud delivery models, models' optimizations and associated
architectural changes.
5. New cloud economic and billing models.
6. Cloud security, privacy and compliance challenges.
7. Toolkits, frameworks and processes to enable clouds and allow
seamless transitions from traditional IT environments to clouds.
8. Experiences with existing cloud infrastructure, services and uses.
9. Novel human interfaces and browsers for accessing clouds.
10.Interaction of mobile computing, mCommerce and Clouds.
Cloud Computing conference is sponsored by ICST (Institute for Computer
Sciences, Social-Informatics and Telecommunications Engineering).
Important Dates:
----------------
* Paper submission: June 14, 2010
* Authors' notification: July 26, 2010
* Final manuscripts: August 23, 2010
Organization:
-------------
* General Chair:
o Mazin Yousif, IBM Corporation, Canada
* Program Chairs:
o Burkhard Neidecker-Lutz, SAP Research, Germany
o Christine Morin, INRIA, France
* Program Committee:
o Albert Zomaya, University of Sydney, Australia
o Cristiana Amza, University of Toronto, Canada
o Daniel Mosse, University of Pittsburgh, USA
o Dongyan Xu, Purdue University, USA
o Erol Gelenbe, Imperial College, UK
o Franco Zambonelli, Universita di Modena e Reggio Emilia, Italy
o Frederic Desprez, INRIA
o George Galambos, IBM Canada, Canada
o Giovanna Di Marzo Serugendo, University of London, UK
o Guillaume Pierre, Vrije Universiteit Amsterdam
o Ignacio Llorente, Universidad Complutense de Madrid
o Ivona Brandic, Vienna University of Technology, Austria
o Jose Antonio Lozano, Telefonica I+D, Spain
o Jose Bernabeu-Auban, Microsoft, USA
o Juergen Falkner, Fraunhofer, Germany
o Julie McCann, Imperial College, UK
o Karsten Schwan, Georgia Institute of Technology, USA
o Keith Jeffery, Rutherford Appleton Laboratory, UK
o Mikhail Smirnov, Fraunhofer, Germany
o Omar Rana, Cardiff University, UK
o Ozalp Babaoglu, University of Bologna, Italy
o Rajkumar Buyya, Manjrasoft, Australia
o Renato Figueiredo, University of Florida, USA
o Ricardo Bianchini, Rutgers University, USA
o Ron Brightwell, Sandia National Labs, USA
o Simon Dobson, University of St Andrews, UK
o Tamer Aboualy, IBM Corporation, Canada
o Wu Feng, Virginia Tech University, USA
* Demos/Exhibits Chair:
o Katerina Goulioutkina, IBM Corporation, Canada
* Workshop/Tutorial Chair:
o Simon Dobson, University of St Andrews, UK
* CloudComp Summit Chair:
o Duncan Johnston-Watt, Cloudsoft Corporation, UK
* Publicity Chair:
o Ming Zhao, Florida International University, USA
* Local Arrangements Chair:
o Joan Serrat, UPC, Barcelona, Spain
* Industry Session Chair:
o Duncan Johnston-Watt, Cloudsoft Corporation, UK
Further Information:
--------------------
Web: http://www.cloudcomp.eu
Email: cloudcomp2010@icst.org
--
Ming Zhao, Assistant Professor
School of Computing and Information Sciences
Florida International University
Tel: (305) 348-2034, Fax: (305) 348-3549
Web: http://www.cis.fiu.edu/~zhaom
^ permalink raw reply
* Re: [PATCH] virtio_net: indicate oom when addbuf returns failure
From: Michael S. Tsirkin @ 2010-06-07 9:15 UTC (permalink / raw)
To: Herbert Xu; +Cc: Bruce Rogers, netdev, Shirley Ma, virtualization, stable
In-Reply-To: <20100606222441.GA5992@gondor.apana.org.au>
On Mon, Jun 07, 2010 at 08:24:41AM +1000, Herbert Xu wrote:
> On Sun, Jun 06, 2010 at 11:13:00PM +0300, Michael S. Tsirkin wrote:
> >
> > Actually this code looks strange:
> > Note that add_buf inicates out of memory
> > condition with a positive return value, and ring full
> > (which is not an error!) with -ENOSPC.
>
> Indeed, this ultimately came from
>
> commit 9ab86bbcf8be755256f0a5e994e0b38af6b4d399
> Author: Shirley Ma <mashirle@us.ibm.com>
> Date: Fri Jan 29 03:20:04 2010 +0000
>
> virtio_net: Defer skb allocation in receive path Date: Wed, 13 Jan 2010 12:53:38 -0800
>
> (Greg, please don't apply this even though I've just given you
> the upstream commit ID that you were asking for :)
>
> where it confuses a memory allocation error with an add_buf failure.
>
> > Possibly the right thing to do is to
> > 1. handle ENOMEM specially
> > 2. fix add_buf to return ENOMEM on error
>
> I think we should make it so that only a memory allocation error
> is returned as before. There is no need for returning the add_buf
> error unless add_buf is now doing an allocation itself that needs
> to be retried.
That's what my patch did, right? Ack it?
> Thanks,
> --
> Visit Openswan at http://www.openswan.org/
> Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [PATCH] virtio_net: indicate oom when addbuf returns failure
From: Rusty Russell @ 2010-06-07 2:24 UTC (permalink / raw)
To: Herbert Xu
Cc: Bruce Rogers, netdev, Shirley Ma, virtualization,
Michael S. Tsirkin, stable
In-Reply-To: <20100606222441.GA5992@gondor.apana.org.au>
On Mon, 7 Jun 2010 07:54:41 am Herbert Xu wrote:
> On Sun, Jun 06, 2010 at 11:13:00PM +0300, Michael S. Tsirkin wrote:
> >
> > Actually this code looks strange:
> > Note that add_buf inicates out of memory
> > condition with a positive return value, and ring full
> > (which is not an error!) with -ENOSPC.
>
> Indeed, this ultimately came from
>
> commit 9ab86bbcf8be755256f0a5e994e0b38af6b4d399
> Author: Shirley Ma <mashirle@us.ibm.com>
> Date: Fri Jan 29 03:20:04 2010 +0000
>
> virtio_net: Defer skb allocation in receive path Date: Wed, 13 Jan 2010 12:53:38 -0800
>
> (Greg, please don't apply this even though I've just given you
> the upstream commit ID that you were asking for :)
>
> where it confuses a memory allocation error with an add_buf failure.
>
> > Possibly the right thing to do is to
> > 1. handle ENOMEM specially
> > 2. fix add_buf to return ENOMEM on error
>
> I think we should make it so that only a memory allocation error
> is returned as before. There is no need for returning the add_buf
> error unless add_buf is now doing an allocation itself that needs
> to be retried.
With indirect bufs, this is indeed the case. The code works except
for the bigpackets !mergable case, but should be clarified anyway.
See my other mail...
Thanks,
Rusty.
^ permalink raw reply
* Re: [PATCH] virtio_net: indicate oom when addbuf returns failure
From: Rusty Russell @ 2010-06-07 2:17 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Herbert Xu, netdev, virtualization, stable, Bruce Rogers
In-Reply-To: <20100606201258.GA21196@redhat.com>
On Mon, 7 Jun 2010 05:43:00 am Michael S. Tsirkin wrote:
> On Fri, Jun 04, 2010 at 10:28:56AM +0930, Rusty Russell wrote:
> > This patch is a subset of an already upstream patch, but this portion
> > is useful in earlier releases.
> >
> > Please consider for the 2.6.32 and 2.6.33 stable trees.
> >
> > If the add_buf operation fails, indicate failure to the caller.
> >
> > Signed-off-by: Bruce Rogers <brogers@novell.com>
> > Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
>
> Actually this code looks strange:
> Note that add_buf inicates out of memory
> condition with a positive return value, and ring full
> (which is not an error!) with -ENOSPC.
>
> So it seems that this patch (and upstream code) will fill
> the ring and then end up setting oom = true and rescheduling the work
> forever. And I suspect I actually saw this at some point
> on one of my systems: observed BW would drop
> with high CPU usage until reboot.
> Can't reproduce it now anymore ..
I thought that at first too, but it's subtler than that.
When the ring is exactly filled, err = 0. With mergeable bufs and small
bufs that's the. With big buffers, it's probably not, and the code will
indeed respond as if always out of memory, always trying to refill.
Our current code has the same error. Probably a combination of noone using
big buffers, and noone noticing one timer every 1/2 second.
Want to fix that properly? And comment it? :)
Thanks,
Rusty.
^ permalink raw reply
* Re: [PATCH] virtio_net: indicate oom when addbuf returns failure
From: Herbert Xu @ 2010-06-06 22:24 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Bruce Rogers, netdev, Shirley Ma, virtualization, stable
In-Reply-To: <20100606201258.GA21196@redhat.com>
On Sun, Jun 06, 2010 at 11:13:00PM +0300, Michael S. Tsirkin wrote:
>
> Actually this code looks strange:
> Note that add_buf inicates out of memory
> condition with a positive return value, and ring full
> (which is not an error!) with -ENOSPC.
Indeed, this ultimately came from
commit 9ab86bbcf8be755256f0a5e994e0b38af6b4d399
Author: Shirley Ma <mashirle@us.ibm.com>
Date: Fri Jan 29 03:20:04 2010 +0000
virtio_net: Defer skb allocation in receive path Date: Wed, 13 Jan 2010 12:53:38 -0800
(Greg, please don't apply this even though I've just given you
the upstream commit ID that you were asking for :)
where it confuses a memory allocation error with an add_buf failure.
> Possibly the right thing to do is to
> 1. handle ENOMEM specially
> 2. fix add_buf to return ENOMEM on error
I think we should make it so that only a memory allocation error
is returned as before. There is no need for returning the add_buf
error unless add_buf is now doing an allocation itself that needs
to be retried.
Thanks,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [PATCH] virtio_net: indicate oom when addbuf returns failure
From: Michael S. Tsirkin @ 2010-06-06 20:13 UTC (permalink / raw)
To: Rusty Russell; +Cc: Herbert Xu, netdev, virtualization, stable, Bruce Rogers
In-Reply-To: <201006041028.56798.rusty@rustcorp.com.au>
On Fri, Jun 04, 2010 at 10:28:56AM +0930, Rusty Russell wrote:
> This patch is a subset of an already upstream patch, but this portion
> is useful in earlier releases.
>
> Please consider for the 2.6.32 and 2.6.33 stable trees.
>
> If the add_buf operation fails, indicate failure to the caller.
>
> Signed-off-by: Bruce Rogers <brogers@novell.com>
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Actually this code looks strange:
Note that add_buf inicates out of memory
condition with a positive return value, and ring full
(which is not an error!) with -ENOSPC.
So it seems that this patch (and upstream code) will fill
the ring and then end up setting oom = true and rescheduling the work
forever. And I suspect I actually saw this at some point
on one of my systems: observed BW would drop
with high CPU usage until reboot.
Can't reproduce it now anymore ..
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
>
> @@ -318,6 +318,7 @@ static bool try_fill_recv_maxbufs(struct
> skb_unlink(skb, &vi->recv);
> trim_pages(vi, skb);
> kfree_skb(skb);
> + oom = true;
> break;
> }
> vi->num++;
> @@ -368,6 +369,7 @@ static bool try_fill_recv(struct virtnet
> if (err < 0) {
> skb_unlink(skb, &vi->recv);
> kfree_skb(skb);
> + oom = true;
> break;
> }
> vi->num++;
Possibly the right thing to do is to
1. handle ENOMEM specially
2. fix add_buf to return ENOMEM on error
Something like the below for upstream (warning: compile
tested only) and a similar one later for stable:
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 06c30df..85615a3 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -416,7 +416,7 @@ static int add_recvbuf_mergeable(struct virtnet_info *vi, gfp_t gfp)
static bool try_fill_recv(struct virtnet_info *vi, gfp_t gfp)
{
int err;
- bool oom = false;
+ bool oom;
do {
if (vi->mergeable_rx_bufs)
@@ -426,10 +426,9 @@ static bool try_fill_recv(struct virtnet_info *vi, gfp_t gfp)
else
err = add_recvbuf_small(vi, gfp);
- if (err < 0) {
- oom = true;
+ oom = err == -ENOMEM;
+ if (err < 0)
break;
- }
++vi->num;
} while (err > 0);
if (unlikely(vi->num > vi->max))
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index f9e6fbb..3c7f10a 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -119,7 +119,7 @@ static int vring_add_indirect(struct vring_virtqueue *vq,
desc = kmalloc((out + in) * sizeof(struct vring_desc), gfp);
if (!desc)
- return vq->vring.num;
+ return -ENOMEM;
/* Transfer entries from the sg list into the indirect page */
for (i = 0; i < out; i++) {
^ permalink raw reply related
* Re: [PATCHv3 1/2] virtio: support layout with avail ring before idx
From: Michael S. Tsirkin @ 2010-06-06 9:11 UTC (permalink / raw)
To: Rusty Russell
Cc: qemu-devel, Andrew Morton, linux-kernel, kvm, virtualization
In-Reply-To: <201006051340.27194.rusty@rustcorp.com.au>
On Sat, Jun 05, 2010 at 01:40:26PM +0930, Rusty Russell wrote:
> On Fri, 4 Jun 2010 09:12:05 pm Michael S. Tsirkin wrote:
> > On Fri, Jun 04, 2010 at 08:46:49PM +0930, Rusty Russell wrote:
> > > I'm uncomfortable with moving a field.
> > >
> > > We haven't done that before and I wonder what will break with old code.
> >
> > With e.g. my patch, We only do this conditionally when bit is negotitated.
>
> Of course, but see this change:
>
> commit ef688e151c00e5d529703be9a04fd506df8bc54e
> Author: Rusty Russell <rusty@rustcorp.com.au>
> Date: Fri Jun 12 22:16:35 2009 -0600
>
> virtio: meet virtio spec by finalizing features before using device
>
> Virtio devices are supposed to negotiate features before they start using
> the device, but the current code doesn't do this. This is because the
> driver's probe() function invariably has to add buffers to a virtqueue,
> or probe the disk (virtio_blk).
>
> This currently doesn't matter since no existing backend is strict about
> the feature negotiation. But it's possible to imagine a future feature
> which completely changes how a device operates: in this case, we'd need
> to acknowledge it before using the device.
>
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
>
> Now, this isn't impossible to overcome: we know that if they use the ring
> before completing feature negotiation then they don't understand the new
> format.
>
> But we have to be aware of that on the qemu side. Are we?
I think we are ok. virtqueue_init which sets the avail/ysed pointers is
called when we write the base address. So we only need to be careful
and not change this feature bit after creating the rings.
> > > Should we instead just abandon the flags field and use last_used only?
> > > Or, more radically, put flags == last_used when the feature is on?
> > >
> > > Thoughts?
> > > Rusty.
> >
> > Hmm, e.g. with TX and virtio net, we almost never want interrupts,
> > whatever the index value.
>
> Good point. OK, I give in, I'll take your patch which moves the fields
> to the end. Is that your preference?
Yes, I think so.
You mean PATCHv3 unchanged with 254 byte padding?
> Please be careful with the qemu side though...
>
> It's not inconceivable that I'll write that virtio cacheline simulator this
> (coming) week, too...
>
> Thanks.
> Rusty.
^ permalink raw reply
* Re: [PATCHv3 1/2] virtio: support layout with avail ring before idx
From: Rusty Russell @ 2010-06-05 4:10 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: qemu-devel, Andrew Morton, linux-kernel, kvm, virtualization
In-Reply-To: <20100604114205.GA22599@redhat.com>
On Fri, 4 Jun 2010 09:12:05 pm Michael S. Tsirkin wrote:
> On Fri, Jun 04, 2010 at 08:46:49PM +0930, Rusty Russell wrote:
> > I'm uncomfortable with moving a field.
> >
> > We haven't done that before and I wonder what will break with old code.
>
> With e.g. my patch, We only do this conditionally when bit is negotitated.
Of course, but see this change:
commit ef688e151c00e5d529703be9a04fd506df8bc54e
Author: Rusty Russell <rusty@rustcorp.com.au>
Date: Fri Jun 12 22:16:35 2009 -0600
virtio: meet virtio spec by finalizing features before using device
Virtio devices are supposed to negotiate features before they start using
the device, but the current code doesn't do this. This is because the
driver's probe() function invariably has to add buffers to a virtqueue,
or probe the disk (virtio_blk).
This currently doesn't matter since no existing backend is strict about
the feature negotiation. But it's possible to imagine a future feature
which completely changes how a device operates: in this case, we'd need
to acknowledge it before using the device.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Now, this isn't impossible to overcome: we know that if they use the ring
before completing feature negotiation then they don't understand the new
format.
But we have to be aware of that on the qemu side. Are we?
> > Should we instead just abandon the flags field and use last_used only?
> > Or, more radically, put flags == last_used when the feature is on?
> >
> > Thoughts?
> > Rusty.
>
> Hmm, e.g. with TX and virtio net, we almost never want interrupts,
> whatever the index value.
Good point. OK, I give in, I'll take your patch which moves the fields
to the end. Is that your preference?
Please be careful with the qemu side though...
It's not inconceivable that I'll write that virtio cacheline simulator this
(coming) week, too...
Thanks.
Rusty.
^ permalink raw reply
* Re: [PATCHv3 1/2] virtio: support layout with avail ring before idx
From: Michael S. Tsirkin @ 2010-06-04 11:42 UTC (permalink / raw)
To: Rusty Russell
Cc: qemu-devel, Andrew Morton, linux-kernel, kvm, virtualization
In-Reply-To: <201006042046.49872.rusty@rustcorp.com.au>
On Fri, Jun 04, 2010 at 08:46:49PM +0930, Rusty Russell wrote:
> On Fri, 4 Jun 2010 08:05:43 pm Michael S. Tsirkin wrote:
> > On Fri, Jun 04, 2010 at 12:04:57PM +0930, Rusty Russell wrote:
> > > On Wed, 2 Jun 2010 12:17:12 am Michael S. Tsirkin wrote:
> > > > This adds an (unused) option to put available ring before control (avail
> > > > index, flags), and adds padding between index and flags. This avoids
> > > > cache line sharing between control and ring, and also makes it possible
> > > > to extend avail control without incurring extra cache misses.
> > > >
> > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > >
> > > No no no no. 254? You're trying to Morton me![1]
> >
> > Hmm, I wonder what will we do if we want a 3rd field on
> > a separate chacheline. But ok.
> >
> > > How's this (untested):
> >
> > I think we also want to put flags there as well,
> > they are used on interrupt path, together with last used index.
>
> I'm uncomfortable with moving a field.
>
> We haven't done that before and I wonder what will break with old code.
With e.g. my patch, We only do this conditionally when bit is negotitated.
> Should we instead just abandon the flags field and use last_used only?
> Or, more radically, put flags == last_used when the feature is on?
>
> Thoughts?
> Rusty.
Hmm, e.g. with TX and virtio net, we almost never want interrupts,
whatever the index value.
--
MST
^ permalink raw reply
* Re: [PATCHv3 1/2] virtio: support layout with avail ring before idx
From: Rusty Russell @ 2010-06-04 11:16 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: qemu-devel, Andrew Morton, linux-kernel, kvm, virtualization
In-Reply-To: <20100604103543.GA22270@redhat.com>
On Fri, 4 Jun 2010 08:05:43 pm Michael S. Tsirkin wrote:
> On Fri, Jun 04, 2010 at 12:04:57PM +0930, Rusty Russell wrote:
> > On Wed, 2 Jun 2010 12:17:12 am Michael S. Tsirkin wrote:
> > > This adds an (unused) option to put available ring before control (avail
> > > index, flags), and adds padding between index and flags. This avoids
> > > cache line sharing between control and ring, and also makes it possible
> > > to extend avail control without incurring extra cache misses.
> > >
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >
> > No no no no. 254? You're trying to Morton me![1]
>
> Hmm, I wonder what will we do if we want a 3rd field on
> a separate chacheline. But ok.
>
> > How's this (untested):
>
> I think we also want to put flags there as well,
> they are used on interrupt path, together with last used index.
I'm uncomfortable with moving a field.
We haven't done that before and I wonder what will break with old code.
Should we instead just abandon the flags field and use last_used only?
Or, more radically, put flags == last_used when the feature is on?
Thoughts?
Rusty.
^ permalink raw reply
* Re: [PATCHv3 1/2] virtio: support layout with avail ring before idx
From: Michael S. Tsirkin @ 2010-06-04 10:35 UTC (permalink / raw)
To: Rusty Russell
Cc: qemu-devel, Andrew Morton, linux-kernel, kvm, virtualization
In-Reply-To: <201006041204.57973.rusty@rustcorp.com.au>
On Fri, Jun 04, 2010 at 12:04:57PM +0930, Rusty Russell wrote:
> On Wed, 2 Jun 2010 12:17:12 am Michael S. Tsirkin wrote:
> > This adds an (unused) option to put available ring before control (avail
> > index, flags), and adds padding between index and flags. This avoids
> > cache line sharing between control and ring, and also makes it possible
> > to extend avail control without incurring extra cache misses.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> No no no no. 254? You're trying to Morton me![1]
Hmm, I wonder what will we do if we want a 3rd field on
a separate chacheline. But ok.
> How's this (untested):
I think we also want to put flags there as well,
they are used on interrupt path, together with last used index.
> diff --git a/include/linux/virtio_ring.h b/include/linux/virtio_ring.h
> --- a/include/linux/virtio_ring.h
> +++ b/include/linux/virtio_ring.h
> @@ -74,8 +74,8 @@ struct vring {
> /* The standard layout for the ring is a continuous chunk of memory which looks
> * like this. We assume num is a power of 2.
> *
> - * struct vring
> - * {
> + * struct vring {
> + * *** The driver writes to this part.
> * // The actual descriptors (16 bytes each)
> * struct vring_desc desc[num];
> *
> @@ -84,9 +84,11 @@ struct vring {
> * __u16 avail_idx;
> * __u16 available[num];
> *
> - * // Padding to the next align boundary.
> + * // Padding so used_flags is on the next align boundary.
> * char pad[];
> + * __u16 last_used; // On a cacheline of its own.
> *
> + * *** The device writes to this part.
> * // A ring of used descriptor heads with free-running index.
> * __u16 used_flags;
> * __u16 used_idx;
> @@ -110,6 +112,12 @@ static inline unsigned vring_size(unsign
> + sizeof(__u16) * 2 + sizeof(struct vring_used_elem) * num;
> }
>
> +/* Last used index sits at the very end of the driver part of the struct */
> +static inline __u16 *vring_last_used_idx(const struct vring *vr)
> +{
> + return (__u16 *)vr->used - 1;
> +}
> +
> #ifdef __KERNEL__
> #include <linux/irqreturn.h>
> struct virtio_device;
>
> Cheers,
> Rusty.
> [1] Andrew Morton has this technique where he posts a solution so ugly it
> forces others to fix it properly. Ego-roping, basically.
^ permalink raw reply
* Re: [PATCHv3 1/2] virtio: support layout with avail ring before idx
From: Rusty Russell @ 2010-06-04 2:34 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: qemu-devel, Andrew Morton, linux-kernel, kvm, virtualization
In-Reply-To: <10a74b58c908bad64ff890c881e2b2de88687f0e.1275403477.git.mst@redhat.com>
On Wed, 2 Jun 2010 12:17:12 am Michael S. Tsirkin wrote:
> This adds an (unused) option to put available ring before control (avail
> index, flags), and adds padding between index and flags. This avoids
> cache line sharing between control and ring, and also makes it possible
> to extend avail control without incurring extra cache misses.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
No no no no. 254? You're trying to Morton me![1]
How's this (untested):
diff --git a/include/linux/virtio_ring.h b/include/linux/virtio_ring.h
--- a/include/linux/virtio_ring.h
+++ b/include/linux/virtio_ring.h
@@ -74,8 +74,8 @@ struct vring {
/* The standard layout for the ring is a continuous chunk of memory which looks
* like this. We assume num is a power of 2.
*
- * struct vring
- * {
+ * struct vring {
+ * *** The driver writes to this part.
* // The actual descriptors (16 bytes each)
* struct vring_desc desc[num];
*
@@ -84,9 +84,11 @@ struct vring {
* __u16 avail_idx;
* __u16 available[num];
*
- * // Padding to the next align boundary.
+ * // Padding so used_flags is on the next align boundary.
* char pad[];
+ * __u16 last_used; // On a cacheline of its own.
*
+ * *** The device writes to this part.
* // A ring of used descriptor heads with free-running index.
* __u16 used_flags;
* __u16 used_idx;
@@ -110,6 +112,12 @@ static inline unsigned vring_size(unsign
+ sizeof(__u16) * 2 + sizeof(struct vring_used_elem) * num;
}
+/* Last used index sits at the very end of the driver part of the struct */
+static inline __u16 *vring_last_used_idx(const struct vring *vr)
+{
+ return (__u16 *)vr->used - 1;
+}
+
#ifdef __KERNEL__
#include <linux/irqreturn.h>
struct virtio_device;
Cheers,
Rusty.
[1] Andrew Morton has this technique where he posts a solution so ugly it
forces others to fix it properly. Ego-roping, basically.
^ permalink raw reply
* Re: [PATCH 0/3][STABLE] KVM: Various issues in virtio_net
From: Rusty Russell @ 2010-06-04 0:38 UTC (permalink / raw)
To: Bruce Rogers; +Cc: virtualization, Michael S. Tsirkin
In-Reply-To: <4C07D73202000048000974FB@sinclair.provo.novell.com>
On Fri, 4 Jun 2010 07:54:18 am Bruce Rogers wrote:
> These are patches which we have found useful for our 2.6.32 based SLES 11 SP1 release.
>
> The first patch is already upstream, but should be included in 2.6.32 stable tree.
>
> The second patch is a subset of another upstream patch. Again, stable material, applicable to 2.6.32 and 2.6.33.
>
> The third patch solves the last remaining issue we saw when testing kvm configurations with the SUSE certification test suite. Under heavy load, we observed rx stalls (first two patches applied), and this third patch was crafted to address the issue. Please apply to 2.6.32, 2.6.33, and 2.6.34 stable trees.
Hi Bruce, thanks for these.
netdev is the right place for these patches. I'll send the first two to
stable@kernel.org myself; please add the following two lines to the last one
and repost to netdev@vger.kernel.org.
Acked-by: Rusty Russell <rusty@rustcorp.com.au>
Cc: stable@kernel.org
Thanks!
Rusty.
^ permalink raw reply
* [PATCH 3/3][STABLE] KVM: add schedule check to napi_enable call
From: Bruce Rogers @ 2010-06-03 22:29 UTC (permalink / raw)
To: rusty; +Cc: virtualization
Please consider this patch for the 2.6.32, 2.6.33, and 2.6.34 stable trees as well as current development trees. (I've only tested on 2.6.32 however)
virtio_net: Add schedule check to napi_enable call
Under harsh testing conditions, including low memory, the guest would
stop receiving packets. With this patch applied we no longer see any
problems in the driver while performing these tests for extended periods
of time.
Make sure napi is scheduled subsequent to each napi_enable.
Signed-off-by: Bruce Rogers <brogers@novell.com>
Signed-off-by: Olaf Kirch <okir@suse.de>
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -388,6 +388,20 @@ static void skb_recv_done(struct virtque
}
}
+static void virtnet_napi_enable(struct virtnet_info *vi)
+{
+ napi_enable(&vi->napi);
+
+ /* If all buffers were filled by other side before we napi_enabled, we
+ * won't get another interrupt, so process any outstanding packets
+ * now. virtnet_poll wants re-enable the queue, so we disable here.
+ * We synchronize against interrupts via NAPI_STATE_SCHED */
+ if (napi_schedule_prep(&vi->napi)) {
+ vi->rvq->vq_ops->disable_cb(vi->rvq);
+ __napi_schedule(&vi->napi);
+ }
+}
+
static void refill_work(struct work_struct *work)
{
struct virtnet_info *vi;
@@ -397,7 +411,7 @@ static void refill_work(struct work_stru
napi_disable(&vi->napi);
try_fill_recv(vi, GFP_KERNEL);
still_empty = (vi->num == 0);
- napi_enable(&vi->napi);
+ virtnet_napi_enable(vi);
/* In theory, this can happen: if we don't get any buffers in
* we will *never* try to fill again. */
@@ -589,16 +603,7 @@ static int virtnet_open(struct net_devic
{
struct virtnet_info *vi = netdev_priv(dev);
- napi_enable(&vi->napi);
-
- /* If all buffers were filled by other side before we napi_enabled, we
- * won't get another interrupt, so process any outstanding packets
- * now. virtnet_poll wants re-enable the queue, so we disable here.
- * We synchronize against interrupts via NAPI_STATE_SCHED */
- if (napi_schedule_prep(&vi->napi)) {
- vi->rvq->vq_ops->disable_cb(vi->rvq);
- __napi_schedule(&vi->napi);
- }
+ virtnet_napi_enable(vi);
return 0;
}
^ 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