diff mbox series

virtiofsd: Used glib "shared" thread pool

Message ID 20200921213216.GE13362@redhat.com
State New
Headers show
Series virtiofsd: Used glib "shared" thread pool | expand

Commit Message

Vivek Goyal Sept. 21, 2020, 9:32 p.m. UTC
glib offers thread pools and it seems to support "exclusive" and "shared"
thread pools.

https://developer.gnome.org/glib/stable/glib-Thread-Pools.html#g-thread-pool-new

Currently we use "exlusive" thread pools but its performance seems to be
poor. I tried using "shared" thread pools and performance seems much
better. I posted performance results here.

https://www.redhat.com/archives/virtio-fs/2020-September/msg00080.html

So lets switch to shared thread pools. We can think of making it optional
once somebody can show in what cases exclusive thread pools offer better
results. For now, my simple performance tests across the board see
better results with shared thread pools.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 tools/virtiofsd/fuse_virtio.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Miklos Szeredi Sept. 22, 2020, 12:03 p.m. UTC | #1
On Mon, Sep 21, 2020 at 11:32 PM Vivek Goyal <vgoyal@redhat.com> wrote:

> glib offers thread pools and it seems to support "exclusive" and "shared"
> thread pools.
>
>
> https://developer.gnome.org/glib/stable/glib-Thread-Pools.html#g-thread-pool-new
>
> Currently we use "exlusive" thread pools but its performance seems to be
> poor. I tried using "shared" thread pools and performance seems much
> better. I posted performance results here.
>
> https://www.redhat.com/archives/virtio-fs/2020-September/msg00080.html
>
> So lets switch to shared thread pools. We can think of making it optional
> once somebody can show in what cases exclusive thread pools offer better
> results. For now, my simple performance tests across the board see
> better results with shared thread pools.
>

Needs this as well:

