* [Virtio-fs] [RFC][PATCH][v3] virtiofsd: virtiofsd: Use thread pool only after certain queue depth
@ 2021-05-04 15:18 Vivek Goyal
2021-05-04 15:54 ` Greg Kurz
0 siblings, 1 reply; 7+ messages in thread
From: Vivek Goyal @ 2021-05-04 15:18 UTC (permalink / raw)
To: virtio-fs-list
For low queue depth workloads, inline processing works well. But for
high queue depth (and multiple process/thread) workloads, parallel
processing of thread pool can benefit.
This patch provides a knob --thread-pool-threshold=<nr-requests>, which
uses a thread pool only if number of requests on virtqueue are more
than nr-requests. IOW, upto nr-requests, requests are processed inline
and anything more than nr-requests is sent for processing in thread
pool.
I have got good results with "--thread-pool-size=16 --thread-pool-threshold=3"
on my system.
Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
Test results with this patch are available here.
https://github.com/rhvgoyal/virtiofs-tests/tree/master/performance-results/thread-pool-threshold/4-may-2021
Changes since v2:
- Renamed knob name to "--thread-pool-threshold"
- Started using GQueue instead of GList to keep a list of requests
received on virtqueue (Greg)
- When threshold is crossed only requests above threshold are sent to
thread pool. Rest are still processed inline. (Greg)
---
tools/virtiofsd/fuse_i.h | 1 +
tools/virtiofsd/fuse_lowlevel.c | 8 +++++++-
tools/virtiofsd/fuse_virtio.c | 27 +++++++++++++--------------
3 files changed, 21 insertions(+), 15 deletions(-)
Index: rhvgoyal-qemu/tools/virtiofsd/fuse_virtio.c
===================================================================
--- rhvgoyal-qemu.orig/tools/virtiofsd/fuse_virtio.c 2021-05-04 09:55:30.467514740 -0400
+++ rhvgoyal-qemu/tools/virtiofsd/fuse_virtio.c 2021-05-04 10:06:32.453801299 -0400
@@ -445,7 +445,6 @@ err:
static __thread bool clone_fs_called;
-/* Process one FVRequest in a thread pool */
static void fv_queue_worker(gpointer data, gpointer user_data)
{
struct fv_QueueInfo *qi = user_data;
@@ -604,11 +603,12 @@ static void *fv_queue_thread(void *opaqu
struct VuVirtq *q = vu_get_queue(dev, qi->qidx);
struct fuse_session *se = qi->virtio_dev->se;
GThreadPool *pool = NULL;
- GList *req_list = NULL;
+ GQueue req_queue = G_QUEUE_INIT;
if (se->thread_pool_size) {
- fuse_log(FUSE_LOG_DEBUG, "%s: Creating thread pool for Queue %d\n",
- __func__, qi->qidx);
+ fuse_log(FUSE_LOG_DEBUG, "%s: Creating thread pool for Queue %d."
+ " thread_pool_threshold=%u\n", __func__, qi->qidx,
+ se->thread_pool_threshold);
pool = g_thread_pool_new(fv_queue_worker, qi, se->thread_pool_size,
FALSE, NULL);
if (!pool) {
@@ -686,22 +686,21 @@ static void *fv_queue_thread(void *opaqu
}
req->reply_sent = false;
-
- if (!se->thread_pool_size) {
- req_list = g_list_prepend(req_list, req);
- } else {
- g_thread_pool_push(pool, req, NULL);
- }
+ g_queue_push_tail(&req_queue, req);
}
pthread_mutex_unlock(&qi->vq_lock);
vu_dispatch_unlock(qi->virtio_dev);
/* Process all the requests. */
- if (!se->thread_pool_size && req_list != NULL) {
- g_list_foreach(req_list, fv_queue_worker, qi);
- g_list_free(req_list);
- req_list = NULL;
+ for (int i = g_queue_get_length(&req_queue); i > 0; i--) {
+ FVRequest *req = g_queue_pop_head(&req_queue);
+
+ if (se->thread_pool_size && i > se->thread_pool_threshold) {
+ g_thread_pool_push(pool, req, NULL);
+ } else {
+ fv_queue_worker(req, qi);
+ }
}
}
Index: rhvgoyal-qemu/tools/virtiofsd/fuse_i.h
===================================================================
--- rhvgoyal-qemu.orig/tools/virtiofsd/fuse_i.h 2021-05-04 09:55:30.467514740 -0400
+++ rhvgoyal-qemu/tools/virtiofsd/fuse_i.h 2021-05-04 10:04:58.737938382 -0400
@@ -73,6 +73,7 @@ struct fuse_session {
int vu_socketfd;
struct fv_VuDev *virtio_dev;
int thread_pool_size;
+ unsigned thread_pool_threshold;
};
struct fuse_chan {
Index: rhvgoyal-qemu/tools/virtiofsd/fuse_lowlevel.c
===================================================================
--- rhvgoyal-qemu.orig/tools/virtiofsd/fuse_lowlevel.c 2021-05-04 09:55:30.467514740 -0400
+++ rhvgoyal-qemu/tools/virtiofsd/fuse_lowlevel.c 2021-05-04 10:17:27.674809167 -0400
@@ -2432,6 +2432,7 @@ static const struct fuse_opt fuse_ll_opt
LL_OPTION("--socket-group=%s", vu_socket_group, 0),
LL_OPTION("--fd=%d", vu_listen_fd, 0),
LL_OPTION("--thread-pool-size=%d", thread_pool_size, 0),
+ LL_OPTION("--thread-pool-threshold=%u", thread_pool_threshold, 0),
FUSE_OPT_END
};
@@ -2452,7 +2453,11 @@ void fuse_lowlevel_help(void)
" --socket-path=PATH path for the vhost-user socket\n"
" --socket-group=GRNAME name of group for the vhost-user socket\n"
" --fd=FDNUM fd number of vhost-user socket\n"
- " --thread-pool-size=NUM thread pool size limit (default %d)\n",
+ " --thread-pool-size=NUM thread pool size limit (default %d)\n"
+ " --thread-pool-threshold=NUM\n"
+ " number of request threshold to\n"
+ " switch between inline and thread pool\n"
+ " processing of request\n",
THREAD_POOL_SIZE);
}
@@ -2508,6 +2513,7 @@ struct fuse_session *fuse_session_new(st
se->fd = -1;
se->vu_listen_fd = -1;
se->thread_pool_size = THREAD_POOL_SIZE;
+ se->thread_pool_threshold = 0;
se->conn.max_write = UINT_MAX;
se->conn.max_readahead = UINT_MAX;
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Virtio-fs] [RFC][PATCH][v3] virtiofsd: virtiofsd: Use thread pool only after certain queue depth
2021-05-04 15:18 [Virtio-fs] [RFC][PATCH][v3] virtiofsd: virtiofsd: Use thread pool only after certain queue depth Vivek Goyal
@ 2021-05-04 15:54 ` Greg Kurz
2021-05-04 16:09 ` Vivek Goyal
0 siblings, 1 reply; 7+ messages in thread
From: Greg Kurz @ 2021-05-04 15:54 UTC (permalink / raw)
To: Vivek Goyal; +Cc: virtio-fs-list
On Tue, 4 May 2021 11:18:41 -0400
Vivek Goyal <vgoyal@redhat.com> wrote:
> For low queue depth workloads, inline processing works well. But for
> high queue depth (and multiple process/thread) workloads, parallel
> processing of thread pool can benefit.
>
> This patch provides a knob --thread-pool-threshold=<nr-requests>, which
> uses a thread pool only if number of requests on virtqueue are more
> than nr-requests. IOW, upto nr-requests, requests are processed inline
> and anything more than nr-requests is sent for processing in thread
> pool.
>
> I have got good results with "--thread-pool-size=16 --thread-pool-threshold=3"
> on my system.
>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>
> Test results with this patch are available here.
>
> https://github.com/rhvgoyal/virtiofs-tests/tree/master/performance-results/thread-pool-threshold/4-may-2021
>
> Changes since v2:
> - Renamed knob name to "--thread-pool-threshold"
> - Started using GQueue instead of GList to keep a list of requests
> received on virtqueue (Greg)
> - When threshold is crossed only requests above threshold are sent to
> thread pool. Rest are still processed inline. (Greg)
> ---
> tools/virtiofsd/fuse_i.h | 1 +
> tools/virtiofsd/fuse_lowlevel.c | 8 +++++++-
> tools/virtiofsd/fuse_virtio.c | 27 +++++++++++++--------------
> 3 files changed, 21 insertions(+), 15 deletions(-)
>
> Index: rhvgoyal-qemu/tools/virtiofsd/fuse_virtio.c
> ===================================================================
> --- rhvgoyal-qemu.orig/tools/virtiofsd/fuse_virtio.c 2021-05-04 09:55:30.467514740 -0400
> +++ rhvgoyal-qemu/tools/virtiofsd/fuse_virtio.c 2021-05-04 10:06:32.453801299 -0400
> @@ -445,7 +445,6 @@ err:
>
> static __thread bool clone_fs_called;
>
> -/* Process one FVRequest in a thread pool */
> static void fv_queue_worker(gpointer data, gpointer user_data)
> {
> struct fv_QueueInfo *qi = user_data;
> @@ -604,11 +603,12 @@ static void *fv_queue_thread(void *opaqu
> struct VuVirtq *q = vu_get_queue(dev, qi->qidx);
> struct fuse_session *se = qi->virtio_dev->se;
> GThreadPool *pool = NULL;
> - GList *req_list = NULL;
> + GQueue req_queue = G_QUEUE_INIT;
>
> if (se->thread_pool_size) {
> - fuse_log(FUSE_LOG_DEBUG, "%s: Creating thread pool for Queue %d\n",
> - __func__, qi->qidx);
> + fuse_log(FUSE_LOG_DEBUG, "%s: Creating thread pool for Queue %d."
> + " thread_pool_threshold=%u\n", __func__, qi->qidx,
> + se->thread_pool_threshold);
> pool = g_thread_pool_new(fv_queue_worker, qi, se->thread_pool_size,
> FALSE, NULL);
> if (!pool) {
> @@ -686,22 +686,21 @@ static void *fv_queue_thread(void *opaqu
> }
>
> req->reply_sent = false;
> -
> - if (!se->thread_pool_size) {
> - req_list = g_list_prepend(req_list, req);
> - } else {
> - g_thread_pool_push(pool, req, NULL);
> - }
> + g_queue_push_tail(&req_queue, req);
So here you replace prepend() with push_tail() but...
> }
>
> pthread_mutex_unlock(&qi->vq_lock);
> vu_dispatch_unlock(qi->virtio_dev);
>
> /* Process all the requests. */
> - if (!se->thread_pool_size && req_list != NULL) {
> - g_list_foreach(req_list, fv_queue_worker, qi);
> - g_list_free(req_list);
> - req_list = NULL;
> + for (int i = g_queue_get_length(&req_queue); i > 0; i--) {
> + FVRequest *req = g_queue_pop_head(&req_queue);
... this pops from the head. Isn't this reversing the order in which
requests are processed ? Not very harmful I guess.
> +
> + if (se->thread_pool_size && i > se->thread_pool_threshold) {
requests are pushed to the thread pool first, good. :)
> + g_thread_pool_push(pool, req, NULL);
> + } else {
> + fv_queue_worker(req, qi);
> + }
> }
> }
>
> Index: rhvgoyal-qemu/tools/virtiofsd/fuse_i.h
> ===================================================================
> --- rhvgoyal-qemu.orig/tools/virtiofsd/fuse_i.h 2021-05-04 09:55:30.467514740 -0400
> +++ rhvgoyal-qemu/tools/virtiofsd/fuse_i.h 2021-05-04 10:04:58.737938382 -0400
> @@ -73,6 +73,7 @@ struct fuse_session {
> int vu_socketfd;
> struct fv_VuDev *virtio_dev;
> int thread_pool_size;
> + unsigned thread_pool_threshold;
> };
>
> struct fuse_chan {
> Index: rhvgoyal-qemu/tools/virtiofsd/fuse_lowlevel.c
> ===================================================================
> --- rhvgoyal-qemu.orig/tools/virtiofsd/fuse_lowlevel.c 2021-05-04 09:55:30.467514740 -0400
> +++ rhvgoyal-qemu/tools/virtiofsd/fuse_lowlevel.c 2021-05-04 10:17:27.674809167 -0400
> @@ -2432,6 +2432,7 @@ static const struct fuse_opt fuse_ll_opt
> LL_OPTION("--socket-group=%s", vu_socket_group, 0),
> LL_OPTION("--fd=%d", vu_listen_fd, 0),
> LL_OPTION("--thread-pool-size=%d", thread_pool_size, 0),
> + LL_OPTION("--thread-pool-threshold=%u", thread_pool_threshold, 0),
> FUSE_OPT_END
> };
>
> @@ -2452,7 +2453,11 @@ void fuse_lowlevel_help(void)
> " --socket-path=PATH path for the vhost-user socket\n"
> " --socket-group=GRNAME name of group for the vhost-user socket\n"
> " --fd=FDNUM fd number of vhost-user socket\n"
> - " --thread-pool-size=NUM thread pool size limit (default %d)\n",
> + " --thread-pool-size=NUM thread pool size limit (default %d)\n"
> + " --thread-pool-threshold=NUM\n"
> + " number of request threshold to\n"
> + " switch between inline and thread pool\n"
> + " processing of request\n",
The number of requests the queue thread pops out from the virtqueue
looks a bit cryptic from a user point of view IMHO. Is this really
something we want to expose in the API ? Shouldn't this be something
we want to only change internally depending one some heuristics in
the future ?
> THREAD_POOL_SIZE);
> }
>
> @@ -2508,6 +2513,7 @@ struct fuse_session *fuse_session_new(st
> se->fd = -1;
> se->vu_listen_fd = -1;
> se->thread_pool_size = THREAD_POOL_SIZE;
> + se->thread_pool_threshold = 0;
> se->conn.max_write = UINT_MAX;
> se->conn.max_readahead = UINT_MAX;
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Virtio-fs] [RFC][PATCH][v3] virtiofsd: virtiofsd: Use thread pool only after certain queue depth
2021-05-04 15:54 ` Greg Kurz
@ 2021-05-04 16:09 ` Vivek Goyal
2021-05-04 16:34 ` Greg Kurz
0 siblings, 1 reply; 7+ messages in thread
From: Vivek Goyal @ 2021-05-04 16:09 UTC (permalink / raw)
To: Greg Kurz; +Cc: virtio-fs-list
On Tue, May 04, 2021 at 05:54:38PM +0200, Greg Kurz wrote:
> On Tue, 4 May 2021 11:18:41 -0400
> Vivek Goyal <vgoyal@redhat.com> wrote:
>
> > For low queue depth workloads, inline processing works well. But for
> > high queue depth (and multiple process/thread) workloads, parallel
> > processing of thread pool can benefit.
> >
> > This patch provides a knob --thread-pool-threshold=<nr-requests>, which
> > uses a thread pool only if number of requests on virtqueue are more
> > than nr-requests. IOW, upto nr-requests, requests are processed inline
> > and anything more than nr-requests is sent for processing in thread
> > pool.
> >
> > I have got good results with "--thread-pool-size=16 --thread-pool-threshold=3"
> > on my system.
> >
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > ---
> >
> > Test results with this patch are available here.
> >
> > https://github.com/rhvgoyal/virtiofs-tests/tree/master/performance-results/thread-pool-threshold/4-may-2021
> >
> > Changes since v2:
> > - Renamed knob name to "--thread-pool-threshold"
> > - Started using GQueue instead of GList to keep a list of requests
> > received on virtqueue (Greg)
> > - When threshold is crossed only requests above threshold are sent to
> > thread pool. Rest are still processed inline. (Greg)
> > ---
> > tools/virtiofsd/fuse_i.h | 1 +
> > tools/virtiofsd/fuse_lowlevel.c | 8 +++++++-
> > tools/virtiofsd/fuse_virtio.c | 27 +++++++++++++--------------
> > 3 files changed, 21 insertions(+), 15 deletions(-)
> >
> > Index: rhvgoyal-qemu/tools/virtiofsd/fuse_virtio.c
> > ===================================================================
> > --- rhvgoyal-qemu.orig/tools/virtiofsd/fuse_virtio.c 2021-05-04 09:55:30.467514740 -0400
> > +++ rhvgoyal-qemu/tools/virtiofsd/fuse_virtio.c 2021-05-04 10:06:32.453801299 -0400
> > @@ -445,7 +445,6 @@ err:
> >
> > static __thread bool clone_fs_called;
> >
> > -/* Process one FVRequest in a thread pool */
> > static void fv_queue_worker(gpointer data, gpointer user_data)
> > {
> > struct fv_QueueInfo *qi = user_data;
> > @@ -604,11 +603,12 @@ static void *fv_queue_thread(void *opaqu
> > struct VuVirtq *q = vu_get_queue(dev, qi->qidx);
> > struct fuse_session *se = qi->virtio_dev->se;
> > GThreadPool *pool = NULL;
> > - GList *req_list = NULL;
> > + GQueue req_queue = G_QUEUE_INIT;
> >
> > if (se->thread_pool_size) {
> > - fuse_log(FUSE_LOG_DEBUG, "%s: Creating thread pool for Queue %d\n",
> > - __func__, qi->qidx);
> > + fuse_log(FUSE_LOG_DEBUG, "%s: Creating thread pool for Queue %d."
> > + " thread_pool_threshold=%u\n", __func__, qi->qidx,
> > + se->thread_pool_threshold);
> > pool = g_thread_pool_new(fv_queue_worker, qi, se->thread_pool_size,
> > FALSE, NULL);
> > if (!pool) {
> > @@ -686,22 +686,21 @@ static void *fv_queue_thread(void *opaqu
> > }
> >
> > req->reply_sent = false;
> > -
> > - if (!se->thread_pool_size) {
> > - req_list = g_list_prepend(req_list, req);
> > - } else {
> > - g_thread_pool_push(pool, req, NULL);
> > - }
> > + g_queue_push_tail(&req_queue, req);
>
> So here you replace prepend() with push_tail() but...
>
> > }
> >
> > pthread_mutex_unlock(&qi->vq_lock);
> > vu_dispatch_unlock(qi->virtio_dev);
> >
> > /* Process all the requests. */
> > - if (!se->thread_pool_size && req_list != NULL) {
> > - g_list_foreach(req_list, fv_queue_worker, qi);
> > - g_list_free(req_list);
> > - req_list = NULL;
> > + for (int i = g_queue_get_length(&req_queue); i > 0; i--) {
> > + FVRequest *req = g_queue_pop_head(&req_queue);
>
> ... this pops from the head. Isn't this reversing the order in which
> requests are processed ? Not very harmful I guess.
IIUC, g_queue_push_tail() will add each new request to tail of the queue.
So when I pop from head, I will first pop the element which came first.
That will be FIFO order. Am I misunderstanding it?
>
> > +
> > + if (se->thread_pool_size && i > se->thread_pool_threshold) {
>
> requests are pushed to the thread pool first, good. :)
>
> > + g_thread_pool_push(pool, req, NULL);
> > + } else {
> > + fv_queue_worker(req, qi);
> > + }
> > }
> > }
> >
> > Index: rhvgoyal-qemu/tools/virtiofsd/fuse_i.h
> > ===================================================================
> > --- rhvgoyal-qemu.orig/tools/virtiofsd/fuse_i.h 2021-05-04 09:55:30.467514740 -0400
> > +++ rhvgoyal-qemu/tools/virtiofsd/fuse_i.h 2021-05-04 10:04:58.737938382 -0400
> > @@ -73,6 +73,7 @@ struct fuse_session {
> > int vu_socketfd;
> > struct fv_VuDev *virtio_dev;
> > int thread_pool_size;
> > + unsigned thread_pool_threshold;
> > };
> >
> > struct fuse_chan {
> > Index: rhvgoyal-qemu/tools/virtiofsd/fuse_lowlevel.c
> > ===================================================================
> > --- rhvgoyal-qemu.orig/tools/virtiofsd/fuse_lowlevel.c 2021-05-04 09:55:30.467514740 -0400
> > +++ rhvgoyal-qemu/tools/virtiofsd/fuse_lowlevel.c 2021-05-04 10:17:27.674809167 -0400
> > @@ -2432,6 +2432,7 @@ static const struct fuse_opt fuse_ll_opt
> > LL_OPTION("--socket-group=%s", vu_socket_group, 0),
> > LL_OPTION("--fd=%d", vu_listen_fd, 0),
> > LL_OPTION("--thread-pool-size=%d", thread_pool_size, 0),
> > + LL_OPTION("--thread-pool-threshold=%u", thread_pool_threshold, 0),
> > FUSE_OPT_END
> > };
> >
> > @@ -2452,7 +2453,11 @@ void fuse_lowlevel_help(void)
> > " --socket-path=PATH path for the vhost-user socket\n"
> > " --socket-group=GRNAME name of group for the vhost-user socket\n"
> > " --fd=FDNUM fd number of vhost-user socket\n"
> > - " --thread-pool-size=NUM thread pool size limit (default %d)\n",
> > + " --thread-pool-size=NUM thread pool size limit (default %d)\n"
> > + " --thread-pool-threshold=NUM\n"
> > + " number of request threshold to\n"
> > + " switch between inline and thread pool\n"
> > + " processing of request\n",
>
> The number of requests the queue thread pops out from the virtqueue
> looks a bit cryptic from a user point of view IMHO. Is this really
> something we want to expose in the API ? Shouldn't this be something
> we want to only change internally depending one some heuristics in
> the future ?
I think there are few things to consider.
- We will always have capability to modify defaults internally until
and unless user overrides that with options. So even if user does
not specify --thread-pool-threshold, we can always use it internally
if we can get good results across broad range of systems.
- Anohter option could be that we just give user a knob to enable
this switching behavior but number of requests are determined
internally. We can add that knob later as well. Right now I don't
feel like hardcoding this value "3" internally because it will
vary from systems to systems and from workload to worklaod
probably. That's why I explicitly provided this interface
so that users can speicify this value.
Having said that, this looks ugly. How a user is supposed to know
what value they should configure by default. That makes it more
of an interface usable in specific scenarios and not a generic
interface.
So question is how to arrive at better heuristics so that virtiofsd
itself determines right threhosld and user should not have to specify
the threshold. Users should probably choose between whether to
opt-in/opt-out of that new behavior.
That makes me think that this is a good experimental patch to show
that we can improve performance across spectrum if we wisely choose
between inline process and when to take help of thread pool. But we
need to develop a method for some sort of smart heuristic to determine
that and not leave it to user.
Thanks
Vivek
>
> > THREAD_POOL_SIZE);
> > }
> >
> > @@ -2508,6 +2513,7 @@ struct fuse_session *fuse_session_new(st
> > se->fd = -1;
> > se->vu_listen_fd = -1;
> > se->thread_pool_size = THREAD_POOL_SIZE;
> > + se->thread_pool_threshold = 0;
> > se->conn.max_write = UINT_MAX;
> > se->conn.max_readahead = UINT_MAX;
> >
> >
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Virtio-fs] [RFC][PATCH][v3] virtiofsd: virtiofsd: Use thread pool only after certain queue depth
2021-05-04 16:09 ` Vivek Goyal
@ 2021-05-04 16:34 ` Greg Kurz
2021-05-04 18:07 ` Vivek Goyal
0 siblings, 1 reply; 7+ messages in thread
From: Greg Kurz @ 2021-05-04 16:34 UTC (permalink / raw)
To: Vivek Goyal; +Cc: virtio-fs-list
On Tue, 4 May 2021 12:09:09 -0400
Vivek Goyal <vgoyal@redhat.com> wrote:
> On Tue, May 04, 2021 at 05:54:38PM +0200, Greg Kurz wrote:
> > On Tue, 4 May 2021 11:18:41 -0400
> > Vivek Goyal <vgoyal@redhat.com> wrote:
> >
> > > For low queue depth workloads, inline processing works well. But for
> > > high queue depth (and multiple process/thread) workloads, parallel
> > > processing of thread pool can benefit.
> > >
> > > This patch provides a knob --thread-pool-threshold=<nr-requests>, which
> > > uses a thread pool only if number of requests on virtqueue are more
> > > than nr-requests. IOW, upto nr-requests, requests are processed inline
> > > and anything more than nr-requests is sent for processing in thread
> > > pool.
> > >
> > > I have got good results with "--thread-pool-size=16 --thread-pool-threshold=3"
> > > on my system.
> > >
> > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > > ---
> > >
> > > Test results with this patch are available here.
> > >
> > > https://github.com/rhvgoyal/virtiofs-tests/tree/master/performance-results/thread-pool-threshold/4-may-2021
> > >
> > > Changes since v2:
> > > - Renamed knob name to "--thread-pool-threshold"
> > > - Started using GQueue instead of GList to keep a list of requests
> > > received on virtqueue (Greg)
> > > - When threshold is crossed only requests above threshold are sent to
> > > thread pool. Rest are still processed inline. (Greg)
> > > ---
> > > tools/virtiofsd/fuse_i.h | 1 +
> > > tools/virtiofsd/fuse_lowlevel.c | 8 +++++++-
> > > tools/virtiofsd/fuse_virtio.c | 27 +++++++++++++--------------
> > > 3 files changed, 21 insertions(+), 15 deletions(-)
> > >
> > > Index: rhvgoyal-qemu/tools/virtiofsd/fuse_virtio.c
> > > ===================================================================
> > > --- rhvgoyal-qemu.orig/tools/virtiofsd/fuse_virtio.c 2021-05-04 09:55:30.467514740 -0400
> > > +++ rhvgoyal-qemu/tools/virtiofsd/fuse_virtio.c 2021-05-04 10:06:32.453801299 -0400
> > > @@ -445,7 +445,6 @@ err:
> > >
> > > static __thread bool clone_fs_called;
> > >
> > > -/* Process one FVRequest in a thread pool */
> > > static void fv_queue_worker(gpointer data, gpointer user_data)
> > > {
> > > struct fv_QueueInfo *qi = user_data;
> > > @@ -604,11 +603,12 @@ static void *fv_queue_thread(void *opaqu
> > > struct VuVirtq *q = vu_get_queue(dev, qi->qidx);
> > > struct fuse_session *se = qi->virtio_dev->se;
> > > GThreadPool *pool = NULL;
> > > - GList *req_list = NULL;
> > > + GQueue req_queue = G_QUEUE_INIT;
> > >
> > > if (se->thread_pool_size) {
> > > - fuse_log(FUSE_LOG_DEBUG, "%s: Creating thread pool for Queue %d\n",
> > > - __func__, qi->qidx);
> > > + fuse_log(FUSE_LOG_DEBUG, "%s: Creating thread pool for Queue %d."
> > > + " thread_pool_threshold=%u\n", __func__, qi->qidx,
> > > + se->thread_pool_threshold);
> > > pool = g_thread_pool_new(fv_queue_worker, qi, se->thread_pool_size,
> > > FALSE, NULL);
> > > if (!pool) {
> > > @@ -686,22 +686,21 @@ static void *fv_queue_thread(void *opaqu
> > > }
> > >
> > > req->reply_sent = false;
> > > -
> > > - if (!se->thread_pool_size) {
> > > - req_list = g_list_prepend(req_list, req);
> > > - } else {
> > > - g_thread_pool_push(pool, req, NULL);
> > > - }
> > > + g_queue_push_tail(&req_queue, req);
> >
> > So here you replace prepend() with push_tail() but...
> >
> > > }
> > >
> > > pthread_mutex_unlock(&qi->vq_lock);
> > > vu_dispatch_unlock(qi->virtio_dev);
> > >
> > > /* Process all the requests. */
> > > - if (!se->thread_pool_size && req_list != NULL) {
> > > - g_list_foreach(req_list, fv_queue_worker, qi);
> > > - g_list_free(req_list);
> > > - req_list = NULL;
> > > + for (int i = g_queue_get_length(&req_queue); i > 0; i--) {
> > > + FVRequest *req = g_queue_pop_head(&req_queue);
> >
> > ... this pops from the head. Isn't this reversing the order in which
> > requests are processed ? Not very harmful I guess.
>
> IIUC, g_queue_push_tail() will add each new request to tail of the queue.
> So when I pop from head, I will first pop the element which came first.
> That will be FIFO order. Am I misunderstanding it?
>
This isn't what the current code is doing in the no-thread pool case
AFAICT. It pushes requests to the head and then does foreach(), i.e.
LIFO.
> >
> > > +
> > > + if (se->thread_pool_size && i > se->thread_pool_threshold) {
> >
> > requests are pushed to the thread pool first, good. :)
> >
> > > + g_thread_pool_push(pool, req, NULL);
> > > + } else {
> > > + fv_queue_worker(req, qi);
> > > + }
> > > }
> > > }
> > >
> > > Index: rhvgoyal-qemu/tools/virtiofsd/fuse_i.h
> > > ===================================================================
> > > --- rhvgoyal-qemu.orig/tools/virtiofsd/fuse_i.h 2021-05-04 09:55:30.467514740 -0400
> > > +++ rhvgoyal-qemu/tools/virtiofsd/fuse_i.h 2021-05-04 10:04:58.737938382 -0400
> > > @@ -73,6 +73,7 @@ struct fuse_session {
> > > int vu_socketfd;
> > > struct fv_VuDev *virtio_dev;
> > > int thread_pool_size;
> > > + unsigned thread_pool_threshold;
> > > };
> > >
> > > struct fuse_chan {
> > > Index: rhvgoyal-qemu/tools/virtiofsd/fuse_lowlevel.c
> > > ===================================================================
> > > --- rhvgoyal-qemu.orig/tools/virtiofsd/fuse_lowlevel.c 2021-05-04 09:55:30.467514740 -0400
> > > +++ rhvgoyal-qemu/tools/virtiofsd/fuse_lowlevel.c 2021-05-04 10:17:27.674809167 -0400
> > > @@ -2432,6 +2432,7 @@ static const struct fuse_opt fuse_ll_opt
> > > LL_OPTION("--socket-group=%s", vu_socket_group, 0),
> > > LL_OPTION("--fd=%d", vu_listen_fd, 0),
> > > LL_OPTION("--thread-pool-size=%d", thread_pool_size, 0),
> > > + LL_OPTION("--thread-pool-threshold=%u", thread_pool_threshold, 0),
> > > FUSE_OPT_END
> > > };
> > >
> > > @@ -2452,7 +2453,11 @@ void fuse_lowlevel_help(void)
> > > " --socket-path=PATH path for the vhost-user socket\n"
> > > " --socket-group=GRNAME name of group for the vhost-user socket\n"
> > > " --fd=FDNUM fd number of vhost-user socket\n"
> > > - " --thread-pool-size=NUM thread pool size limit (default %d)\n",
> > > + " --thread-pool-size=NUM thread pool size limit (default %d)\n"
> > > + " --thread-pool-threshold=NUM\n"
> > > + " number of request threshold to\n"
> > > + " switch between inline and thread pool\n"
> > > + " processing of request\n",
> >
> > The number of requests the queue thread pops out from the virtqueue
> > looks a bit cryptic from a user point of view IMHO. Is this really
> > something we want to expose in the API ? Shouldn't this be something
> > we want to only change internally depending one some heuristics in
> > the future ?
>
> I think there are few things to consider.
>
> - We will always have capability to modify defaults internally until
> and unless user overrides that with options. So even if user does
> not specify --thread-pool-threshold, we can always use it internally
> if we can get good results across broad range of systems.
>
> - Anohter option could be that we just give user a knob to enable
> this switching behavior but number of requests are determined
> internally. We can add that knob later as well. Right now I don't
> feel like hardcoding this value "3" internally because it will
You're right but until we come up with a heuristic, if you had
good results with "3", I guess it's ok to hardcode that for now.
> vary from systems to systems and from workload to worklaod
> probably. That's why I explicitly provided this interface
> so that users can speicify this value.
>
> Having said that, this looks ugly. How a user is supposed to know
> what value they should configure by default. That makes it more
> of an interface usable in specific scenarios and not a generic
> interface.
>
That's my point. Ugly interfaces can prove to be burden in the
long term.... At least it should have x- prefix so that people
know they should probably not rely on it too much.
> So question is how to arrive at better heuristics so that virtiofsd
> itself determines right threhosld and user should not have to specify
> the threshold. Users should probably choose between whether to
> opt-in/opt-out of that new behavior.
>
I'm not even sure the user needs to know that we internally
decide to process a few requests inline when a thread pool is
available.
> That makes me think that this is a good experimental patch to show
> that we can improve performance across spectrum if we wisely choose
> between inline process and when to take help of thread pool. But we
> need to develop a method for some sort of smart heuristic to determine
> that and not leave it to user.
>
Yeah definitely.
> Thanks
> Vivek
>
> >
> > > THREAD_POOL_SIZE);
> > > }
> > >
> > > @@ -2508,6 +2513,7 @@ struct fuse_session *fuse_session_new(st
> > > se->fd = -1;
> > > se->vu_listen_fd = -1;
> > > se->thread_pool_size = THREAD_POOL_SIZE;
> > > + se->thread_pool_threshold = 0;
> > > se->conn.max_write = UINT_MAX;
> > > se->conn.max_readahead = UINT_MAX;
> > >
> > >
> >
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Virtio-fs] [RFC][PATCH][v3] virtiofsd: virtiofsd: Use thread pool only after certain queue depth
2021-05-04 16:34 ` Greg Kurz
@ 2021-05-04 18:07 ` Vivek Goyal
2021-05-05 7:49 ` Greg Kurz
0 siblings, 1 reply; 7+ messages in thread
From: Vivek Goyal @ 2021-05-04 18:07 UTC (permalink / raw)
To: Greg Kurz; +Cc: virtio-fs-list
On Tue, May 04, 2021 at 06:34:44PM +0200, Greg Kurz wrote:
> On Tue, 4 May 2021 12:09:09 -0400
> Vivek Goyal <vgoyal@redhat.com> wrote:
>
> > On Tue, May 04, 2021 at 05:54:38PM +0200, Greg Kurz wrote:
> > > On Tue, 4 May 2021 11:18:41 -0400
> > > Vivek Goyal <vgoyal@redhat.com> wrote:
> > >
> > > > For low queue depth workloads, inline processing works well. But for
> > > > high queue depth (and multiple process/thread) workloads, parallel
> > > > processing of thread pool can benefit.
> > > >
> > > > This patch provides a knob --thread-pool-threshold=<nr-requests>, which
> > > > uses a thread pool only if number of requests on virtqueue are more
> > > > than nr-requests. IOW, upto nr-requests, requests are processed inline
> > > > and anything more than nr-requests is sent for processing in thread
> > > > pool.
> > > >
> > > > I have got good results with "--thread-pool-size=16 --thread-pool-threshold=3"
> > > > on my system.
> > > >
> > > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > > > ---
> > > >
> > > > Test results with this patch are available here.
> > > >
> > > > https://github.com/rhvgoyal/virtiofs-tests/tree/master/performance-results/thread-pool-threshold/4-may-2021
> > > >
> > > > Changes since v2:
> > > > - Renamed knob name to "--thread-pool-threshold"
> > > > - Started using GQueue instead of GList to keep a list of requests
> > > > received on virtqueue (Greg)
> > > > - When threshold is crossed only requests above threshold are sent to
> > > > thread pool. Rest are still processed inline. (Greg)
> > > > ---
> > > > tools/virtiofsd/fuse_i.h | 1 +
> > > > tools/virtiofsd/fuse_lowlevel.c | 8 +++++++-
> > > > tools/virtiofsd/fuse_virtio.c | 27 +++++++++++++--------------
> > > > 3 files changed, 21 insertions(+), 15 deletions(-)
> > > >
> > > > Index: rhvgoyal-qemu/tools/virtiofsd/fuse_virtio.c
> > > > ===================================================================
> > > > --- rhvgoyal-qemu.orig/tools/virtiofsd/fuse_virtio.c 2021-05-04 09:55:30.467514740 -0400
> > > > +++ rhvgoyal-qemu/tools/virtiofsd/fuse_virtio.c 2021-05-04 10:06:32.453801299 -0400
> > > > @@ -445,7 +445,6 @@ err:
> > > >
> > > > static __thread bool clone_fs_called;
> > > >
> > > > -/* Process one FVRequest in a thread pool */
> > > > static void fv_queue_worker(gpointer data, gpointer user_data)
> > > > {
> > > > struct fv_QueueInfo *qi = user_data;
> > > > @@ -604,11 +603,12 @@ static void *fv_queue_thread(void *opaqu
> > > > struct VuVirtq *q = vu_get_queue(dev, qi->qidx);
> > > > struct fuse_session *se = qi->virtio_dev->se;
> > > > GThreadPool *pool = NULL;
> > > > - GList *req_list = NULL;
> > > > + GQueue req_queue = G_QUEUE_INIT;
> > > >
> > > > if (se->thread_pool_size) {
> > > > - fuse_log(FUSE_LOG_DEBUG, "%s: Creating thread pool for Queue %d\n",
> > > > - __func__, qi->qidx);
> > > > + fuse_log(FUSE_LOG_DEBUG, "%s: Creating thread pool for Queue %d."
> > > > + " thread_pool_threshold=%u\n", __func__, qi->qidx,
> > > > + se->thread_pool_threshold);
> > > > pool = g_thread_pool_new(fv_queue_worker, qi, se->thread_pool_size,
> > > > FALSE, NULL);
> > > > if (!pool) {
> > > > @@ -686,22 +686,21 @@ static void *fv_queue_thread(void *opaqu
> > > > }
> > > >
> > > > req->reply_sent = false;
> > > > -
> > > > - if (!se->thread_pool_size) {
> > > > - req_list = g_list_prepend(req_list, req);
> > > > - } else {
> > > > - g_thread_pool_push(pool, req, NULL);
> > > > - }
> > > > + g_queue_push_tail(&req_queue, req);
> > >
> > > So here you replace prepend() with push_tail() but...
> > >
> > > > }
> > > >
> > > > pthread_mutex_unlock(&qi->vq_lock);
> > > > vu_dispatch_unlock(qi->virtio_dev);
> > > >
> > > > /* Process all the requests. */
> > > > - if (!se->thread_pool_size && req_list != NULL) {
> > > > - g_list_foreach(req_list, fv_queue_worker, qi);
> > > > - g_list_free(req_list);
> > > > - req_list = NULL;
> > > > + for (int i = g_queue_get_length(&req_queue); i > 0; i--) {
> > > > + FVRequest *req = g_queue_pop_head(&req_queue);
> > >
> > > ... this pops from the head. Isn't this reversing the order in which
> > > requests are processed ? Not very harmful I guess.
> >
> > IIUC, g_queue_push_tail() will add each new request to tail of the queue.
> > So when I pop from head, I will first pop the element which came first.
> > That will be FIFO order. Am I misunderstanding it?
> >
>
> This isn't what the current code is doing in the no-thread pool case
> AFAICT. It pushes requests to the head and then does foreach(), i.e.
> LIFO.
Got it. Yes, so current behavior seems to be LIFO (no-thread pool case)
and now with this patch it will become FIFO.
That LIFO behavior was not intentional. I think I was just lazy and
did not think through it.
Though we do not guarantee any ordering in terms of request ordering,
I guess it does not hurt to process FIFO, if possible.
So if you see a problem with FIFO processing, I can change it.
>
> > >
> > > > +
> > > > + if (se->thread_pool_size && i > se->thread_pool_threshold) {
> > >
> > > requests are pushed to the thread pool first, good. :)
> > >
> > > > + g_thread_pool_push(pool, req, NULL);
> > > > + } else {
> > > > + fv_queue_worker(req, qi);
> > > > + }
> > > > }
> > > > }
> > > >
> > > > Index: rhvgoyal-qemu/tools/virtiofsd/fuse_i.h
> > > > ===================================================================
> > > > --- rhvgoyal-qemu.orig/tools/virtiofsd/fuse_i.h 2021-05-04 09:55:30.467514740 -0400
> > > > +++ rhvgoyal-qemu/tools/virtiofsd/fuse_i.h 2021-05-04 10:04:58.737938382 -0400
> > > > @@ -73,6 +73,7 @@ struct fuse_session {
> > > > int vu_socketfd;
> > > > struct fv_VuDev *virtio_dev;
> > > > int thread_pool_size;
> > > > + unsigned thread_pool_threshold;
> > > > };
> > > >
> > > > struct fuse_chan {
> > > > Index: rhvgoyal-qemu/tools/virtiofsd/fuse_lowlevel.c
> > > > ===================================================================
> > > > --- rhvgoyal-qemu.orig/tools/virtiofsd/fuse_lowlevel.c 2021-05-04 09:55:30.467514740 -0400
> > > > +++ rhvgoyal-qemu/tools/virtiofsd/fuse_lowlevel.c 2021-05-04 10:17:27.674809167 -0400
> > > > @@ -2432,6 +2432,7 @@ static const struct fuse_opt fuse_ll_opt
> > > > LL_OPTION("--socket-group=%s", vu_socket_group, 0),
> > > > LL_OPTION("--fd=%d", vu_listen_fd, 0),
> > > > LL_OPTION("--thread-pool-size=%d", thread_pool_size, 0),
> > > > + LL_OPTION("--thread-pool-threshold=%u", thread_pool_threshold, 0),
> > > > FUSE_OPT_END
> > > > };
> > > >
> > > > @@ -2452,7 +2453,11 @@ void fuse_lowlevel_help(void)
> > > > " --socket-path=PATH path for the vhost-user socket\n"
> > > > " --socket-group=GRNAME name of group for the vhost-user socket\n"
> > > > " --fd=FDNUM fd number of vhost-user socket\n"
> > > > - " --thread-pool-size=NUM thread pool size limit (default %d)\n",
> > > > + " --thread-pool-size=NUM thread pool size limit (default %d)\n"
> > > > + " --thread-pool-threshold=NUM\n"
> > > > + " number of request threshold to\n"
> > > > + " switch between inline and thread pool\n"
> > > > + " processing of request\n",
> > >
> > > The number of requests the queue thread pops out from the virtqueue
> > > looks a bit cryptic from a user point of view IMHO. Is this really
> > > something we want to expose in the API ? Shouldn't this be something
> > > we want to only change internally depending one some heuristics in
> > > the future ?
> >
> > I think there are few things to consider.
> >
> > - We will always have capability to modify defaults internally until
> > and unless user overrides that with options. So even if user does
> > not specify --thread-pool-threshold, we can always use it internally
> > if we can get good results across broad range of systems.
> >
> > - Anohter option could be that we just give user a knob to enable
> > this switching behavior but number of requests are determined
> > internally. We can add that knob later as well. Right now I don't
> > feel like hardcoding this value "3" internally because it will
>
> You're right but until we come up with a heuristic, if you had
> good results with "3", I guess it's ok to hardcode that for now.
>
> > vary from systems to systems and from workload to worklaod
> > probably. That's why I explicitly provided this interface
> > so that users can speicify this value.
> >
> > Having said that, this looks ugly. How a user is supposed to know
> > what value they should configure by default. That makes it more
> > of an interface usable in specific scenarios and not a generic
> > interface.
> >
>
> That's my point. Ugly interfaces can prove to be burden in the
> long term.... At least it should have x- prefix so that people
> know they should probably not rely on it too much.
>
> > So question is how to arrive at better heuristics so that virtiofsd
> > itself determines right threhosld and user should not have to specify
> > the threshold. Users should probably choose between whether to
> > opt-in/opt-out of that new behavior.
> >
>
> I'm not even sure the user needs to know that we internally
> decide to process a few requests inline when a thread pool is
> available.
Currently we offer --thread-pool-size=X option. It means a thread pool
is created and as of now all requests are sent to thread pool.
Is it ok, to change that behavior (without user opting in) where upto 3
requests are processed inline and anything more than 3 is sent to
thread pool.
Or just leave it untouched and choose new behavior as default only
if user did not specify --thread-pool-size.
Thanks
Vivek
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Virtio-fs] [RFC][PATCH][v3] virtiofsd: virtiofsd: Use thread pool only after certain queue depth
2021-05-04 18:07 ` Vivek Goyal
@ 2021-05-05 7:49 ` Greg Kurz
2021-05-05 19:05 ` Vivek Goyal
0 siblings, 1 reply; 7+ messages in thread
From: Greg Kurz @ 2021-05-05 7:49 UTC (permalink / raw)
To: Vivek Goyal; +Cc: virtio-fs-list
On Tue, 4 May 2021 14:07:29 -0400
Vivek Goyal <vgoyal@redhat.com> wrote:
> On Tue, May 04, 2021 at 06:34:44PM +0200, Greg Kurz wrote:
> > On Tue, 4 May 2021 12:09:09 -0400
> > Vivek Goyal <vgoyal@redhat.com> wrote:
> >
> > > On Tue, May 04, 2021 at 05:54:38PM +0200, Greg Kurz wrote:
> > > > On Tue, 4 May 2021 11:18:41 -0400
> > > > Vivek Goyal <vgoyal@redhat.com> wrote:
> > > >
> > > > > For low queue depth workloads, inline processing works well. But for
> > > > > high queue depth (and multiple process/thread) workloads, parallel
> > > > > processing of thread pool can benefit.
> > > > >
> > > > > This patch provides a knob --thread-pool-threshold=<nr-requests>, which
> > > > > uses a thread pool only if number of requests on virtqueue are more
> > > > > than nr-requests. IOW, upto nr-requests, requests are processed inline
> > > > > and anything more than nr-requests is sent for processing in thread
> > > > > pool.
> > > > >
> > > > > I have got good results with "--thread-pool-size=16 --thread-pool-threshold=3"
> > > > > on my system.
> > > > >
> > > > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > > > > ---
> > > > >
> > > > > Test results with this patch are available here.
> > > > >
> > > > > https://github.com/rhvgoyal/virtiofs-tests/tree/master/performance-results/thread-pool-threshold/4-may-2021
> > > > >
> > > > > Changes since v2:
> > > > > - Renamed knob name to "--thread-pool-threshold"
> > > > > - Started using GQueue instead of GList to keep a list of requests
> > > > > received on virtqueue (Greg)
> > > > > - When threshold is crossed only requests above threshold are sent to
> > > > > thread pool. Rest are still processed inline. (Greg)
> > > > > ---
> > > > > tools/virtiofsd/fuse_i.h | 1 +
> > > > > tools/virtiofsd/fuse_lowlevel.c | 8 +++++++-
> > > > > tools/virtiofsd/fuse_virtio.c | 27 +++++++++++++--------------
> > > > > 3 files changed, 21 insertions(+), 15 deletions(-)
> > > > >
> > > > > Index: rhvgoyal-qemu/tools/virtiofsd/fuse_virtio.c
> > > > > ===================================================================
> > > > > --- rhvgoyal-qemu.orig/tools/virtiofsd/fuse_virtio.c 2021-05-04 09:55:30.467514740 -0400
> > > > > +++ rhvgoyal-qemu/tools/virtiofsd/fuse_virtio.c 2021-05-04 10:06:32.453801299 -0400
> > > > > @@ -445,7 +445,6 @@ err:
> > > > >
> > > > > static __thread bool clone_fs_called;
> > > > >
> > > > > -/* Process one FVRequest in a thread pool */
> > > > > static void fv_queue_worker(gpointer data, gpointer user_data)
> > > > > {
> > > > > struct fv_QueueInfo *qi = user_data;
> > > > > @@ -604,11 +603,12 @@ static void *fv_queue_thread(void *opaqu
> > > > > struct VuVirtq *q = vu_get_queue(dev, qi->qidx);
> > > > > struct fuse_session *se = qi->virtio_dev->se;
> > > > > GThreadPool *pool = NULL;
> > > > > - GList *req_list = NULL;
> > > > > + GQueue req_queue = G_QUEUE_INIT;
> > > > >
> > > > > if (se->thread_pool_size) {
> > > > > - fuse_log(FUSE_LOG_DEBUG, "%s: Creating thread pool for Queue %d\n",
> > > > > - __func__, qi->qidx);
> > > > > + fuse_log(FUSE_LOG_DEBUG, "%s: Creating thread pool for Queue %d."
> > > > > + " thread_pool_threshold=%u\n", __func__, qi->qidx,
> > > > > + se->thread_pool_threshold);
> > > > > pool = g_thread_pool_new(fv_queue_worker, qi, se->thread_pool_size,
> > > > > FALSE, NULL);
> > > > > if (!pool) {
> > > > > @@ -686,22 +686,21 @@ static void *fv_queue_thread(void *opaqu
> > > > > }
> > > > >
> > > > > req->reply_sent = false;
> > > > > -
> > > > > - if (!se->thread_pool_size) {
> > > > > - req_list = g_list_prepend(req_list, req);
> > > > > - } else {
> > > > > - g_thread_pool_push(pool, req, NULL);
> > > > > - }
> > > > > + g_queue_push_tail(&req_queue, req);
> > > >
> > > > So here you replace prepend() with push_tail() but...
> > > >
> > > > > }
> > > > >
> > > > > pthread_mutex_unlock(&qi->vq_lock);
> > > > > vu_dispatch_unlock(qi->virtio_dev);
> > > > >
> > > > > /* Process all the requests. */
> > > > > - if (!se->thread_pool_size && req_list != NULL) {
> > > > > - g_list_foreach(req_list, fv_queue_worker, qi);
> > > > > - g_list_free(req_list);
> > > > > - req_list = NULL;
> > > > > + for (int i = g_queue_get_length(&req_queue); i > 0; i--) {
> > > > > + FVRequest *req = g_queue_pop_head(&req_queue);
> > > >
> > > > ... this pops from the head. Isn't this reversing the order in which
> > > > requests are processed ? Not very harmful I guess.
> > >
> > > IIUC, g_queue_push_tail() will add each new request to tail of the queue.
> > > So when I pop from head, I will first pop the element which came first.
> > > That will be FIFO order. Am I misunderstanding it?
> > >
> >
> > This isn't what the current code is doing in the no-thread pool case
> > AFAICT. It pushes requests to the head and then does foreach(), i.e.
> > LIFO.
>
> Got it. Yes, so current behavior seems to be LIFO (no-thread pool case)
> and now with this patch it will become FIFO.
>
> That LIFO behavior was not intentional. I think I was just lazy and
> did not think through it.
>
> Though we do not guarantee any ordering in terms of request ordering,
> I guess it does not hurt to process FIFO, if possible.
>
> So if you see a problem with FIFO processing, I can change it.
>
No that's fine with me since, as you say, we don't guarantee
ordering anyway and the previous LIFO behavior wasn't intentional.
> >
> > > >
> > > > > +
> > > > > + if (se->thread_pool_size && i > se->thread_pool_threshold) {
> > > >
> > > > requests are pushed to the thread pool first, good. :)
> > > >
> > > > > + g_thread_pool_push(pool, req, NULL);
> > > > > + } else {
> > > > > + fv_queue_worker(req, qi);
> > > > > + }
> > > > > }
> > > > > }
> > > > >
> > > > > Index: rhvgoyal-qemu/tools/virtiofsd/fuse_i.h
> > > > > ===================================================================
> > > > > --- rhvgoyal-qemu.orig/tools/virtiofsd/fuse_i.h 2021-05-04 09:55:30.467514740 -0400
> > > > > +++ rhvgoyal-qemu/tools/virtiofsd/fuse_i.h 2021-05-04 10:04:58.737938382 -0400
> > > > > @@ -73,6 +73,7 @@ struct fuse_session {
> > > > > int vu_socketfd;
> > > > > struct fv_VuDev *virtio_dev;
> > > > > int thread_pool_size;
> > > > > + unsigned thread_pool_threshold;
> > > > > };
> > > > >
> > > > > struct fuse_chan {
> > > > > Index: rhvgoyal-qemu/tools/virtiofsd/fuse_lowlevel.c
> > > > > ===================================================================
> > > > > --- rhvgoyal-qemu.orig/tools/virtiofsd/fuse_lowlevel.c 2021-05-04 09:55:30.467514740 -0400
> > > > > +++ rhvgoyal-qemu/tools/virtiofsd/fuse_lowlevel.c 2021-05-04 10:17:27.674809167 -0400
> > > > > @@ -2432,6 +2432,7 @@ static const struct fuse_opt fuse_ll_opt
> > > > > LL_OPTION("--socket-group=%s", vu_socket_group, 0),
> > > > > LL_OPTION("--fd=%d", vu_listen_fd, 0),
> > > > > LL_OPTION("--thread-pool-size=%d", thread_pool_size, 0),
> > > > > + LL_OPTION("--thread-pool-threshold=%u", thread_pool_threshold, 0),
> > > > > FUSE_OPT_END
> > > > > };
> > > > >
> > > > > @@ -2452,7 +2453,11 @@ void fuse_lowlevel_help(void)
> > > > > " --socket-path=PATH path for the vhost-user socket\n"
> > > > > " --socket-group=GRNAME name of group for the vhost-user socket\n"
> > > > > " --fd=FDNUM fd number of vhost-user socket\n"
> > > > > - " --thread-pool-size=NUM thread pool size limit (default %d)\n",
> > > > > + " --thread-pool-size=NUM thread pool size limit (default %d)\n"
> > > > > + " --thread-pool-threshold=NUM\n"
> > > > > + " number of request threshold to\n"
> > > > > + " switch between inline and thread pool\n"
> > > > > + " processing of request\n",
> > > >
> > > > The number of requests the queue thread pops out from the virtqueue
> > > > looks a bit cryptic from a user point of view IMHO. Is this really
> > > > something we want to expose in the API ? Shouldn't this be something
> > > > we want to only change internally depending one some heuristics in
> > > > the future ?
> > >
> > > I think there are few things to consider.
> > >
> > > - We will always have capability to modify defaults internally until
> > > and unless user overrides that with options. So even if user does
> > > not specify --thread-pool-threshold, we can always use it internally
> > > if we can get good results across broad range of systems.
> > >
> > > - Anohter option could be that we just give user a knob to enable
> > > this switching behavior but number of requests are determined
> > > internally. We can add that knob later as well. Right now I don't
> > > feel like hardcoding this value "3" internally because it will
> >
> > You're right but until we come up with a heuristic, if you had
> > good results with "3", I guess it's ok to hardcode that for now.
> >
> > > vary from systems to systems and from workload to worklaod
> > > probably. That's why I explicitly provided this interface
> > > so that users can speicify this value.
> > >
> > > Having said that, this looks ugly. How a user is supposed to know
> > > what value they should configure by default. That makes it more
> > > of an interface usable in specific scenarios and not a generic
> > > interface.
> > >
> >
> > That's my point. Ugly interfaces can prove to be burden in the
> > long term.... At least it should have x- prefix so that people
> > know they should probably not rely on it too much.
> >
> > > So question is how to arrive at better heuristics so that virtiofsd
> > > itself determines right threhosld and user should not have to specify
> > > the threshold. Users should probably choose between whether to
> > > opt-in/opt-out of that new behavior.
> > >
> >
> > I'm not even sure the user needs to know that we internally
> > decide to process a few requests inline when a thread pool is
> > available.
>
> Currently we offer --thread-pool-size=X option. It means a thread pool
> is created and as of now all requests are sent to thread pool.
>
Indeed we're doing this now but this is just an implementation detail
IMHO. I don't think that pushing all or only some requests to the
thread pool is relevant. I see the ---thread-pool-size=X option more
like a way to provision the maximum number of extra threads virtiofsd
is permitted to start in order to benefit from parallelism.
> Is it ok, to change that behavior (without user opting in) where upto 3
> requests are processed inline and anything more than 3 is sent to
> thread pool.
>
I think it is ok.
> Or just leave it untouched and choose new behavior as default only
> if user did not specify --thread-pool-size.
>
Not passing --thread-pool-size currently means no thread pool, i.e.
all requests are serialized. I'd rather preserve this behavior
because it clearly indicates that the user doesn't want parallelism.
Also if we use a thread pool anyway in this case, what would be its
size ?
> Thanks
> Vivek
>
Cheers,
--
Greg
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Virtio-fs] [RFC][PATCH][v3] virtiofsd: virtiofsd: Use thread pool only after certain queue depth
2021-05-05 7:49 ` Greg Kurz
@ 2021-05-05 19:05 ` Vivek Goyal
0 siblings, 0 replies; 7+ messages in thread
From: Vivek Goyal @ 2021-05-05 19:05 UTC (permalink / raw)
To: Greg Kurz; +Cc: virtio-fs-list
On Wed, May 05, 2021 at 09:49:19AM +0200, Greg Kurz wrote:
[..]
> > > > > > Index: rhvgoyal-qemu/tools/virtiofsd/fuse_lowlevel.c
> > > > > > ===================================================================
> > > > > > --- rhvgoyal-qemu.orig/tools/virtiofsd/fuse_lowlevel.c 2021-05-04 09:55:30.467514740 -0400
> > > > > > +++ rhvgoyal-qemu/tools/virtiofsd/fuse_lowlevel.c 2021-05-04 10:17:27.674809167 -0400
> > > > > > @@ -2432,6 +2432,7 @@ static const struct fuse_opt fuse_ll_opt
> > > > > > LL_OPTION("--socket-group=%s", vu_socket_group, 0),
> > > > > > LL_OPTION("--fd=%d", vu_listen_fd, 0),
> > > > > > LL_OPTION("--thread-pool-size=%d", thread_pool_size, 0),
> > > > > > + LL_OPTION("--thread-pool-threshold=%u", thread_pool_threshold, 0),
> > > > > > FUSE_OPT_END
> > > > > > };
> > > > > >
> > > > > > @@ -2452,7 +2453,11 @@ void fuse_lowlevel_help(void)
> > > > > > " --socket-path=PATH path for the vhost-user socket\n"
> > > > > > " --socket-group=GRNAME name of group for the vhost-user socket\n"
> > > > > > " --fd=FDNUM fd number of vhost-user socket\n"
> > > > > > - " --thread-pool-size=NUM thread pool size limit (default %d)\n",
> > > > > > + " --thread-pool-size=NUM thread pool size limit (default %d)\n"
> > > > > > + " --thread-pool-threshold=NUM\n"
> > > > > > + " number of request threshold to\n"
> > > > > > + " switch between inline and thread pool\n"
> > > > > > + " processing of request\n",
> > > > >
> > > > > The number of requests the queue thread pops out from the virtqueue
> > > > > looks a bit cryptic from a user point of view IMHO. Is this really
> > > > > something we want to expose in the API ? Shouldn't this be something
> > > > > we want to only change internally depending one some heuristics in
> > > > > the future ?
> > > >
> > > > I think there are few things to consider.
> > > >
> > > > - We will always have capability to modify defaults internally until
> > > > and unless user overrides that with options. So even if user does
> > > > not specify --thread-pool-threshold, we can always use it internally
> > > > if we can get good results across broad range of systems.
> > > >
> > > > - Anohter option could be that we just give user a knob to enable
> > > > this switching behavior but number of requests are determined
> > > > internally. We can add that knob later as well. Right now I don't
> > > > feel like hardcoding this value "3" internally because it will
> > >
> > > You're right but until we come up with a heuristic, if you had
> > > good results with "3", I guess it's ok to hardcode that for now.
> > >
> > > > vary from systems to systems and from workload to worklaod
> > > > probably. That's why I explicitly provided this interface
> > > > so that users can speicify this value.
> > > >
> > > > Having said that, this looks ugly. How a user is supposed to know
> > > > what value they should configure by default. That makes it more
> > > > of an interface usable in specific scenarios and not a generic
> > > > interface.
> > > >
> > >
> > > That's my point. Ugly interfaces can prove to be burden in the
> > > long term.... At least it should have x- prefix so that people
> > > know they should probably not rely on it too much.
> > >
> > > > So question is how to arrive at better heuristics so that virtiofsd
> > > > itself determines right threhosld and user should not have to specify
> > > > the threshold. Users should probably choose between whether to
> > > > opt-in/opt-out of that new behavior.
> > > >
> > >
> > > I'm not even sure the user needs to know that we internally
> > > decide to process a few requests inline when a thread pool is
> > > available.
> >
> > Currently we offer --thread-pool-size=X option. It means a thread pool
> > is created and as of now all requests are sent to thread pool.
> >
>
> Indeed we're doing this now but this is just an implementation detail
> IMHO. I don't think that pushing all or only some requests to the
> thread pool is relevant. I see the ---thread-pool-size=X option more
> like a way to provision the maximum number of extra threads virtiofsd
> is permitted to start in order to benefit from parallelism.
Hi Greg,
This sounds reasonable. I think easiest is to hardcode threshold
internally and if number of requests are more than threshold then
send extra requests to a thread pool (if one exists).
>
> > Is it ok, to change that behavior (without user opting in) where upto 3
> > requests are processed inline and anything more than 3 is sent to
> > thread pool.
> >
>
> I think it is ok.
>
> > Or just leave it untouched and choose new behavior as default only
> > if user did not specify --thread-pool-size.
> >
>
> Not passing --thread-pool-size currently means no thread pool, i.e.
> all requests are serialized. I'd rather preserve this behavior
> because it clearly indicates that the user doesn't want parallelism.
User can specify --thread-pool-size=0 if they don't want any parallelism.
Right now default is no thread pool but we should be able to change
that anytime.
In fact, with this patch I would like to change default to say
use thread-pool of 16 and a threshold of 3 requests. So upto 3
requests everything is processed internally and anything extra
is sent to thread pool.
Not specifying --thread-pool-size just means fallback to default and
its up to virtiofsd to decide what defaults does it see best.
>
> Also if we use a thread pool anyway in this case, what would be its
> size ?
Previously it used to be 64 by default. I think that probably is too
big. May be 16 is good enough.
Vivek
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-05-05 19:05 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-04 15:18 [Virtio-fs] [RFC][PATCH][v3] virtiofsd: virtiofsd: Use thread pool only after certain queue depth Vivek Goyal
2021-05-04 15:54 ` Greg Kurz
2021-05-04 16:09 ` Vivek Goyal
2021-05-04 16:34 ` Greg Kurz
2021-05-04 18:07 ` Vivek Goyal
2021-05-05 7:49 ` Greg Kurz
2021-05-05 19:05 ` Vivek Goyal
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.