[Qemu-devel] [PATCH] block: switch from g_slice allocator to malloc

[Qemu-devel] [PATCH] block: switch from g_slice allocator to malloc

[Paolo Bonzini]

Simplify memory allocation by sticking with a single API.  GSlice
is not that fast anyway (tcmalloc/jemalloc are better).

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/io.c            | 4 ++--
 block/mirror.c        | 4 ++--
 block/raw-posix.c     | 8 ++++----
 block/raw-win32.c     | 4 ++--
 hw/block/virtio-blk.c | 4 ++--
 5 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/block/io.c b/block/io.c
index 94e18e6..17293c3 100644
--- a/block/io.c
+++ b/block/io.c
@@ -2218,7 +2218,7 @@ void *qemu_aio_get(const AIOCBInfo *aiocb_info, BlockDriverState *bs,
 {
     BlockAIOCB *acb;
 
-    acb = g_slice_alloc(aiocb_info->aiocb_size);
+    acb = g_malloc(aiocb_info->aiocb_size);
     acb->aiocb_info = aiocb_info;
     acb->bs = bs;
     acb->cb = cb;
@@ -2238,7 +2238,7 @@ void qemu_aio_unref(void *p)
     BlockAIOCB *acb = p;
     assert(acb->refcnt > 0);
     if (--acb->refcnt == 0) {
-        g_slice_free1(acb->aiocb_info->aiocb_size, acb);
+        g_free(acb);
     }
 }
 
diff --git a/block/mirror.c b/block/mirror.c
index a258926..c4cf851 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -113,7 +113,7 @@ static void mirror_iteration_done(MirrorOp *op, int ret)
     }
 
     qemu_iovec_destroy(&op->qiov);
-    g_slice_free(MirrorOp, op);
+    g_free(op);
 
     if (s->waiting_for_io) {
         qemu_coroutine_enter(s->common.co, NULL);
@@ -264,7 +264,7 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
     } while (delay_ns == 0 && next_sector < end);
 
     /* Allocate a MirrorOp that is used as an AIO callback.  */
-    op = g_slice_new(MirrorOp);
+    op = g_new(MirrorOp, 1);
     op->s = s;
     op->sector_num = sector_num;
     op->nb_sectors = nb_sectors;
diff --git a/block/raw-posix.c b/block/raw-posix.c
index 30df8ad..a5f9707 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -1259,7 +1259,7 @@ static int aio_worker(void *arg)
         break;
     }
 
-    g_slice_free(RawPosixAIOData, aiocb);
+    g_free(aiocb);
     return ret;
 }
 