--- qemu.orig/tools/virtiofsd/passthrough_seccomp.c 2020-09-16
20:21:13.168686176 +0200
+++ qemu/tools/virtiofsd/passthrough_seccomp.c 2020-09-22
14:01:38.499164501 +0200
@@ -94,6 +94,8 @@ static const int syscall_whitelist[] = {
     SCMP_SYS(rt_sigaction),
     SCMP_SYS(rt_sigprocmask),
     SCMP_SYS(rt_sigreturn),
+    SCMP_SYS(sched_getattr),
+    SCMP_SYS(sched_setattr),
     SCMP_SYS(sendmsg),
     SCMP_SYS(setresgid),
     SCMP_SYS(setresuid),

Thanks,
Miklos
<div dir="ltr"><div dir="ltr"><div class="gmail_default" style="font-size:small"><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Sep 21, 2020 at 11:32 PM Vivek Goyal &lt;<a href="mailto:vgoyal@redhat.com">vgoyal@redhat.com</a>&gt; wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">glib offers thread pools and it seems to support &quot;exclusive&quot; and &quot;shared&quot;<br>
thread pools.<br>
<br>
<a href="https://developer.gnome.org/glib/stable/glib-Thread-Pools.html#g-thread-pool-new" rel="noreferrer" target="_blank">https://developer.gnome.org/glib/stable/glib-Thread-Pools.html#g-thread-pool-new</a><br>
<br>
Currently we use &quot;exlusive&quot; thread pools but its performance seems to be<br>
poor. I tried using &quot;shared&quot; thread pools and performance seems much<br>
better. I posted performance results here.<br>
<br>
<a href="https://www.redhat.com/archives/virtio-fs/2020-September/msg00080.html" rel="noreferrer" target="_blank">https://www.redhat.com/archives/virtio-fs/2020-September/msg00080.html</a><br>
<br>
So lets switch to shared thread pools. We can think of making it optional<br>
once somebody can show in what cases exclusive thread pools offer better<br>
results. For now, my simple performance tests across the board see<br>
better results with shared thread pools.<br></blockquote><div><br></div><div style="font-size:small" class="gmail_default">Needs this as well:</div><div style="font-size:small" class="gmail_default"><br></div><div style="font-size:small" class="gmail_default">--- qemu.orig/tools/virtiofsd/passthrough_seccomp.c	2020-09-16 20:21:13.168686176 +0200<br>+++ qemu/tools/virtiofsd/passthrough_seccomp.c	2020-09-22 14:01:38.499164501 +0200<br>@@ -94,6 +94,8 @@ static const int syscall_whitelist[] = {<br>     SCMP_SYS(rt_sigaction),<br>     SCMP_SYS(rt_sigprocmask),<br>     SCMP_SYS(rt_sigreturn),<br>+    SCMP_SYS(sched_getattr),<br>+    SCMP_SYS(sched_setattr),<br>     SCMP_SYS(sendmsg),<br>     SCMP_SYS(setresgid),<br>     SCMP_SYS(setresuid),<br></div></div><div class="gmail_quote"><div style="font-size:small" class="gmail_default"><br></div><div style="font-size:small" class="gmail_default">Thanks,</div><div style="font-size:small" class="gmail_default">Miklos</div><br></div></div>
Vivek Goyal Sept. 22, 2020, 12:40 p.m. UTC | #2
On Tue, Sep 22, 2020 at 02:03:18PM +0200, Miklos Szeredi wrote:
> On Mon, Sep 21, 2020 at 11:32 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> 
> > glib offers thread pools and it seems to support "exclusive" and "shared"
> > thread pools.
> >
> >
> > https://developer.gnome.org/glib/stable/glib-Thread-Pools.html#g-thread-pool-new
> >
> > Currently we use "exlusive" thread pools but its performance seems to be
> > poor. I tried using "shared" thread pools and performance seems much
> > better. I posted performance results here.
> >
> > https://www.redhat.com/archives/virtio-fs/2020-September/msg00080.html
> >
> > So lets switch to shared thread pools. We can think of making it optional
> > once somebody can show in what cases exclusive thread pools offer better
> > results. For now, my simple performance tests across the board see
> > better results with shared thread pools.
> >
> 
> Needs this as well:

Thanks Miklos. I am wondering why I did not face this issue. May be
I don't have seccomp enabled. Will check.

David, can you please pick this patch as well. May be merge both
the patches.

Do let me know if you want to me post a merged patch instead.

Thanks
Vivek

> 
> --- qemu.orig/tools/virtiofsd/passthrough_seccomp.c 2020-09-16
> 20:21:13.168686176 +0200
> +++ qemu/tools/virtiofsd/passthrough_seccomp.c 2020-09-22
> 14:01:38.499164501 +0200
> @@ -94,6 +94,8 @@ static const int syscall_whitelist[] = {
>      SCMP_SYS(rt_sigaction),
>      SCMP_SYS(rt_sigprocmask),
>      SCMP_SYS(rt_sigreturn),
> +    SCMP_SYS(sched_getattr),
> +    SCMP_SYS(sched_setattr),
>      SCMP_SYS(sendmsg),
>      SCMP_SYS(setresgid),
>      SCMP_SYS(setresuid),
> 
> Thanks,
> Miklos
Daniel P. Berrangé Sept. 22, 2020, 12:59 p.m. UTC | #3
On Mon, Sep 21, 2020 at 05:32:16PM -0400, Vivek Goyal wrote:
> glib offers thread pools and it seems to support "exclusive" and "shared"

> thread pools.

> 

> https://developer.gnome.org/glib/stable/glib-Thread-Pools.html#g-thread-pool-new

> 

> Currently we use "exlusive" thread pools but its performance seems to be

> poor. I tried using "shared" thread pools and performance seems much

> better. I posted performance results here.

> 

> https://www.redhat.com/archives/virtio-fs/2020-September/msg00080.html

> 

> So lets switch to shared thread pools. We can think of making it optional

> once somebody can show in what cases exclusive thread pools offer better

> results. For now, my simple performance tests across the board see

> better results with shared thread pools.


I'm really curious why  there's any perf difference between shared and
exclusive thread pools in the GLib impl.

Looking at the code the main difference between the two is appears to
be around the way threads are spawned, specifically around the scheduler
attributes assigned.

In the shared case, the threads in the pool will have their scheduler
attributes copied from the very first thread that calls g_thread_pool_new.

In the exclusive case, the threads in the pool will inherit their
scheduler attributes from the thread which pushs the job that
causes the worker thread to be created.

By schedular attributes, I mean all the items in the 'struct schedattr'
filled by sched_getattr()

IOW, if threads in virtiofsd have varying schedular attributes this
could possibly explain the difference in performance you see between
the two setups.

> 

> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>

> ---

>  tools/virtiofsd/fuse_virtio.c |    2 +-

>  1 file changed, 1 insertion(+), 1 deletion(-)

> 

> Index: qemu/tools/virtiofsd/fuse_virtio.c

> ===================================================================

> --- qemu.orig/tools/virtiofsd/fuse_virtio.c	2020-09-21 17:28:27.444438015 -0400

> +++ qemu/tools/virtiofsd/fuse_virtio.c	2020-09-21 17:28:30.584568910 -0400

> @@ -695,7 +695,7 @@ static void *fv_queue_thread(void *opaqu

>      struct fuse_session *se = qi->virtio_dev->se;

>      GThreadPool *pool;

>  

> -    pool = g_thread_pool_new(fv_queue_worker, qi, se->thread_pool_size, TRUE,

> +    pool = g_thread_pool_new(fv_queue_worker, qi, se->thread_pool_size, FALSE,

>                               NULL);

>      if (!pool) {

>          fuse_log(FUSE_LOG_ERR, "%s: g_thread_pool_new failed\n", __func__);

> 

> 


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
Vivek Goyal Sept. 22, 2020, 5:29 p.m. UTC | #4
On Tue, Sep 22, 2020 at 02:03:18PM +0200, Miklos Szeredi wrote:
> On Mon, Sep 21, 2020 at 11:32 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> 
> > glib offers thread pools and it seems to support "exclusive" and "shared"
> > thread pools.
> >
> >
> > https://developer.gnome.org/glib/stable/glib-Thread-Pools.html#g-thread-pool-new
> >
> > Currently we use "exlusive" thread pools but its performance seems to be
> > poor. I tried using "shared" thread pools and performance seems much
> > better. I posted performance results here.
> >
> > https://www.redhat.com/archives/virtio-fs/2020-September/msg00080.html
> >
> > So lets switch to shared thread pools. We can think of making it optional
> > once somebody can show in what cases exclusive thread pools offer better
> > results. For now, my simple performance tests across the board see
> > better results with shared thread pools.
> >
> 
> Needs this as well:

I was wondering why I did not face this issue. I think my glib is old
(glib2, 2.58.3) from fedora 29 and this change about sched_getattr seems
relatively recent.

commit 8aeca4fa647bfd0f35c4a86b1e6ca6e955519ca5
Author: Sebastian Dröge <sebastian@centricular.com>
Date:   Tue Dec 24 15:33:30 2019 +0200

    GThreadPool - Don't inherit thread priorities when creating new threads

I think my glib2 does not have this change and probably that's why
seccomp did not trigger.

Vivek

> 
> --- qemu.orig/tools/virtiofsd/passthrough_seccomp.c 2020-09-16
> 20:21:13.168686176 +0200
> +++ qemu/tools/virtiofsd/passthrough_seccomp.c 2020-09-22
> 14:01:38.499164501 +0200
> @@ -94,6 +94,8 @@ static const int syscall_whitelist[] = {
>      SCMP_SYS(rt_sigaction),
>      SCMP_SYS(rt_sigprocmask),
>      SCMP_SYS(rt_sigreturn),
> +    SCMP_SYS(sched_getattr),
> +    SCMP_SYS(sched_setattr),
>      SCMP_SYS(sendmsg),
>      SCMP_SYS(setresgid),
>      SCMP_SYS(setresuid),
> 
> Thanks,
> Miklos
Vivek Goyal Sept. 22, 2020, 5:42 p.m. UTC | #5
On Tue, Sep 22, 2020 at 01:59:57PM +0100, Daniel P. Berrangé wrote:
> On Mon, Sep 21, 2020 at 05:32:16PM -0400, Vivek Goyal wrote:

> > glib offers thread pools and it seems to support "exclusive" and "shared"

> > thread pools.

> > 

> > https://developer.gnome.org/glib/stable/glib-Thread-Pools.html#g-thread-pool-new

> > 

> > Currently we use "exlusive" thread pools but its performance seems to be

> > poor. I tried using "shared" thread pools and performance seems much

> > better. I posted performance results here.

> > 

> > https://www.redhat.com/archives/virtio-fs/2020-September/msg00080.html

> > 

> > So lets switch to shared thread pools. We can think of making it optional

> > once somebody can show in what cases exclusive thread pools offer better

> > results. For now, my simple performance tests across the board see

> > better results with shared thread pools.

> 

> I'm really curious why  there's any perf difference between shared and

> exclusive thread pools in the GLib impl.

> 

> Looking at the code the main difference between the two is appears to

> be around the way threads are spawned, specifically around the scheduler

> attributes assigned.

> 

> In the shared case, the threads in the pool will have their scheduler

> attributes copied from the very first thread that calls g_thread_pool_new.

> 

> In the exclusive case, the threads in the pool will inherit their

> scheduler attributes from the thread which pushs the job that

> causes the worker thread to be created.

> 

> By schedular attributes, I mean all the items in the 'struct schedattr'

> filled by sched_getattr()

> 

> IOW, if threads in virtiofsd have varying schedular attributes this

> could possibly explain the difference in performance you see between

> the two setups.


Hi Daniel,

Few things.

- I think scheduler attributes are same for the thread creating
  pool as well as for thread pushing the job for virtiofsd.

- My glib2 is old (2.58.3) and I think that did not have sched_getattr()
  stuff.

- One difference I noticed is that in case of shared pool, it does not
  create extra threads if client is doing one request at a time. While
  exclusive pool seemed to push every request to a new thread in pool
  in sort of round robin fashion. It feels keeping requests being served
  from same thread helps in this particilar workload case.

Thanks
Vivek

> 

> > 

> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>

> > ---

> >  tools/virtiofsd/fuse_virtio.c |    2 +-

> >  1 file changed, 1 insertion(+), 1 deletion(-)

> > 

> > Index: qemu/tools/virtiofsd/fuse_virtio.c

> > ===================================================================

> > --- qemu.orig/tools/virtiofsd/fuse_virtio.c	2020-09-21 17:28:27.444438015 -0400

> > +++ qemu/tools/virtiofsd/fuse_virtio.c	2020-09-21 17:28:30.584568910 -0400

> > @@ -695,7 +695,7 @@ static void *fv_queue_thread(void *opaqu

> >      struct fuse_session *se = qi->virtio_dev->se;

> >      GThreadPool *pool;

> >  

> > -    pool = g_thread_pool_new(fv_queue_worker, qi, se->thread_pool_size, TRUE,

> > +    pool = g_thread_pool_new(fv_queue_worker, qi, se->thread_pool_size, FALSE,

> >                               NULL);

> >      if (!pool) {

> >          fuse_log(FUSE_LOG_ERR, "%s: g_thread_pool_new failed\n", __func__);

> > 

> > 

> 

> Regards,

> Daniel

> -- 

> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|

> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|

> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
Daniel P. Berrangé Sept. 22, 2020, 5:46 p.m. UTC | #6
On Tue, Sep 22, 2020 at 01:42:55PM -0400, Vivek Goyal wrote:
> On Tue, Sep 22, 2020 at 01:59:57PM +0100, Daniel P. Berrangé wrote:

> > On Mon, Sep 21, 2020 at 05:32:16PM -0400, Vivek Goyal wrote:

> > > glib offers thread pools and it seems to support "exclusive" and "shared"

> > > thread pools.

> > > 

> > > https://developer.gnome.org/glib/stable/glib-Thread-Pools.html#g-thread-pool-new

> > > 

> > > Currently we use "exlusive" thread pools but its performance seems to be

> > > poor. I tried using "shared" thread pools and performance seems much

> > > better. I posted performance results here.

> > > 

> > > https://www.redhat.com/archives/virtio-fs/2020-September/msg00080.html

> > > 

> > > So lets switch to shared thread pools. We can think of making it optional

> > > once somebody can show in what cases exclusive thread pools offer better

> > > results. For now, my simple performance tests across the board see

> > > better results with shared thread pools.

> > 

> > I'm really curious why  there's any perf difference between shared and

> > exclusive thread pools in the GLib impl.

> > 

> > Looking at the code the main difference between the two is appears to

> > be around the way threads are spawned, specifically around the scheduler

> > attributes assigned.

> > 

> > In the shared case, the threads in the pool will have their scheduler

> > attributes copied from the very first thread that calls g_thread_pool_new.

> > 

> > In the exclusive case, the threads in the pool will inherit their

> > scheduler attributes from the thread which pushs the job that

> > causes the worker thread to be created.

> > 

> > By schedular attributes, I mean all the items in the 'struct schedattr'

> > filled by sched_getattr()

> > 

> > IOW, if threads in virtiofsd have varying schedular attributes this

> > could possibly explain the difference in performance you see between

> > the two setups.

> 

> Hi Daniel,

> 

> Few things.

> 

> - I think scheduler attributes are same for the thread creating

>   pool as well as for thread pushing the job for virtiofsd.

> 

> - My glib2 is old (2.58.3) and I think that did not have sched_getattr()

>   stuff.

> 

> - One difference I noticed is that in case of shared pool, it does not

>   create extra threads if client is doing one request at a time. While

>   exclusive pool seemed to push every request to a new thread in pool

>   in sort of round robin fashion. It feels keeping requests being served

>   from same thread helps in this particilar workload case.


Yeah, that does sound like a candidate for the cause. I wonder if that
was intentional in the GLib design or just an accidental impl they didn't
realize had performance implications. Might be worth filing a bug against
GLib if someone has free time & motivation to figure out a standalone
reproducer to demonstrate the performance difference in the GLib APIs.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
Stefan Hajnoczi Sept. 23, 2020, 12:22 p.m. UTC | #7
On Mon, Sep 21, 2020 at 05:32:16PM -0400, Vivek Goyal wrote:
> glib offers thread pools and it seems to support "exclusive" and "shared"
> thread pools.
> 
> https://developer.gnome.org/glib/stable/glib-Thread-Pools.html#g-thread-pool-new
> 
> Currently we use "exlusive" thread pools but its performance seems to be
> poor. I tried using "shared" thread pools and performance seems much
> better. I posted performance results here.
> 
> https://www.redhat.com/archives/virtio-fs/2020-September/msg00080.html
> 
> So lets switch to shared thread pools. We can think of making it optional
> once somebody can show in what cases exclusive thread pools offer better
> results. For now, my simple performance tests across the board see
> better results with shared thread pools.
> 
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  tools/virtiofsd/fuse_virtio.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Index: qemu/tools/virtiofsd/fuse_virtio.c
> ===================================================================
> --- qemu.orig/tools/virtiofsd/fuse_virtio.c	2020-09-21 17:28:27.444438015 -0400
> +++ qemu/tools/virtiofsd/fuse_virtio.c	2020-09-21 17:28:30.584568910 -0400
> @@ -695,7 +695,7 @@ static void *fv_queue_thread(void *opaqu
>      struct fuse_session *se = qi->virtio_dev->se;
>      GThreadPool *pool;
>  
> -    pool = g_thread_pool_new(fv_queue_worker, qi, se->thread_pool_size, TRUE,
> +    pool = g_thread_pool_new(fv_queue_worker, qi, se->thread_pool_size, FALSE,
>                               NULL);
>      if (!pool) {
>          fuse_log(FUSE_LOG_ERR, "%s: g_thread_pool_new failed\n", __func__);

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Dr. David Alan Gilbert Sept. 24, 2020, 9:29 a.m. UTC | #8
* Vivek Goyal (vgoyal@redhat.com) wrote:
> glib offers thread pools and it seems to support "exclusive" and "shared"

> thread pools.

> 

> https://developer.gnome.org/glib/stable/glib-Thread-Pools.html#g-thread-pool-new

> 

> Currently we use "exlusive" thread pools but its performance seems to be

> poor. I tried using "shared" thread pools and performance seems much

> better. I posted performance results here.

> 

> https://www.redhat.com/archives/virtio-fs/2020-September/msg00080.html

> 

> So lets switch to shared thread pools. We can think of making it optional

> once somebody can show in what cases exclusive thread pools offer better

> results. For now, my simple performance tests across the board see

> better results with shared thread pools.

> 

> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>


Queued (with Miklos's seccomp fix); although my gut feeling is we'll be
coming back to this again to understand how the threading should work.

Dave

> ---

>  tools/virtiofsd/fuse_virtio.c |    2 +-

>  1 file changed, 1 insertion(+), 1 deletion(-)

> 

> Index: qemu/tools/virtiofsd/fuse_virtio.c

> ===================================================================

> --- qemu.orig/tools/virtiofsd/fuse_virtio.c	2020-09-21 17:28:27.444438015 -0400

> +++ qemu/tools/virtiofsd/fuse_virtio.c	2020-09-21 17:28:30.584568910 -0400

> @@ -695,7 +695,7 @@ static void *fv_queue_thread(void *opaqu

>      struct fuse_session *se = qi->virtio_dev->se;

>      GThreadPool *pool;

>  

> -    pool = g_thread_pool_new(fv_queue_worker, qi, se->thread_pool_size, TRUE,

> +    pool = g_thread_pool_new(fv_queue_worker, qi, se->thread_pool_size, FALSE,

>                               NULL);

>      if (!pool) {

>          fuse_log(FUSE_LOG_ERR, "%s: g_thread_pool_new failed\n", __func__);

-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
diff mbox series

Patch

Index: qemu/tools/virtiofsd/fuse_virtio.c
===================================================================
--- qemu.orig/tools/virtiofsd/fuse_virtio.c	2020-09-21 17:28:27.444438015 -0400
+++ qemu/tools/virtiofsd/fuse_virtio.c	2020-09-21 17:28:30.584568910 -0400
@@ -695,7 +695,7 @@  static void *fv_queue_thread(void *opaqu
     struct fuse_session *se = qi->virtio_dev->se;
     GThreadPool *pool;
 
-    pool = g_thread_pool_new(fv_queue_worker, qi, se->thread_pool_size, TRUE,
+    pool = g_thread_pool_new(fv_queue_worker, qi, se->thread_pool_size, FALSE,
                              NULL);
     if (!pool) {
         fuse_log(FUSE_LOG_ERR, "%s: g_thread_pool_new failed\n", __func__);