2:39 p.m.
One way to reproduce the bugs is to set admin_max_workers=0 in
libvirtd.conf, restart libvirtd, and then call:
$ virt-admin server-threadpool-info admin
Marc Hartmayer (2):
rpc: Fix deadlock if there is no worker pool available
rpc: Fix segmentation fault when no worker pool is available
src/rpc/virnetserver.c | 33 ++++++++++++++++++++++-----------
1 file changed, 22 insertions(+), 11 deletions(-)
--
2.13.6
2:39 p.m.
New subject: [libvirt] [PATCH 1/2] rpc: Fix deadlock if there is no worker pool available
@srv must be unlocked for the call virNetServerProcessMsg otherwise a
deadlock can occur.
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
---
src/rpc/virnetserver.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
index 5c7f7dd08f..5ae809e372 100644
--- a/src/rpc/virnetserver.c
+++ b/src/rpc/virnetserver.c
@@ -201,7 +201,7 @@ static void virNetServerDispatchNewMessage(virNetServerClientPtr
client,
virNetServerJobPtr job;
if (VIR_ALLOC(job) < 0)
- goto error;
+ goto error_unlock;
job->client = client;
job->msg = msg;
@@ -216,20 +216,23 @@ static void virNetServerDispatchNewMessage(virNetServerClientPtr
client,
virObjectUnref(client);
VIR_FREE(job);
virObjectUnref(prog);
- goto error;
+ goto error_unlock;
}
+ virObjectUnlock(srv);
} else {
+ /* @srv must be unlocked for virNetServerProcessMsg */
+ virObjectUnlock(srv);
if (virNetServerProcessMsg(srv, client, prog, msg) < 0)
goto error;
}
- virObjectUnlock(srv);
return;
+ error_unlock:
+ virObjectUnlock(srv);
error:
virNetMessageFree(msg);
virNetServerClientClose(client);
- virObjectUnlock(srv);
}
/**
--
2.13.6
2:39 p.m.
New subject: [libvirt] [PATCH 2/2] rpc: Fix segmentation fault when no worker pool is available
If srv->workers is a NULL pointer, as it is the case if there are no
workers, then don't try to dereference it.
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
---
src/rpc/virnetserver.c | 22 +++++++++++++++-------
1 file changed, 15 insertions(+), 7 deletions(-)
diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
index 5ae809e372..be6f610880 100644
--- a/src/rpc/virnetserver.c
+++ b/src/rpc/virnetserver.c
@@ -933,13 +933,21 @@ virNetServerGetThreadPoolParameters(virNetServerPtr srv,
size_t *jobQueueDepth)
{
virObjectLock(srv);
-
- *minWorkers = virThreadPoolGetMinWorkers(srv->workers);
- *maxWorkers = virThreadPoolGetMaxWorkers(srv->workers);
- *freeWorkers = virThreadPoolGetFreeWorkers(srv->workers);
- *nWorkers = virThreadPoolGetCurrentWorkers(srv->workers);
- *nPrioWorkers = virThreadPoolGetPriorityWorkers(srv->workers);
- *jobQueueDepth = virThreadPoolGetJobQueueDepth(srv->workers);
+ if (srv->workers) {
+ *minWorkers = virThreadPoolGetMinWorkers(srv->workers);
+ *maxWorkers = virThreadPoolGetMaxWorkers(srv->workers);
+ *freeWorkers = virThreadPoolGetFreeWorkers(srv->workers);
+ *nWorkers = virThreadPoolGetCurrentWorkers(srv->workers);
+ *nPrioWorkers = virThreadPoolGetPriorityWorkers(srv->workers);
+ *jobQueueDepth = virThreadPoolGetJobQueueDepth(srv->workers);
+ } else {
+ *minWorkers = 0;
+ *maxWorkers = 0;
+ *freeWorkers = 0;
+ *nWorkers = 0;
+ *nPrioWorkers = 0;
+ *jobQueueDepth = 0;
+ }
virObjectUnlock(srv);
return 0;
--
2.13.6
4:31 p.m.
New subject: [libvirt] [PATCH 2/2] rpc: Fix segmentation fault when no worker pool is available
On Fri, Jun 15, 2018 at 01:39 PM +0200, Marc Hartmayer <mhartmay(a)linux.ibm.com>
wrote:
If srv->workers is a NULL pointer, as it is the case if there are
no
workers, then don't try to dereference it.
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
---
src/rpc/virnetserver.c | 22 +++++++++++++++-------
1 file changed, 15 insertions(+), 7 deletions(-)
diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
index 5ae809e372..be6f610880 100644
--- a/src/rpc/virnetserver.c
+++ b/src/rpc/virnetserver.c
@@ -933,13 +933,21 @@ virNetServerGetThreadPoolParameters(virNetServerPtr srv,
size_t *jobQueueDepth)
{
virObjectLock(srv);
-
- *minWorkers = virThreadPoolGetMinWorkers(srv->workers);
- *maxWorkers = virThreadPoolGetMaxWorkers(srv->workers);
- *freeWorkers = virThreadPoolGetFreeWorkers(srv->workers);
- *nWorkers = virThreadPoolGetCurrentWorkers(srv->workers);
- *nPrioWorkers = virThreadPoolGetPriorityWorkers(srv->workers);
- *jobQueueDepth = virThreadPoolGetJobQueueDepth(srv->workers);
+ if (srv->workers) {
+ *minWorkers = virThreadPoolGetMinWorkers(srv->workers);
+ *maxWorkers = virThreadPoolGetMaxWorkers(srv->workers);
+ *freeWorkers = virThreadPoolGetFreeWorkers(srv->workers);
+ *nWorkers = virThreadPoolGetCurrentWorkers(srv->workers);
+ *nPrioWorkers = virThreadPoolGetPriorityWorkers(srv->workers);
+ *jobQueueDepth = virThreadPoolGetJobQueueDepth(srv->workers);
+ } else {
+ *minWorkers = 0;
+ *maxWorkers = 0;
+ *freeWorkers = 0;
+ *nWorkers = 0;
+ *nPrioWorkers = 0;
+ *jobQueueDepth = 0;
+ }
virObjectUnlock(srv);
return 0;
--
2.13.6
After thinking again it probably makes more sense (and the code more
beautiful) to initialize the worker pool even for maxworker=0 (within
virNetServerNew) (=> we'll have to adapt virNetServerDispatchNewMessage
as well). BTW, there is also a segmentation fault in
virThreadPoolSetParameters… And currently it’s not possible to start
with maxworkers set to 0 and then increase it via
virThreadPoolSetParameters. These problems would all be fixed if we
would initialize a thread pool for max_workers = 0. Shall I revise this
patch this way?
Beste Grüße / Kind regards
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
10:47 a.m.
New subject: [libvirt] [PATCH 2/2] rpc: Fix segmentation fault when no worker pool is available
On Fri, Jun 15, 2018 at 03:31:34PM +0200, Marc Hartmayer wrote:
On Fri, Jun 15, 2018 at 01:39 PM +0200, Marc Hartmayer
<mhartmay(a)linux.ibm.com> wrote:
> If srv->workers is a NULL pointer, as it is the case if there are no
> workers, then don't try to dereference it.
>
> Signed-off-by: Marc Hartmayer <mhartmay(a)linux.ibm.com>
> Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
> ---
> src/rpc/virnetserver.c | 22 +++++++++++++++-------
> 1 file changed, 15 insertions(+), 7 deletions(-)
>
> diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
> index 5ae809e372..be6f610880 100644
> --- a/src/rpc/virnetserver.c
> +++ b/src/rpc/virnetserver.c
> @@ -933,13 +933,21 @@ virNetServerGetThreadPoolParameters(virNetServerPtr srv,
> size_t *jobQueueDepth)
> {
> virObjectLock(srv);
> -
> - *minWorkers = virThreadPoolGetMinWorkers(srv->workers);
> - *maxWorkers = virThreadPoolGetMaxWorkers(srv->workers);
> - *freeWorkers = virThreadPoolGetFreeWorkers(srv->workers);
> - *nWorkers = virThreadPoolGetCurrentWorkers(srv->workers);
> - *nPrioWorkers = virThreadPoolGetPriorityWorkers(srv->workers);
> - *jobQueueDepth = virThreadPoolGetJobQueueDepth(srv->workers);
> + if (srv->workers) {
> + *minWorkers = virThreadPoolGetMinWorkers(srv->workers);
> + *maxWorkers = virThreadPoolGetMaxWorkers(srv->workers);
> + *freeWorkers = virThreadPoolGetFreeWorkers(srv->workers);
> + *nWorkers = virThreadPoolGetCurrentWorkers(srv->workers);
> + *nPrioWorkers = virThreadPoolGetPriorityWorkers(srv->workers);
> + *jobQueueDepth = virThreadPoolGetJobQueueDepth(srv->workers);
> + } else {
> + *minWorkers = 0;
> + *maxWorkers = 0;
> + *freeWorkers = 0;
> + *nWorkers = 0;
> + *nPrioWorkers = 0;
> + *jobQueueDepth = 0;
> + }
>
> virObjectUnlock(srv);
> return 0;
> --
> 2.13.6
After thinking again it probably makes more sense (and the code more
beautiful) to initialize the worker pool even for maxworker=0 (within
I don't understand why should we do that. We don't even initialize it for
libvirtd server - the implications are clear - you don't have workers, you
don't get to process a job.
virNetServerNew) (=> we'll have to adapt
virNetServerDispatchNewMessage
as well). BTW, there is also a segmentation fault in
virThreadPoolSetParameters… And currently it’s not possible to start
with maxworkers set to 0 and then increase it via
Do I assume correctly that virNetServerDispatchNewMessage would allocate a new
worker if there was a request to process but the threadpool was empty? If so, I
don't see a reason to do that, why would anyone want to run with no workers?
They don't consume any resources, since they're waiting on a condition.
However, any segfaults or deadlocks must be fixed, I'll have a look at the
series as is, unless you've got a compelling reason why it's beneficial to run
with no workers at all.
Thanks,
Erik
1:38 p.m.
New subject: [libvirt] [PATCH 2/2] rpc: Fix segmentation fault when no worker pool is available
On Mon, Jun 18, 2018 at 09:47 AM +0200, Erik Skultety <eskultet(a)redhat.com> wrote:
On Fri, Jun 15, 2018 at 03:31:34PM +0200, Marc Hartmayer wrote:
> On Fri, Jun 15, 2018 at 01:39 PM +0200, Marc Hartmayer <mhartmay(a)linux.ibm.com>
wrote:
> > If srv->workers is a NULL pointer, as it is the case if there are no
> > workers, then don't try to dereference it.
> >
> > Signed-off-by: Marc Hartmayer <mhartmay(a)linux.ibm.com>
> > Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
> > ---
> > src/rpc/virnetserver.c | 22 +++++++++++++++-------
> > 1 file changed, 15 insertions(+), 7 deletions(-)
> >
> > diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
> > index 5ae809e372..be6f610880 100644
> > --- a/src/rpc/virnetserver.c
> > +++ b/src/rpc/virnetserver.c
> > @@ -933,13 +933,21 @@ virNetServerGetThreadPoolParameters(virNetServerPtr srv,
> > size_t *jobQueueDepth)
> > {
> > virObjectLock(srv);
> > -
> > - *minWorkers = virThreadPoolGetMinWorkers(srv->workers);
> > - *maxWorkers = virThreadPoolGetMaxWorkers(srv->workers);
> > - *freeWorkers = virThreadPoolGetFreeWorkers(srv->workers);
> > - *nWorkers = virThreadPoolGetCurrentWorkers(srv->workers);
> > - *nPrioWorkers = virThreadPoolGetPriorityWorkers(srv->workers);
> > - *jobQueueDepth = virThreadPoolGetJobQueueDepth(srv->workers);
> > + if (srv->workers) {
> > + *minWorkers = virThreadPoolGetMinWorkers(srv->workers);
> > + *maxWorkers = virThreadPoolGetMaxWorkers(srv->workers);
> > + *freeWorkers = virThreadPoolGetFreeWorkers(srv->workers);
> > + *nWorkers = virThreadPoolGetCurrentWorkers(srv->workers);
> > + *nPrioWorkers = virThreadPoolGetPriorityWorkers(srv->workers);
> > + *jobQueueDepth = virThreadPoolGetJobQueueDepth(srv->workers);
> > + } else {
> > + *minWorkers = 0;
> > + *maxWorkers = 0;
> > + *freeWorkers = 0;
> > + *nWorkers = 0;
> > + *nPrioWorkers = 0;
> > + *jobQueueDepth = 0;
> > + }
> >
> > virObjectUnlock(srv);
> > return 0;
> > --
> > 2.13.6
>
> After thinking again it probably makes more sense (and the code more
> beautiful) to initialize the worker pool even for maxworker=0 (within
I don't understand why should we do that.
Because right now there are several functionalities broken. Segmentation
faults in virNetServerGet/SetThreadPoolParameters, it’s not possible to
start with maxworkers=0 and then change it at runtime via
virNetServerSetThreadPoolParameters. Sure we can fix all these problems,
but then we’ve to introduce new code paths. Why can’t we just call
virThreadPoolNew(0, 0 , …)? This will only initialize the memory
structure of the pool and _no_ threads. The only change we’ve to do then
is to change the condition 'if (srv->workers)' of
'virNetServerDispatchNewMessage' to something like 'if
(virThreadPoolGetMaxWorkers(srv->workers) > 0)'.
We don't even initialize it for
libvirtd server - the implications are clear - you don't have workers, you
don't get to process a job.
Yep. I don’t want to change that behavior either.
> virNetServerNew) (=> we'll have to adapt virNetServerDispatchNewMessage
> as well). BTW, there is also a segmentation fault in
> virThreadPoolSetParameters… And currently it’s not possible to start
> with maxworkers set to 0 and then increase it via
Do I assume correctly that virNetServerDispatchNewMessage would allocate a new
worker if there was a request to process but the threadpool was empty?
Do you mean with my proposed changes? Or without?
If so, I
don't see a reason to do that, why would anyone want to run with no
workers?
Commit dff6d809fb6c851244ea07afd07f580d7320cc7a which actually
introduces this feature says:
“Allow RPC server to run single threaded
Refactor the RPC server dispatcher code so that if 'max_workers==0' the
entire server will run single threaded. This is useful for use cases
where there will only ever be 1 client connected which serializes its
requests”.
They don't consume any resources, since they're waiting on a
condition.
However, any segfaults or deadlocks must be fixed, I'll have a look at the
series as is, unless you've got a compelling reason why it's beneficial to run
with no workers at all.
See the commit message above.
Thanks,
Erik
Thank you for looking at it.
--
Beste Grüße / Kind regards
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
5:51 p.m.
New subject: [libvirt] [PATCH 2/2] rpc: Fix segmentation fault when no worker pool is available
On Mon, Jun 18, 2018 at 12:38:06PM +0200, Marc Hartmayer wrote:
On Mon, Jun 18, 2018 at 09:47 AM +0200, Erik Skultety
<eskultet(a)redhat.com> wrote:
> On Fri, Jun 15, 2018 at 03:31:34PM +0200, Marc Hartmayer wrote:
>> On Fri, Jun 15, 2018 at 01:39 PM +0200, Marc Hartmayer
<mhartmay(a)linux.ibm.com> wrote:
>> > If srv->workers is a NULL pointer, as it is the case if there are no
>> > workers, then don't try to dereference it.
>> >
>> > Signed-off-by: Marc Hartmayer <mhartmay(a)linux.ibm.com>
>> > Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
>> > ---
>> > src/rpc/virnetserver.c | 22 +++++++++++++++-------
>> > 1 file changed, 15 insertions(+), 7 deletions(-)
>> >
>> > diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
>> > index 5ae809e372..be6f610880 100644
>> > --- a/src/rpc/virnetserver.c
>> > +++ b/src/rpc/virnetserver.c
>> > @@ -933,13 +933,21 @@ virNetServerGetThreadPoolParameters(virNetServerPtr
srv,
>> > size_t *jobQueueDepth)
>> > {
>> > virObjectLock(srv);
>> > -
>> > - *minWorkers = virThreadPoolGetMinWorkers(srv->workers);
>> > - *maxWorkers = virThreadPoolGetMaxWorkers(srv->workers);
>> > - *freeWorkers = virThreadPoolGetFreeWorkers(srv->workers);
>> > - *nWorkers = virThreadPoolGetCurrentWorkers(srv->workers);
>> > - *nPrioWorkers = virThreadPoolGetPriorityWorkers(srv->workers);
>> > - *jobQueueDepth = virThreadPoolGetJobQueueDepth(srv->workers);
>> > + if (srv->workers) {
>> > + *minWorkers = virThreadPoolGetMinWorkers(srv->workers);
>> > + *maxWorkers = virThreadPoolGetMaxWorkers(srv->workers);
>> > + *freeWorkers = virThreadPoolGetFreeWorkers(srv->workers);
>> > + *nWorkers = virThreadPoolGetCurrentWorkers(srv->workers);
>> > + *nPrioWorkers = virThreadPoolGetPriorityWorkers(srv->workers);
>> > + *jobQueueDepth = virThreadPoolGetJobQueueDepth(srv->workers);
>> > + } else {
>> > + *minWorkers = 0;
>> > + *maxWorkers = 0;
>> > + *freeWorkers = 0;
>> > + *nWorkers = 0;
>> > + *nPrioWorkers = 0;
>> > + *jobQueueDepth = 0;
>> > + }
>> >
>> > virObjectUnlock(srv);
>> > return 0;
>> > --
>> > 2.13.6
>>
>> After thinking again it probably makes more sense (and the code more
>> beautiful) to initialize the worker pool even for maxworker=0 (within
>
> I don't understand why should we do that.
Because right now there are several functionalities broken. Segmentation
faults in virNetServerGet/SetThreadPoolParameters, it’s not possible to
start with maxworkers=0 and then change it at runtime via
Naturally, since no workers means noone to process the request, that is IMHO
the expected behaviour.
virNetServerSetThreadPoolParameters. Sure we can fix all these
problems,
but then we’ve to introduce new code paths. Why can’t we just call
virThreadPoolNew(0, 0 , …)? This will only initialize the memory
structure of the pool and _no_ threads. The only change we’ve to do then
I got the picture from the previous message, it's just I wasn't convinced.
is to change the condition 'if (srv->workers)' of
'virNetServerDispatchNewMessage' to something like 'if
(virThreadPoolGetMaxWorkers(srv->workers) > 0)'.
> We don't even initialize it for
> libvirtd server - the implications are clear - you don't have workers, you
> don't get to process a job.
Yep. I don’t want to change that behavior either.
>
>> virNetServerNew) (=> we'll have to adapt virNetServerDispatchNewMessage
>> as well). BTW, there is also a segmentation fault in
>> virThreadPoolSetParameters… And currently it’s not possible to start
>> with maxworkers set to 0 and then increase it via
>
> Do I assume correctly that virNetServerDispatchNewMessage would allocate a new
> worker if there was a request to process but the threadpool was empty?
Do you mean with my proposed changes? Or without?
> If so, I
> don't see a reason to do that, why would anyone want to run with no
> workers?
Commit dff6d809fb6c851244ea07afd07f580d7320cc7a which actually
introduces this feature says:
“Allow RPC server to run single threaded
Refactor the RPC server dispatcher code so that if 'max_workers==0' the
entire server will run single threaded. This is useful for use cases
where there will only ever be 1 client connected which serializes its
requests”.
Having said all of the above, even though I'm surprised we have something like
^this, because to me max_workers == 0 means something different (mind the
risks), we should remain consistent, so thanks for pointing that out.
Besides, the more I think about it, I guess it makes sense to be able to both
expand and shrink the worker pool as the user pleases, even with setting the
worker pool size to 0, it's true that setting it wrong one time (be it for
whatever reason) resulting in essentially locking yourself up.
Now, as long as noone has been using admin_max_workers == 0 in the config to
turn the admin interface off (/o\ why?), then I think we might want to make
a clear statement here that max_workers must be/will always be at least 1 to
make sure there's always a single thread running and able to process admin
requests, because without making this clear it might look confusing that
GetThreadPoolParameters would return max_workers=1 even though the caller tried
to set it to 0. With that said, would you need to perform any changes to the RPC
layer even if you adjusted SetThreadPoolParameters to always let at least one
worker in the pool?
Erik
5:55 p.m.
New subject: [libvirt] [PATCH 2/2] rpc: Fix segmentation fault when no worker pool is available
On Tue, Jun 19, 2018 at 04:51:13PM +0200, Erik Skultety wrote:
On Mon, Jun 18, 2018 at 12:38:06PM +0200, Marc Hartmayer wrote:
> On Mon, Jun 18, 2018 at 09:47 AM +0200, Erik Skultety <eskultet(a)redhat.com>
wrote:
> > On Fri, Jun 15, 2018 at 03:31:34PM +0200, Marc Hartmayer wrote:
> >> On Fri, Jun 15, 2018 at 01:39 PM +0200, Marc Hartmayer
<mhartmay(a)linux.ibm.com> wrote:
> >> > If srv->workers is a NULL pointer, as it is the case if there are
no
> >> > workers, then don't try to dereference it.
> >> >
> >> > Signed-off-by: Marc Hartmayer <mhartmay(a)linux.ibm.com>
> >> > Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
> >> > ---
> >> > src/rpc/virnetserver.c | 22 +++++++++++++++-------
> >> > 1 file changed, 15 insertions(+), 7 deletions(-)
> >> >
> >> > diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
> >> > index 5ae809e372..be6f610880 100644
> >> > --- a/src/rpc/virnetserver.c
> >> > +++ b/src/rpc/virnetserver.c
> >> > @@ -933,13 +933,21 @@
virNetServerGetThreadPoolParameters(virNetServerPtr srv,
> >> > size_t *jobQueueDepth)
> >> > {
> >> > virObjectLock(srv);
> >> > -
> >> > - *minWorkers = virThreadPoolGetMinWorkers(srv->workers);
> >> > - *maxWorkers = virThreadPoolGetMaxWorkers(srv->workers);
> >> > - *freeWorkers = virThreadPoolGetFreeWorkers(srv->workers);
> >> > - *nWorkers = virThreadPoolGetCurrentWorkers(srv->workers);
> >> > - *nPrioWorkers =
virThreadPoolGetPriorityWorkers(srv->workers);
> >> > - *jobQueueDepth = virThreadPoolGetJobQueueDepth(srv->workers);
> >> > + if (srv->workers) {
> >> > + *minWorkers = virThreadPoolGetMinWorkers(srv->workers);
> >> > + *maxWorkers = virThreadPoolGetMaxWorkers(srv->workers);
> >> > + *freeWorkers = virThreadPoolGetFreeWorkers(srv->workers);
> >> > + *nWorkers = virThreadPoolGetCurrentWorkers(srv->workers);
> >> > + *nPrioWorkers =
virThreadPoolGetPriorityWorkers(srv->workers);
> >> > + *jobQueueDepth =
virThreadPoolGetJobQueueDepth(srv->workers);
> >> > + } else {
> >> > + *minWorkers = 0;
> >> > + *maxWorkers = 0;
> >> > + *freeWorkers = 0;
> >> > + *nWorkers = 0;
> >> > + *nPrioWorkers = 0;
> >> > + *jobQueueDepth = 0;
> >> > + }
> >> >
> >> > virObjectUnlock(srv);
> >> > return 0;
> >> > --
> >> > 2.13.6
> >>
> >> After thinking again it probably makes more sense (and the code more
> >> beautiful) to initialize the worker pool even for maxworker=0 (within
> >
> > I don't understand why should we do that.
>
> Because right now there are several functionalities broken. Segmentation
> faults in virNetServerGet/SetThreadPoolParameters, it’s not possible to
> start with maxworkers=0 and then change it at runtime via
Naturally, since no workers means noone to process the request, that is IMHO
the expected behaviour.
Yes, a daemon should either run with no workers, or should run with
1 or more workers. It is not value to change between these two modes.
So if there's a codepath that lets you change from 0 -> 1 workers,
or the reverse, we should make sure that reports an error.
Essentially workers=0 is only intended for things like virtlockd
or virlogd which don't need to be multithreaded, or indeed must
*never* be multithreaded to avoid tickling kernel bugs like
virtlockd did in the past.
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 :|
6:03 p.m.
New subject: [libvirt] [PATCH 2/2] rpc: Fix segmentation fault when no worker pool is available
On Tue, Jun 19, 2018 at 03:55:09PM +0100, Daniel P. Berrangé wrote:
On Tue, Jun 19, 2018 at 04:51:13PM +0200, Erik Skultety wrote:
> On Mon, Jun 18, 2018 at 12:38:06PM +0200, Marc Hartmayer wrote:
> > On Mon, Jun 18, 2018 at 09:47 AM +0200, Erik Skultety
<eskultet(a)redhat.com> wrote:
> > > On Fri, Jun 15, 2018 at 03:31:34PM +0200, Marc Hartmayer wrote:
> > >> On Fri, Jun 15, 2018 at 01:39 PM +0200, Marc Hartmayer
<mhartmay(a)linux.ibm.com> wrote:
> > >> > If srv->workers is a NULL pointer, as it is the case if there
are no
> > >> > workers, then don't try to dereference it.
> > >> >
> > >> > Signed-off-by: Marc Hartmayer <mhartmay(a)linux.ibm.com>
> > >> > Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
> > >> > ---
> > >> > src/rpc/virnetserver.c | 22 +++++++++++++++-------
> > >> > 1 file changed, 15 insertions(+), 7 deletions(-)
> > >> >
> > >> > diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
> > >> > index 5ae809e372..be6f610880 100644
> > >> > --- a/src/rpc/virnetserver.c
> > >> > +++ b/src/rpc/virnetserver.c
> > >> > @@ -933,13 +933,21 @@
virNetServerGetThreadPoolParameters(virNetServerPtr srv,
> > >> > size_t *jobQueueDepth)
> > >> > {
> > >> > virObjectLock(srv);
> > >> > -
> > >> > - *minWorkers = virThreadPoolGetMinWorkers(srv->workers);
> > >> > - *maxWorkers = virThreadPoolGetMaxWorkers(srv->workers);
> > >> > - *freeWorkers =
virThreadPoolGetFreeWorkers(srv->workers);
> > >> > - *nWorkers =
virThreadPoolGetCurrentWorkers(srv->workers);
> > >> > - *nPrioWorkers =
virThreadPoolGetPriorityWorkers(srv->workers);
> > >> > - *jobQueueDepth =
virThreadPoolGetJobQueueDepth(srv->workers);
> > >> > + if (srv->workers) {
> > >> > + *minWorkers =
virThreadPoolGetMinWorkers(srv->workers);
> > >> > + *maxWorkers =
virThreadPoolGetMaxWorkers(srv->workers);
> > >> > + *freeWorkers =
virThreadPoolGetFreeWorkers(srv->workers);
> > >> > + *nWorkers =
virThreadPoolGetCurrentWorkers(srv->workers);
> > >> > + *nPrioWorkers =
virThreadPoolGetPriorityWorkers(srv->workers);
> > >> > + *jobQueueDepth =
virThreadPoolGetJobQueueDepth(srv->workers);
> > >> > + } else {
> > >> > + *minWorkers = 0;
> > >> > + *maxWorkers = 0;
> > >> > + *freeWorkers = 0;
> > >> > + *nWorkers = 0;
> > >> > + *nPrioWorkers = 0;
> > >> > + *jobQueueDepth = 0;
> > >> > + }
> > >> >
> > >> > virObjectUnlock(srv);
> > >> > return 0;
> > >> > --
> > >> > 2.13.6
> > >>
> > >> After thinking again it probably makes more sense (and the code more
> > >> beautiful) to initialize the worker pool even for maxworker=0 (within
> > >
> > > I don't understand why should we do that.
> >
> > Because right now there are several functionalities broken. Segmentation
> > faults in virNetServerGet/SetThreadPoolParameters, it’s not possible to
> > start with maxworkers=0 and then change it at runtime via
>
> Naturally, since no workers means noone to process the request, that is IMHO
> the expected behaviour.
Yes, a daemon should either run with no workers, or should run with
1 or more workers. It is not value to change between these two modes.
So if there's a codepath that lets you change from 0 -> 1 workers,
or the reverse, we should make sure that reports an error.
Essentially workers=0 is only intended for things like virtlockd
or virlogd which don't need to be multithreaded, or indeed must
*never* be multithreaded to avoid tickling kernel bugs like
virtlockd did in the past.
Also note that workers=0 will cause libvirtd to deadlock, because
the QEMU driver (and others too) assume that they run in a seperate
thread from the main event loop.
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 :|
6:27 p.m.
New subject: [libvirt] [PATCH 2/2] rpc: Fix segmentation fault when no worker pool is available
On Tue, Jun 19, 2018 at 05:03 PM +0200, "Daniel P. Berrangé"
<berrange(a)redhat.com> wrote:
On Tue, Jun 19, 2018 at 03:55:09PM +0100, Daniel P. Berrangé wrote:
> On Tue, Jun 19, 2018 at 04:51:13PM +0200, Erik Skultety wrote:
> > On Mon, Jun 18, 2018 at 12:38:06PM +0200, Marc Hartmayer wrote:
> > > On Mon, Jun 18, 2018 at 09:47 AM +0200, Erik Skultety
<eskultet(a)redhat.com> wrote:
> > > > On Fri, Jun 15, 2018 at 03:31:34PM +0200, Marc Hartmayer wrote:
> > > >> On Fri, Jun 15, 2018 at 01:39 PM +0200, Marc Hartmayer
<mhartmay(a)linux.ibm.com> wrote:
> > > >> > If srv->workers is a NULL pointer, as it is the case if
there are no
> > > >> > workers, then don't try to dereference it.
> > > >> >
> > > >> > Signed-off-by: Marc Hartmayer <mhartmay(a)linux.ibm.com>
> > > >> > Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
> > > >> > ---
> > > >> > src/rpc/virnetserver.c | 22 +++++++++++++++-------
> > > >> > 1 file changed, 15 insertions(+), 7 deletions(-)
> > > >> >
> > > >> > diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
> > > >> > index 5ae809e372..be6f610880 100644
> > > >> > --- a/src/rpc/virnetserver.c
> > > >> > +++ b/src/rpc/virnetserver.c
> > > >> > @@ -933,13 +933,21 @@
virNetServerGetThreadPoolParameters(virNetServerPtr srv,
> > > >> > size_t *jobQueueDepth)
> > > >> > {
> > > >> > virObjectLock(srv);
> > > >> > -
> > > >> > - *minWorkers =
virThreadPoolGetMinWorkers(srv->workers);
> > > >> > - *maxWorkers =
virThreadPoolGetMaxWorkers(srv->workers);
> > > >> > - *freeWorkers =
virThreadPoolGetFreeWorkers(srv->workers);
> > > >> > - *nWorkers =
virThreadPoolGetCurrentWorkers(srv->workers);
> > > >> > - *nPrioWorkers =
virThreadPoolGetPriorityWorkers(srv->workers);
> > > >> > - *jobQueueDepth =
virThreadPoolGetJobQueueDepth(srv->workers);
> > > >> > + if (srv->workers) {
> > > >> > + *minWorkers =
virThreadPoolGetMinWorkers(srv->workers);
> > > >> > + *maxWorkers =
virThreadPoolGetMaxWorkers(srv->workers);
> > > >> > + *freeWorkers =
virThreadPoolGetFreeWorkers(srv->workers);
> > > >> > + *nWorkers =
virThreadPoolGetCurrentWorkers(srv->workers);
> > > >> > + *nPrioWorkers =
virThreadPoolGetPriorityWorkers(srv->workers);
> > > >> > + *jobQueueDepth =
virThreadPoolGetJobQueueDepth(srv->workers);
> > > >> > + } else {
> > > >> > + *minWorkers = 0;
> > > >> > + *maxWorkers = 0;
> > > >> > + *freeWorkers = 0;
> > > >> > + *nWorkers = 0;
> > > >> > + *nPrioWorkers = 0;
> > > >> > + *jobQueueDepth = 0;
> > > >> > + }
> > > >> >
> > > >> > virObjectUnlock(srv);
> > > >> > return 0;
> > > >> > --
> > > >> > 2.13.6
> > > >>
> > > >> After thinking again it probably makes more sense (and the code
more
> > > >> beautiful) to initialize the worker pool even for maxworker=0
(within
> > > >
> > > > I don't understand why should we do that.
> > >
> > > Because right now there are several functionalities broken. Segmentation
> > > faults in virNetServerGet/SetThreadPoolParameters, it’s not possible to
> > > start with maxworkers=0 and then change it at runtime via
> >
> > Naturally, since no workers means noone to process the request, that is IMHO
> > the expected behaviour.
>
> Yes, a daemon should either run with no workers, or should run with
> 1 or more workers. It is not value to change between these two modes.
>
> So if there's a codepath that lets you change from 0 -> 1 workers,
> or the reverse, we should make sure that reports an error.
>
> Essentially workers=0 is only intended for things like virtlockd
> or virlogd which don't need to be multithreaded, or indeed must
> *never* be multithreaded to avoid tickling kernel bugs like
> virtlockd did in the past.
Also note that workers=0 will cause libvirtd to deadlock, because
the QEMU driver (and others too) assume that they run in a seperate
thread from the main event loop.
Yep, I know. What is your preferred way to avoid this?
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 :|
--
Beste Grüße / Kind regards
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
12:14 p.m.
New subject: [libvirt] [PATCH 2/2] rpc: Fix segmentation fault when no worker pool is available
On Tue, Jun 19, 2018 at 05:03 PM +0200, "Daniel P. Berrangé"
<berrange(a)redhat.com> wrote:
On Tue, Jun 19, 2018 at 03:55:09PM +0100, Daniel P. Berrangé wrote:
> On Tue, Jun 19, 2018 at 04:51:13PM +0200, Erik Skultety wrote:
> > On Mon, Jun 18, 2018 at 12:38:06PM +0200, Marc Hartmayer wrote:
> > > On Mon, Jun 18, 2018 at 09:47 AM +0200, Erik Skultety
<eskultet(a)redhat.com> wrote:
> > > > On Fri, Jun 15, 2018 at 03:31:34PM +0200, Marc Hartmayer wrote:
> > > >> On Fri, Jun 15, 2018 at 01:39 PM +0200, Marc Hartmayer
<mhartmay(a)linux.ibm.com> wrote:
> > > >> > If srv->workers is a NULL pointer, as it is the case if
there are no
> > > >> > workers, then don't try to dereference it.
> > > >> >
> > > >> > Signed-off-by: Marc Hartmayer <mhartmay(a)linux.ibm.com>
> > > >> > Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
> > > >> > ---
> > > >> > src/rpc/virnetserver.c | 22 +++++++++++++++-------
> > > >> > 1 file changed, 15 insertions(+), 7 deletions(-)
> > > >> >
> > > >> > diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
> > > >> > index 5ae809e372..be6f610880 100644
> > > >> > --- a/src/rpc/virnetserver.c
> > > >> > +++ b/src/rpc/virnetserver.c
> > > >> > @@ -933,13 +933,21 @@
virNetServerGetThreadPoolParameters(virNetServerPtr srv,
> > > >> > size_t *jobQueueDepth)
> > > >> > {
> > > >> > virObjectLock(srv);
> > > >> > -
> > > >> > - *minWorkers =
virThreadPoolGetMinWorkers(srv->workers);
> > > >> > - *maxWorkers =
virThreadPoolGetMaxWorkers(srv->workers);
> > > >> > - *freeWorkers =
virThreadPoolGetFreeWorkers(srv->workers);
> > > >> > - *nWorkers =
virThreadPoolGetCurrentWorkers(srv->workers);
> > > >> > - *nPrioWorkers =
virThreadPoolGetPriorityWorkers(srv->workers);
> > > >> > - *jobQueueDepth =
virThreadPoolGetJobQueueDepth(srv->workers);
> > > >> > + if (srv->workers) {
> > > >> > + *minWorkers =
virThreadPoolGetMinWorkers(srv->workers);
> > > >> > + *maxWorkers =
virThreadPoolGetMaxWorkers(srv->workers);
> > > >> > + *freeWorkers =
virThreadPoolGetFreeWorkers(srv->workers);
> > > >> > + *nWorkers =
virThreadPoolGetCurrentWorkers(srv->workers);
> > > >> > + *nPrioWorkers =
virThreadPoolGetPriorityWorkers(srv->workers);
> > > >> > + *jobQueueDepth =
virThreadPoolGetJobQueueDepth(srv->workers);
> > > >> > + } else {
> > > >> > + *minWorkers = 0;
> > > >> > + *maxWorkers = 0;
> > > >> > + *freeWorkers = 0;
> > > >> > + *nWorkers = 0;
> > > >> > + *nPrioWorkers = 0;
> > > >> > + *jobQueueDepth = 0;
> > > >> > + }
> > > >> >
> > > >> > virObjectUnlock(srv);
> > > >> > return 0;
> > > >> > --
> > > >> > 2.13.6
> > > >>
> > > >> After thinking again it probably makes more sense (and the code
more
> > > >> beautiful) to initialize the worker pool even for maxworker=0
(within
> > > >
> > > > I don't understand why should we do that.
> > >
> > > Because right now there are several functionalities broken. Segmentation
> > > faults in virNetServerGet/SetThreadPoolParameters, it’s not possible to
> > > start with maxworkers=0 and then change it at runtime via
> >
> > Naturally, since no workers means noone to process the request, that is IMHO
> > the expected behaviour.
>
> Yes, a daemon should either run with no workers, or should run with
> 1 or more workers. It is not value to change between these two modes.
>
> So if there's a codepath that lets you change from 0 -> 1 workers,
> or the reverse, we should make sure that reports an error.
>
> Essentially workers=0 is only intended for things like virtlockd
> or virlogd which don't need to be multithreaded, or indeed must
> *never* be multithreaded to avoid tickling kernel bugs like
> virtlockd did in the past.
Also note that workers=0 will cause libvirtd to deadlock, because
the QEMU driver (and others too) assume that they run in a seperate
thread from the main event loop.
Shall we then disallow this value for the option "max_workers" in
libvirtd.conf?
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 :|
--
Beste Grüße / Kind regards
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
12:20 p.m.
New subject: [libvirt] [PATCH 2/2] rpc: Fix segmentation fault when no worker pool is available
On Wed, Jun 20, 2018 at 11:14:41AM +0200, Marc Hartmayer wrote:
On Tue, Jun 19, 2018 at 05:03 PM +0200, "Daniel P.
Berrangé" <berrange(a)redhat.com> wrote:
> On Tue, Jun 19, 2018 at 03:55:09PM +0100, Daniel P. Berrangé wrote:
>> On Tue, Jun 19, 2018 at 04:51:13PM +0200, Erik Skultety wrote:
>> > On Mon, Jun 18, 2018 at 12:38:06PM +0200, Marc Hartmayer wrote:
>> > > On Mon, Jun 18, 2018 at 09:47 AM +0200, Erik Skultety
<eskultet(a)redhat.com> wrote:
>> > > > On Fri, Jun 15, 2018 at 03:31:34PM +0200, Marc Hartmayer wrote:
>> > > >> On Fri, Jun 15, 2018 at 01:39 PM +0200, Marc Hartmayer
<mhartmay(a)linux.ibm.com> wrote:
>> > > >> > If srv->workers is a NULL pointer, as it is the case
if there are no
>> > > >> > workers, then don't try to dereference it.
>> > > >> >
>> > > >> > Signed-off-by: Marc Hartmayer
<mhartmay(a)linux.ibm.com>
>> > > >> > Reviewed-by: Boris Fiuczynski
<fiuczy(a)linux.ibm.com>
>> > > >> > ---
>> > > >> > src/rpc/virnetserver.c | 22 +++++++++++++++-------
>> > > >> > 1 file changed, 15 insertions(+), 7 deletions(-)
>> > > >> >
>> > > >> > diff --git a/src/rpc/virnetserver.c
b/src/rpc/virnetserver.c
>> > > >> > index 5ae809e372..be6f610880 100644
>> > > >> > --- a/src/rpc/virnetserver.c
>> > > >> > +++ b/src/rpc/virnetserver.c
>> > > >> > @@ -933,13 +933,21 @@
virNetServerGetThreadPoolParameters(virNetServerPtr srv,
>> > > >> > size_t
*jobQueueDepth)
>> > > >> > {
>> > > >> > virObjectLock(srv);
>> > > >> > -
>> > > >> > - *minWorkers =
virThreadPoolGetMinWorkers(srv->workers);
>> > > >> > - *maxWorkers =
virThreadPoolGetMaxWorkers(srv->workers);
>> > > >> > - *freeWorkers =
virThreadPoolGetFreeWorkers(srv->workers);
>> > > >> > - *nWorkers =
virThreadPoolGetCurrentWorkers(srv->workers);
>> > > >> > - *nPrioWorkers =
virThreadPoolGetPriorityWorkers(srv->workers);
>> > > >> > - *jobQueueDepth =
virThreadPoolGetJobQueueDepth(srv->workers);
>> > > >> > + if (srv->workers) {
>> > > >> > + *minWorkers =
virThreadPoolGetMinWorkers(srv->workers);
>> > > >> > + *maxWorkers =
virThreadPoolGetMaxWorkers(srv->workers);
>> > > >> > + *freeWorkers =
virThreadPoolGetFreeWorkers(srv->workers);
>> > > >> > + *nWorkers =
virThreadPoolGetCurrentWorkers(srv->workers);
>> > > >> > + *nPrioWorkers =
virThreadPoolGetPriorityWorkers(srv->workers);
>> > > >> > + *jobQueueDepth =
virThreadPoolGetJobQueueDepth(srv->workers);
>> > > >> > + } else {
>> > > >> > + *minWorkers = 0;
>> > > >> > + *maxWorkers = 0;
>> > > >> > + *freeWorkers = 0;
>> > > >> > + *nWorkers = 0;
>> > > >> > + *nPrioWorkers = 0;
>> > > >> > + *jobQueueDepth = 0;
>> > > >> > + }
>> > > >> >
>> > > >> > virObjectUnlock(srv);
>> > > >> > return 0;
>> > > >> > --
>> > > >> > 2.13.6
>> > > >>
>> > > >> After thinking again it probably makes more sense (and the
code more
>> > > >> beautiful) to initialize the worker pool even for maxworker=0
(within
>> > > >
>> > > > I don't understand why should we do that.
>> > >
>> > > Because right now there are several functionalities broken.
Segmentation
>> > > faults in virNetServerGet/SetThreadPoolParameters, it’s not possible
to
>> > > start with maxworkers=0 and then change it at runtime via
>> >
>> > Naturally, since no workers means noone to process the request, that is
IMHO
>> > the expected behaviour.
>>
>> Yes, a daemon should either run with no workers, or should run with
>> 1 or more workers. It is not value to change between these two modes.
>>
>> So if there's a codepath that lets you change from 0 -> 1 workers,
>> or the reverse, we should make sure that reports an error.
>>
>> Essentially workers=0 is only intended for things like virtlockd
>> or virlogd which don't need to be multithreaded, or indeed must
>> *never* be multithreaded to avoid tickling kernel bugs like
>> virtlockd did in the past.
>
> Also note that workers=0 will cause libvirtd to deadlock, because
> the QEMU driver (and others too) assume that they run in a seperate
> thread from the main event loop.
Shall we then disallow this value for the option "max_workers" in
libvirtd.conf?
Yes, we should, as it can't work correctly.
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 :|
6:18 p.m.
New subject: [libvirt] [PATCH 2/2] rpc: Fix segmentation fault when no worker pool is available
On Tue, Jun 19, 2018 at 04:55 PM +0200, "Daniel P. Berrangé"
<berrange(a)redhat.com> wrote:
On Tue, Jun 19, 2018 at 04:51:13PM +0200, Erik Skultety wrote:
> On Mon, Jun 18, 2018 at 12:38:06PM +0200, Marc Hartmayer wrote:
> > On Mon, Jun 18, 2018 at 09:47 AM +0200, Erik Skultety
<eskultet(a)redhat.com> wrote:
> > > On Fri, Jun 15, 2018 at 03:31:34PM +0200, Marc Hartmayer wrote:
> > >> On Fri, Jun 15, 2018 at 01:39 PM +0200, Marc Hartmayer
<mhartmay(a)linux.ibm.com> wrote:
> > >> > If srv->workers is a NULL pointer, as it is the case if there
are no
> > >> > workers, then don't try to dereference it.
> > >> >
> > >> > Signed-off-by: Marc Hartmayer <mhartmay(a)linux.ibm.com>
> > >> > Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
> > >> > ---
> > >> > src/rpc/virnetserver.c | 22 +++++++++++++++-------
> > >> > 1 file changed, 15 insertions(+), 7 deletions(-)
> > >> >
> > >> > diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
> > >> > index 5ae809e372..be6f610880 100644
> > >> > --- a/src/rpc/virnetserver.c
> > >> > +++ b/src/rpc/virnetserver.c
> > >> > @@ -933,13 +933,21 @@
virNetServerGetThreadPoolParameters(virNetServerPtr srv,
> > >> > size_t *jobQueueDepth)
> > >> > {
> > >> > virObjectLock(srv);
> > >> > -
> > >> > - *minWorkers = virThreadPoolGetMinWorkers(srv->workers);
> > >> > - *maxWorkers = virThreadPoolGetMaxWorkers(srv->workers);
> > >> > - *freeWorkers = virThreadPoolGetFreeWorkers(srv->workers);
> > >> > - *nWorkers = virThreadPoolGetCurrentWorkers(srv->workers);
> > >> > - *nPrioWorkers =
virThreadPoolGetPriorityWorkers(srv->workers);
> > >> > - *jobQueueDepth =
virThreadPoolGetJobQueueDepth(srv->workers);
> > >> > + if (srv->workers) {
> > >> > + *minWorkers =
virThreadPoolGetMinWorkers(srv->workers);
> > >> > + *maxWorkers =
virThreadPoolGetMaxWorkers(srv->workers);
> > >> > + *freeWorkers =
virThreadPoolGetFreeWorkers(srv->workers);
> > >> > + *nWorkers =
virThreadPoolGetCurrentWorkers(srv->workers);
> > >> > + *nPrioWorkers =
virThreadPoolGetPriorityWorkers(srv->workers);
> > >> > + *jobQueueDepth =
virThreadPoolGetJobQueueDepth(srv->workers);
> > >> > + } else {
> > >> > + *minWorkers = 0;
> > >> > + *maxWorkers = 0;
> > >> > + *freeWorkers = 0;
> > >> > + *nWorkers = 0;
> > >> > + *nPrioWorkers = 0;
> > >> > + *jobQueueDepth = 0;
> > >> > + }
> > >> >
> > >> > virObjectUnlock(srv);
> > >> > return 0;
> > >> > --
> > >> > 2.13.6
> > >>
> > >> After thinking again it probably makes more sense (and the code more
> > >> beautiful) to initialize the worker pool even for maxworker=0 (within
> > >
> > > I don't understand why should we do that.
> >
> > Because right now there are several functionalities broken. Segmentation
> > faults in virNetServerGet/SetThreadPoolParameters, it’s not possible to
> > start with maxworkers=0 and then change it at runtime via
>
> Naturally, since no workers means noone to process the request, that is IMHO
> the expected behaviour.
Yes, a daemon should either run with no workers, or should run with
1 or more workers. It is not value to change between these two modes.
Okay.
So if there's a codepath that lets you change from 0 -> 1 workers,
or the reverse, we should make sure that reports an error.
virThreadPoolSetParameters allows this change... Does it make sense to
you to allow an empty thread pool (just to keep the values) but to
prohibit a change from 0 -> i workers (i != 0) (and the reverse)? The
condition in virNetServerDispatchNewMessage would be something like 'if
(virThreadPoolGetMaxWorkers(srv->workers) > 0) {' instead of 'if
(srv->workers)'. If yes, this would, IMHO, simplify the code paths and
would be much safer against null pointer dereferencing.
Essentially workers=0 is only intended for things like virtlockd
or virlogd which don't need to be multithreaded, or indeed must
*never* be multithreaded to avoid tickling kernel bugs like
virtlockd did in the past.
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 :|
--
Beste Grüße / Kind regards
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
6:30 p.m.
New subject: [libvirt] [PATCH 2/2] rpc: Fix segmentation fault when no worker pool is available
On Tue, Jun 19, 2018 at 05:18:17PM +0200, Marc Hartmayer wrote:
On Tue, Jun 19, 2018 at 04:55 PM +0200, "Daniel P.
Berrangé" <berrange(a)redhat.com> wrote:
> On Tue, Jun 19, 2018 at 04:51:13PM +0200, Erik Skultety wrote:
>> On Mon, Jun 18, 2018 at 12:38:06PM +0200, Marc Hartmayer wrote:
>> > On Mon, Jun 18, 2018 at 09:47 AM +0200, Erik Skultety
<eskultet(a)redhat.com> wrote:
>> > > On Fri, Jun 15, 2018 at 03:31:34PM +0200, Marc Hartmayer wrote:
>> > >> On Fri, Jun 15, 2018 at 01:39 PM +0200, Marc Hartmayer
<mhartmay(a)linux.ibm.com> wrote:
>> > >> > If srv->workers is a NULL pointer, as it is the case if
there are no
>> > >> > workers, then don't try to dereference it.
>> > >> >
>> > >> > Signed-off-by: Marc Hartmayer <mhartmay(a)linux.ibm.com>
>> > >> > Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
>> > >> > ---
>> > >> > src/rpc/virnetserver.c | 22 +++++++++++++++-------
>> > >> > 1 file changed, 15 insertions(+), 7 deletions(-)
>> > >> >
>> > >> > diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
>> > >> > index 5ae809e372..be6f610880 100644
>> > >> > --- a/src/rpc/virnetserver.c
>> > >> > +++ b/src/rpc/virnetserver.c
>> > >> > @@ -933,13 +933,21 @@
virNetServerGetThreadPoolParameters(virNetServerPtr srv,
>> > >> > size_t *jobQueueDepth)
>> > >> > {
>> > >> > virObjectLock(srv);
>> > >> > -
>> > >> > - *minWorkers =
virThreadPoolGetMinWorkers(srv->workers);
>> > >> > - *maxWorkers =
virThreadPoolGetMaxWorkers(srv->workers);
>> > >> > - *freeWorkers =
virThreadPoolGetFreeWorkers(srv->workers);
>> > >> > - *nWorkers =
virThreadPoolGetCurrentWorkers(srv->workers);
>> > >> > - *nPrioWorkers =
virThreadPoolGetPriorityWorkers(srv->workers);
>> > >> > - *jobQueueDepth =
virThreadPoolGetJobQueueDepth(srv->workers);
>> > >> > + if (srv->workers) {
>> > >> > + *minWorkers =
virThreadPoolGetMinWorkers(srv->workers);
>> > >> > + *maxWorkers =
virThreadPoolGetMaxWorkers(srv->workers);
>> > >> > + *freeWorkers =
virThreadPoolGetFreeWorkers(srv->workers);
>> > >> > + *nWorkers =
virThreadPoolGetCurrentWorkers(srv->workers);
>> > >> > + *nPrioWorkers =
virThreadPoolGetPriorityWorkers(srv->workers);
>> > >> > + *jobQueueDepth =
virThreadPoolGetJobQueueDepth(srv->workers);
>> > >> > + } else {
>> > >> > + *minWorkers = 0;
>> > >> > + *maxWorkers = 0;
>> > >> > + *freeWorkers = 0;
>> > >> > + *nWorkers = 0;
>> > >> > + *nPrioWorkers = 0;
>> > >> > + *jobQueueDepth = 0;
>> > >> > + }
>> > >> >
>> > >> > virObjectUnlock(srv);
>> > >> > return 0;
>> > >> > --
>> > >> > 2.13.6
>> > >>
>> > >> After thinking again it probably makes more sense (and the code
more
>> > >> beautiful) to initialize the worker pool even for maxworker=0
(within
>> > >
>> > > I don't understand why should we do that.
>> >
>> > Because right now there are several functionalities broken. Segmentation
>> > faults in virNetServerGet/SetThreadPoolParameters, it’s not possible to
>> > start with maxworkers=0 and then change it at runtime via
>>
>> Naturally, since no workers means noone to process the request, that is IMHO
>> the expected behaviour.
>
> Yes, a daemon should either run with no workers, or should run with
> 1 or more workers. It is not value to change between these two modes.
Okay.
>
> So if there's a codepath that lets you change from 0 -> 1 workers,
> or the reverse, we should make sure that reports an error.
virThreadPoolSetParameters allows this change... Does it make sense to
you to allow an empty thread pool (just to keep the values) but to
prohibit a change from 0 -> i workers (i != 0) (and the reverse)? The
condition in virNetServerDispatchNewMessage would be something like 'if
(virThreadPoolGetMaxWorkers(srv->workers) > 0) {' instead of 'if
(srv->workers)'. If yes, this would, IMHO, simplify the code paths and
would be much safer against null pointer dereferencing.
virThreadPoolSetParameters should immediately return an error if
the caller tries to switch between 0 and non-zero in either
direction.
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 :|
6:50 p.m.
New subject: [libvirt] [PATCH 2/2] rpc: Fix segmentation fault when no worker pool is available
On Tue, Jun 19, 2018 at 05:30 PM +0200, "Daniel P. Berrangé"
<berrange(a)redhat.com> wrote:
On Tue, Jun 19, 2018 at 05:18:17PM +0200, Marc Hartmayer wrote:
> On Tue, Jun 19, 2018 at 04:55 PM +0200, "Daniel P. Berrangé"
<berrange(a)redhat.com> wrote:
> > On Tue, Jun 19, 2018 at 04:51:13PM +0200, Erik Skultety wrote:
> >> On Mon, Jun 18, 2018 at 12:38:06PM +0200, Marc Hartmayer wrote:
> >> > On Mon, Jun 18, 2018 at 09:47 AM +0200, Erik Skultety
<eskultet(a)redhat.com> wrote:
> >> > > On Fri, Jun 15, 2018 at 03:31:34PM +0200, Marc Hartmayer wrote:
> >> > >> On Fri, Jun 15, 2018 at 01:39 PM +0200, Marc Hartmayer
<mhartmay(a)linux.ibm.com> wrote:
> >> > >> > If srv->workers is a NULL pointer, as it is the case
if there are no
> >> > >> > workers, then don't try to dereference it.
> >> > >> >
> >> > >> > Signed-off-by: Marc Hartmayer
<mhartmay(a)linux.ibm.com>
> >> > >> > Reviewed-by: Boris Fiuczynski
<fiuczy(a)linux.ibm.com>
> >> > >> > ---
> >> > >> > src/rpc/virnetserver.c | 22 +++++++++++++++-------
> >> > >> > 1 file changed, 15 insertions(+), 7 deletions(-)
> >> > >> >
> >> > >> > diff --git a/src/rpc/virnetserver.c
b/src/rpc/virnetserver.c
> >> > >> > index 5ae809e372..be6f610880 100644
> >> > >> > --- a/src/rpc/virnetserver.c
> >> > >> > +++ b/src/rpc/virnetserver.c
> >> > >> > @@ -933,13 +933,21 @@
virNetServerGetThreadPoolParameters(virNetServerPtr srv,
> >> > >> > size_t
*jobQueueDepth)
> >> > >> > {
> >> > >> > virObjectLock(srv);
> >> > >> > -
> >> > >> > - *minWorkers =
virThreadPoolGetMinWorkers(srv->workers);
> >> > >> > - *maxWorkers =
virThreadPoolGetMaxWorkers(srv->workers);
> >> > >> > - *freeWorkers =
virThreadPoolGetFreeWorkers(srv->workers);
> >> > >> > - *nWorkers =
virThreadPoolGetCurrentWorkers(srv->workers);
> >> > >> > - *nPrioWorkers =
virThreadPoolGetPriorityWorkers(srv->workers);
> >> > >> > - *jobQueueDepth =
virThreadPoolGetJobQueueDepth(srv->workers);
> >> > >> > + if (srv->workers) {
> >> > >> > + *minWorkers =
virThreadPoolGetMinWorkers(srv->workers);
> >> > >> > + *maxWorkers =
virThreadPoolGetMaxWorkers(srv->workers);
> >> > >> > + *freeWorkers =
virThreadPoolGetFreeWorkers(srv->workers);
> >> > >> > + *nWorkers =
virThreadPoolGetCurrentWorkers(srv->workers);
> >> > >> > + *nPrioWorkers =
virThreadPoolGetPriorityWorkers(srv->workers);
> >> > >> > + *jobQueueDepth =
virThreadPoolGetJobQueueDepth(srv->workers);
> >> > >> > + } else {
> >> > >> > + *minWorkers = 0;
> >> > >> > + *maxWorkers = 0;
> >> > >> > + *freeWorkers = 0;
> >> > >> > + *nWorkers = 0;
> >> > >> > + *nPrioWorkers = 0;
> >> > >> > + *jobQueueDepth = 0;
> >> > >> > + }
> >> > >> >
> >> > >> > virObjectUnlock(srv);
> >> > >> > return 0;
> >> > >> > --
> >> > >> > 2.13.6
> >> > >>
> >> > >> After thinking again it probably makes more sense (and the
code more
> >> > >> beautiful) to initialize the worker pool even for maxworker=0
(within
> >> > >
> >> > > I don't understand why should we do that.
> >> >
> >> > Because right now there are several functionalities broken.
Segmentation
> >> > faults in virNetServerGet/SetThreadPoolParameters, it’s not possible
to
> >> > start with maxworkers=0 and then change it at runtime via
> >>
> >> Naturally, since no workers means noone to process the request, that is
IMHO
> >> the expected behaviour.
> >
> > Yes, a daemon should either run with no workers, or should run with
> > 1 or more workers. It is not value to change between these two modes.
>
> Okay.
>
> >
> > So if there's a codepath that lets you change from 0 -> 1 workers,
> > or the reverse, we should make sure that reports an error.
>
> virThreadPoolSetParameters allows this change... Does it make sense to
> you to allow an empty thread pool (just to keep the values) but to
> prohibit a change from 0 -> i workers (i != 0) (and the reverse)? The
> condition in virNetServerDispatchNewMessage would be something like 'if
> (virThreadPoolGetMaxWorkers(srv->workers) > 0) {' instead of 'if
> (srv->workers)'. If yes, this would, IMHO, simplify the code paths and
> would be much safer against null pointer dereferencing.
virThreadPoolSetParameters should immediately return an error if
the caller tries to switch between 0 and non-zero in either
direction.
Yes, that’s clear (at least since your last answer :) ). In my opinion
it still makes sense to initialize an empty virThreadPool (which will
never be allowed to have threads) since this would avoid constructions
like
“if (virJSONValueObjectAppendNumberUint(object, "min_workers",
srv->workers == NULL ? 0 :
virThreadPoolGetMinWorkers(srv->workers)) < 0) {”
and even more important it would avoid segmentation faults (see the
original patch).
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 :|
--
Beste Grüße / Kind regards
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
2:27 p.m.
New subject: [libvirt] [PATCH 2/2] rpc: Fix segmentation fault when no worker pool is available
On Mon, Jun 18, 2018 at 09:47 AM +0200, Erik Skultety <eskultet(a)redhat.com> wrote:
On Fri, Jun 15, 2018 at 03:31:34PM +0200, Marc Hartmayer wrote:
> On Fri, Jun 15, 2018 at 01:39 PM +0200, Marc Hartmayer <mhartmay(a)linux.ibm.com>
wrote:
> > If srv->workers is a NULL pointer, as it is the case if there are no
> > workers, then don't try to dereference it.
> >
> > Signed-off-by: Marc Hartmayer <mhartmay(a)linux.ibm.com>
> > Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
> > ---
> > src/rpc/virnetserver.c | 22 +++++++++++++++-------
> > 1 file changed, 15 insertions(+), 7 deletions(-)
> >
> > diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
> > index 5ae809e372..be6f610880 100644
> > --- a/src/rpc/virnetserver.c
> > +++ b/src/rpc/virnetserver.c
> > @@ -933,13 +933,21 @@ virNetServerGetThreadPoolParameters(virNetServerPtr srv,
> > size_t *jobQueueDepth)
> > {
> > virObjectLock(srv);
> > -
> > - *minWorkers = virThreadPoolGetMinWorkers(srv->workers);
> > - *maxWorkers = virThreadPoolGetMaxWorkers(srv->workers);
> > - *freeWorkers = virThreadPoolGetFreeWorkers(srv->workers);
> > - *nWorkers = virThreadPoolGetCurrentWorkers(srv->workers);
> > - *nPrioWorkers = virThreadPoolGetPriorityWorkers(srv->workers);
> > - *jobQueueDepth = virThreadPoolGetJobQueueDepth(srv->workers);
> > + if (srv->workers) {
> > + *minWorkers = virThreadPoolGetMinWorkers(srv->workers);
> > + *maxWorkers = virThreadPoolGetMaxWorkers(srv->workers);
> > + *freeWorkers = virThreadPoolGetFreeWorkers(srv->workers);
> > + *nWorkers = virThreadPoolGetCurrentWorkers(srv->workers);
> > + *nPrioWorkers = virThreadPoolGetPriorityWorkers(srv->workers);
> > + *jobQueueDepth = virThreadPoolGetJobQueueDepth(srv->workers);
> > + } else {
> > + *minWorkers = 0;
> > + *maxWorkers = 0;
> > + *freeWorkers = 0;
> > + *nWorkers = 0;
> > + *nPrioWorkers = 0;
> > + *jobQueueDepth = 0;
> > + }
> >
> > virObjectUnlock(srv);
> > return 0;
> > --
> > 2.13.6
>
> After thinking again it probably makes more sense (and the code more
> beautiful) to initialize the worker pool even for maxworker=0 (within
I don't understand why should we do that. We don't even initialize it for
libvirtd server - the implications are clear - you don't have workers, you
don't get to process a job.
> virNetServerNew) (=> we'll have to adapt virNetServerDispatchNewMessage
> as well). BTW, there is also a segmentation fault in
> virThreadPoolSetParameters… And currently it’s not possible to start
> with maxworkers set to 0 and then increase it via
Do I assume correctly that virNetServerDispatchNewMessage would allocate a new
worker if there was a request to process but the threadpool was empty? If so, I
don't see a reason to do that, why would anyone want to run with no workers?
They don't consume any resources, since they're waiting on a condition.
However, any segfaults or deadlocks must be fixed, I'll have a look at the
series as is, unless you've got a compelling reason why it's beneficial to run
with no workers at all.
Another problem/inconsistency in the current implementation is that if
you start with maxworkers=5 and then set the value to 0 via
virThreadPoolSetParameters (e.g. 'virt-admin server-threadpool-set
--min-workers 0 --max-workers 0 --priority-workers 0 libvirtd')
srv->workers still remains a non NULL value and the “thread pool memory
struct” still remains allocated and virNetServerDispatchNewMessage will
try to request a job… :)
Thanks,
Erik
--
Beste Grüße / Kind regards
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
2422
days inactive
2427
days old
15 comments
3 participants
participants (3)
-
Daniel P. Berrangé -
Erik Skultety -
Marc Hartmayer