@@ -1267,7 +1267,7 @@ static int paio_submit_co(BlockDriverState *bs, int fd,
         int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,
         int type)
 {
-    RawPosixAIOData *acb = g_slice_new(RawPosixAIOData);
+    RawPosixAIOData *acb = g_new(RawPosixAIOData, 1);
     ThreadPool *pool;
 
     acb->bs = bs;
@@ -1292,7 +1292,7 @@ static BlockAIOCB *paio_submit(BlockDriverState *bs, int fd,
         int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,
         BlockCompletionFunc *cb, void *opaque, int type)
 {
-    RawPosixAIOData *acb = g_slice_new(RawPosixAIOData);
+    RawPosixAIOData *acb = g_new(RawPosixAIOData, 1);
     ThreadPool *pool;
 
     acb->bs = bs;
@@ -2237,7 +2237,7 @@ static BlockAIOCB *hdev_aio_ioctl(BlockDriverState *bs,
     if (fd_open(bs) < 0)
         return NULL;
 
-    acb = g_slice_new(RawPosixAIOData);
+    acb = g_new(RawPosixAIOData, 1);
     acb->bs = bs;
     acb->aio_type = QEMU_AIO_IOCTL;
     acb->aio_fildes = s->fd;
diff --git a/block/raw-win32.c b/block/raw-win32.c
index 68f2338..20a5332 100644
--- a/block/raw-win32.c
+++ b/block/raw-win32.c
@@ -135,7 +135,7 @@ static int aio_worker(void *arg)
         break;
     }
 
-    g_slice_free(RawWin32AIOData, aiocb);
+    g_free(aiocb);
     return ret;
 }
 
@@ -143,7 +143,7 @@ static BlockAIOCB *paio_submit(BlockDriverState *bs, HANDLE hfile,
         int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,
         BlockCompletionFunc *cb, void *opaque, int type)
 {
-    RawWin32AIOData *acb = g_slice_new(RawWin32AIOData);
+    RawWin32AIOData *acb = g_new(RawWin32AIOData, 1);
     ThreadPool *pool;
 
     acb->bs = bs;
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index f9301ae..c6ad2c9 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -30,7 +30,7 @@
 
 VirtIOBlockReq *virtio_blk_alloc_request(VirtIOBlock *s)
 {
-    VirtIOBlockReq *req = g_slice_new(VirtIOBlockReq);
+    VirtIOBlockReq *req = g_new(VirtIOBlockReq, 1);
     req->dev = s;
     req->qiov.size = 0;
     req->in_len = 0;
@@ -42,7 +42,7 @@ VirtIOBlockReq *virtio_blk_alloc_request(VirtIOBlock *s)
 void virtio_blk_free_request(VirtIOBlockReq *req)
 {
     if (req) {
-        g_slice_free(VirtIOBlockReq, req);
+        g_free(req);
     }
 }
 
-- 
2.5.0

Re: [Qemu-devel] [PATCH] block: switch from g_slice allocator to malloc

[Stefan Hajnoczi]

On Thu, Oct 01, 2015 at 01:04:39PM +0200, Paolo Bonzini wrote:
> Simplify memory allocation by sticking with a single API.  GSlice
> is not that fast anyway (tcmalloc/jemalloc are better).
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  block/io.c            | 4 ++--
>  block/mirror.c        | 4 ++--
>  block/raw-posix.c     | 8 ++++----
>  block/raw-win32.c     | 4 ++--
>  hw/block/virtio-blk.c | 4 ++--
>  5 files changed, 12 insertions(+), 12 deletions(-)

Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan

Re: [Qemu-devel] [Qemu-block] [PATCH] block: switch from g_slice allocator to malloc

[Kevin Wolf]

Am 08.10.2015 um 10:54 hat Stefan Hajnoczi geschrieben:
> On Thu, Oct 01, 2015 at 01:04:39PM +0200, Paolo Bonzini wrote:
> > Simplify memory allocation by sticking with a single API.  GSlice
> > is not that fast anyway (tcmalloc/jemalloc are better).
> > 
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > ---
> >  block/io.c            | 4 ++--
> >  block/mirror.c        | 4 ++--
> >  block/raw-posix.c     | 8 ++++----
> >  block/raw-win32.c     | 4 ++--
> >  hw/block/virtio-blk.c | 4 ++--
> >  5 files changed, 12 insertions(+), 12 deletions(-)
> 
> Thanks, applied to my block tree:
> https://github.com/stefanha/qemu/commits/block

Has someone benchmarked this before applying? Just claiming "wasn't fast
anyway" doesn't generally seem sufficient for changes to the I/O path.

Kevin

Re: [Qemu-devel] [PATCH] block: switch from g_slice allocator to malloc

[Paolo Bonzini]

On 08/10/2015 11:41, Kevin Wolf wrote:
> Am 08.10.2015 um 10:54 hat Stefan Hajnoczi geschrieben:
>> On Thu, Oct 01, 2015 at 01:04:39PM +0200, Paolo Bonzini wrote:
>>> Simplify memory allocation by sticking with a single API.  GSlice
>>> is not that fast anyway (tcmalloc/jemalloc are better).
>>>
>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>> ---
>>>  block/io.c            | 4 ++--
>>>  block/mirror.c        | 4 ++--
>>>  block/raw-posix.c     | 8 ++++----
>>>  block/raw-win32.c     | 4 ++--
>>>  hw/block/virtio-blk.c | 4 ++--
>>>  5 files changed, 12 insertions(+), 12 deletions(-)
>>
>> Thanks, applied to my block tree:
>> https://github.com/stefanha/qemu/commits/block
> 
> Has someone benchmarked this before applying? Just claiming "wasn't fast
> anyway" doesn't generally seem sufficient for changes to the I/O path.

I did it about six months ago.  Sorry for not digging up the results
when posting:

baseline: 193 kiops
tcmalloc: 202 kiops
tcmalloc + G_SLICE=always-malloc: 210 kiops

Paolo

Re: [Qemu-devel] [PATCH] block: switch from g_slice allocator to malloc

[Kevin Wolf]

Am 09.10.2015 um 09:49 hat Paolo Bonzini geschrieben:
> On 08/10/2015 11:41, Kevin Wolf wrote:
> > Am 08.10.2015 um 10:54 hat Stefan Hajnoczi geschrieben:
> >> On Thu, Oct 01, 2015 at 01:04:39PM +0200, Paolo Bonzini wrote:
> >>> Simplify memory allocation by sticking with a single API.  GSlice
> >>> is not that fast anyway (tcmalloc/jemalloc are better).
> >>>
> >>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> >>> ---
> >>>  block/io.c            | 4 ++--
> >>>  block/mirror.c        | 4 ++--
> >>>  block/raw-posix.c     | 8 ++++----
> >>>  block/raw-win32.c     | 4 ++--
> >>>  hw/block/virtio-blk.c | 4 ++--
> >>>  5 files changed, 12 insertions(+), 12 deletions(-)
> >>
> >> Thanks, applied to my block tree:
> >> https://github.com/stefanha/qemu/commits/block
> > 
> > Has someone benchmarked this before applying? Just claiming "wasn't fast
> > anyway" doesn't generally seem sufficient for changes to the I/O path.
> 
> I did it about six months ago.  Sorry for not digging up the results
> when posting:
> 
> baseline: 193 kiops
> tcmalloc: 202 kiops
> tcmalloc + G_SLICE=always-malloc: 210 kiops

Thanks. Do you have numbers for g_malloc + G_SLICE=always-malloc, too?
Because I think that's our new default after this patch. tcmalloc must
still be enabled manually.

Kevin

Re: [Qemu-devel] [PATCH] block: switch from g_slice allocator to malloc

[Paolo Bonzini]

On 09/10/2015 10:15, Kevin Wolf wrote:
> > I did it about six months ago.  Sorry for not digging up the results
> > when posting:
> > 
> > baseline: 193 kiops
> > tcmalloc: 202 kiops
> > tcmalloc + G_SLICE=always-malloc: 210 kiops
> 
> Thanks. Do you have numbers for g_malloc + G_SLICE=always-malloc, too?
> Because I think that's our new default after this patch. tcmalloc must
> still be enabled manually.

No, but I remember that it was a wash.

I don't recall exactly, but either g_slice was a bit faster at freeing
and a bit slower at allocating, or it was the other way round.

Paolo