Re: [PATCH v3 2/2] virtio-ring: Allocate indirect buffers from cache when possible
On 09/04/2012 09:41 PM, Michael S. Tsirkin wrote: On Tue, Sep 04, 2012 at 07:34:19PM +0300, Avi Kivity wrote: On 08/31/2012 12:56 PM, Michael S. Tsirkin wrote: On Fri, Aug 31, 2012 at 11:36:07AM +0200, Sasha Levin wrote: On 08/30/2012 03:38 PM, Michael S. Tsirkin wrote: +static unsigned int indirect_alloc_thresh = 16; Why 16? Please make is MAX_SG + 1 this makes some sense. Wouldn't MAX_SG mean we always allocate from the cache? Isn't the memory waste too big in this case? Sorry. I really meant MAX_SKB_FRAGS + 1. MAX_SKB_FRAGS is 17 so gets us threshold of 18. It is less than the size of an skb+shinfo itself so - does it look too big to you? Also why do you think 16 is not too big but 18 is? If there's a reason then I am fine with 16 too but then please put it in code comment near where the value is set. Yes this means virtio net always allocates from cache but this is a good thing, isn't it? Gets us more consistent performance. kmalloc() also goes to a cache. Is there a measurable difference? Yes see 0/2 and followup discussion. I don't see 0/2, looks like this was not threaded properly. What was the subject line? -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/2] virtio-ring: Allocate indirect buffers from cache when possible
On Wed, Sep 05, 2012 at 05:21:12PM +0300, Avi Kivity wrote: On 09/04/2012 09:41 PM, Michael S. Tsirkin wrote: On Tue, Sep 04, 2012 at 07:34:19PM +0300, Avi Kivity wrote: On 08/31/2012 12:56 PM, Michael S. Tsirkin wrote: On Fri, Aug 31, 2012 at 11:36:07AM +0200, Sasha Levin wrote: On 08/30/2012 03:38 PM, Michael S. Tsirkin wrote: +static unsigned int indirect_alloc_thresh = 16; Why 16? Please make is MAX_SG + 1 this makes some sense. Wouldn't MAX_SG mean we always allocate from the cache? Isn't the memory waste too big in this case? Sorry. I really meant MAX_SKB_FRAGS + 1. MAX_SKB_FRAGS is 17 so gets us threshold of 18. It is less than the size of an skb+shinfo itself so - does it look too big to you? Also why do you think 16 is not too big but 18 is? If there's a reason then I am fine with 16 too but then please put it in code comment near where the value is set. Yes this means virtio net always allocates from cache but this is a good thing, isn't it? Gets us more consistent performance. kmalloc() also goes to a cache. Is there a measurable difference? Yes see 0/2 and followup discussion. I don't see 0/2, looks like this was not threaded properly. What was the subject line? My mistake, there is no 0/2, the resolts where in the followup thread of the previous version: [PATCH v2 2/2] virtio-ring: Allocate indirect buffers from cache when possible Sasha, could you please accompany the next version with a cover letter 0/2) including performance results? -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/2] virtio-ring: Allocate indirect buffers from cache when possible
On 08/31/2012 12:56 PM, Michael S. Tsirkin wrote: On Fri, Aug 31, 2012 at 11:36:07AM +0200, Sasha Levin wrote: On 08/30/2012 03:38 PM, Michael S. Tsirkin wrote: +static unsigned int indirect_alloc_thresh = 16; Why 16? Please make is MAX_SG + 1 this makes some sense. Wouldn't MAX_SG mean we always allocate from the cache? Isn't the memory waste too big in this case? Sorry. I really meant MAX_SKB_FRAGS + 1. MAX_SKB_FRAGS is 17 so gets us threshold of 18. It is less than the size of an skb+shinfo itself so - does it look too big to you? Also why do you think 16 is not too big but 18 is? If there's a reason then I am fine with 16 too but then please put it in code comment near where the value is set. Yes this means virtio net always allocates from cache but this is a good thing, isn't it? Gets us more consistent performance. kmalloc() also goes to a cache. Is there a measurable difference? Ugh, there's an ugly loop in __find_general_cachep(), which really wants to be replaced with fls(). -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/2] virtio-ring: Allocate indirect buffers from cache when possible
On 09/04/2012 07:34 PM, Avi Kivity wrote: On 08/31/2012 12:56 PM, Michael S. Tsirkin wrote: On Fri, Aug 31, 2012 at 11:36:07AM +0200, Sasha Levin wrote: On 08/30/2012 03:38 PM, Michael S. Tsirkin wrote: +static unsigned int indirect_alloc_thresh = 16; Why 16? Please make is MAX_SG + 1 this makes some sense. Wouldn't MAX_SG mean we always allocate from the cache? Isn't the memory waste too big in this case? Sorry. I really meant MAX_SKB_FRAGS + 1. MAX_SKB_FRAGS is 17 so gets us threshold of 18. It is less than the size of an skb+shinfo itself so - does it look too big to you? Also why do you think 16 is not too big but 18 is? If there's a reason then I am fine with 16 too but then please put it in code comment near where the value is set. Yes this means virtio net always allocates from cache but this is a good thing, isn't it? Gets us more consistent performance. kmalloc() also goes to a cache. Is there a measurable difference? Ugh, there's an ugly loop in __find_general_cachep(), which really wants to be replaced with fls(). Actually, not, as the loop will be very short for small sizes. -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/2] virtio-ring: Allocate indirect buffers from cache when possible
On Tue, Sep 04, 2012 at 07:34:19PM +0300, Avi Kivity wrote: On 08/31/2012 12:56 PM, Michael S. Tsirkin wrote: On Fri, Aug 31, 2012 at 11:36:07AM +0200, Sasha Levin wrote: On 08/30/2012 03:38 PM, Michael S. Tsirkin wrote: +static unsigned int indirect_alloc_thresh = 16; Why 16? Please make is MAX_SG + 1 this makes some sense. Wouldn't MAX_SG mean we always allocate from the cache? Isn't the memory waste too big in this case? Sorry. I really meant MAX_SKB_FRAGS + 1. MAX_SKB_FRAGS is 17 so gets us threshold of 18. It is less than the size of an skb+shinfo itself so - does it look too big to you? Also why do you think 16 is not too big but 18 is? If there's a reason then I am fine with 16 too but then please put it in code comment near where the value is set. Yes this means virtio net always allocates from cache but this is a good thing, isn't it? Gets us more consistent performance. kmalloc() also goes to a cache. Is there a measurable difference? Yes see 0/2 and followup discussion. Ugh, there's an ugly loop in __find_general_cachep(), which really wants to be replaced with fls(). -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/2] virtio-ring: Allocate indirect buffers from cache when possible
On 08/30/2012 03:38 PM, Michael S. Tsirkin wrote: +static unsigned int indirect_alloc_thresh = 16; Why 16? Please make is MAX_SG + 1 this makes some sense. Wouldn't MAX_SG mean we always allocate from the cache? Isn't the memory waste too big in this case? -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/2] virtio-ring: Allocate indirect buffers from cache when possible
On Fri, Aug 31, 2012 at 11:36:07AM +0200, Sasha Levin wrote: On 08/30/2012 03:38 PM, Michael S. Tsirkin wrote: +static unsigned int indirect_alloc_thresh = 16; Why 16? Please make is MAX_SG + 1 this makes some sense. Wouldn't MAX_SG mean we always allocate from the cache? Isn't the memory waste too big in this case? Sorry. I really meant MAX_SKB_FRAGS + 1. MAX_SKB_FRAGS is 17 so gets us threshold of 18. It is less than the size of an skb+shinfo itself so - does it look too big to you? Also why do you think 16 is not too big but 18 is? If there's a reason then I am fine with 16 too but then please put it in code comment near where the value is set. Yes this means virtio net always allocates from cache but this is a good thing, isn't it? Gets us more consistent performance. -- MST -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/2] virtio-ring: Allocate indirect buffers from cache when possible
On Thu, Aug 30, 2012 at 01:21:58PM +0200, Sasha Levin wrote:
Currently if VIRTIO_RING_F_INDIRECT_DESC is enabled we will
use indirect descriptors and allocate them using a simple
kmalloc().
This patch adds a cache which will allow indirect buffers under
a configurable size to be allocated from that cache instead.
Signed-off-by: Sasha Levin levinsasha...@gmail.com
I am not sure we need these module params.
But assuming we keep them, we need to validate values - they come from
user.
---
drivers/block/virtio_blk.c | 4
drivers/char/hw_random/virtio-rng.c | 4
drivers/char/virtio_console.c | 4
drivers/net/virtio_net.c| 4
drivers/virtio/virtio_balloon.c | 4
drivers/virtio/virtio_ring.c| 34 ++
include/linux/virtio.h | 1 +
net/9p/trans_virtio.c | 5 +
8 files changed, 56 insertions(+), 4 deletions(-)
diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 7c63065..e4c6c42 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -25,6 +25,9 @@ struct workqueue_struct *virtblk_wq;
static unsigned int indirect_thresh;
module_param(indirect_thresh, uint, S_IRUGO);
+static unsigned int indirect_alloc_thresh;
+module_param(indirect_alloc_thresh, uint, S_IRUGO);
+
struct virtio_blk
{
struct virtio_device *vdev;
@@ -739,6 +742,7 @@ static int __devinit virtblk_probe(struct virtio_device
*vdev)
INIT_WORK(vblk-config_work, virtblk_config_changed_work);
vblk-config_enable = true;
vdev-indirect_thresh = indirect_thresh;
+ vdev-indirect_alloc_thresh = indirect_alloc_thresh;
err = init_vq(vblk);
if (err)
diff --git a/drivers/char/hw_random/virtio-rng.c
b/drivers/char/hw_random/virtio-rng.c
index 3a644f1..ed22db8 100644
--- a/drivers/char/hw_random/virtio-rng.c
+++ b/drivers/char/hw_random/virtio-rng.c
@@ -28,6 +28,9 @@
static unsigned int indirect_thresh;
module_param(indirect_thresh, uint, S_IRUGO);
+static unsigned int indirect_alloc_thresh;
+module_param(indirect_alloc_thresh, uint, S_IRUGO);
+
static struct virtqueue *vq;
static unsigned int data_avail;
static DECLARE_COMPLETION(have_data);
@@ -97,6 +100,7 @@ static int probe_common(struct virtio_device *vdev)
/* We expect a single virtqueue. */
vdev-indirect_thresh = indirect_thresh;
+ vdev-indirect_alloc_thresh = indirect_alloc_thresh;
vq = virtio_find_single_vq(vdev, random_recv_done, input);
if (IS_ERR(vq))
return PTR_ERR(vq);
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index da2e44c..8f30732 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -42,6 +42,9 @@
static unsigned int indirect_thresh;
module_param(indirect_thresh, uint, S_IRUGO);
+static unsigned int indirect_alloc_thresh;
+module_param(indirect_alloc_thresh, uint, S_IRUGO);
+
/*
* This is a global struct for storing common data for all the devices
* this driver handles.
@@ -1891,6 +1894,7 @@ static int __devinit virtcons_probe(struct
virtio_device *vdev)
portdev-config.max_nr_ports) == 0)
multiport = true;
vdev-indirect_thresh = indirect_thresh;
+ vdev-indirect_alloc_thresh = indirect_alloc_thresh;
err = init_vqs(portdev);
if (err 0) {
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 949c89e..a00e19d 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -37,6 +37,9 @@ module_param(gso, bool, 0444);
static unsigned int indirect_thresh = 16;
module_param(indirect_thresh, uint, S_IRUGO);
+static unsigned int indirect_alloc_thresh = 16;
Why 16? Please make is MAX_SG + 1 this makes some sense.
+module_param(indirect_alloc_thresh, uint, S_IRUGO);
+
/* FIXME: MTU in config. */
#define MAX_PACKET_LEN (ETH_HLEN + VLAN_HLEN + ETH_DATA_LEN)
#define GOOD_COPY_LEN128
@@ -1132,6 +1135,7 @@ static int virtnet_probe(struct virtio_device *vdev)
if (virtio_has_feature(vdev, VIRTIO_NET_F_MRG_RXBUF))
vi-mergeable_rx_bufs = true;
vdev-indirect_thresh = indirect_thresh;
+ vdev-indirect_alloc_thresh = indirect_alloc_thresh;
err = init_vqs(vi);
if (err)
diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index ca5ae7a..039c4a6 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -38,6 +38,9 @@
static unsigned int indirect_thresh;
module_param(indirect_thresh, uint, S_IRUGO);
+static unsigned int indirect_alloc_thresh;
+module_param(indirect_alloc_thresh, uint, S_IRUGO);
+
struct virtio_balloon
{
struct virtio_device *vdev;
@@ -360,6 +363,7 @@ static int virtballoon_probe(struct virtio_device *vdev)
vb-vdev = vdev;
