[libvirt] [PATCH 0/2] Fixes for segfault and deadlock
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
@srv must be unlocked for the call virNetServerProcessMsg otherwise a deadlock can occur. Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@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
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@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@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
On Fri, Jun 15, 2018 at 01:39 PM +0200, Marc Hartmayer <mhartmay@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@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@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
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@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@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@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
On Mon, Jun 18, 2018 at 09:47 AM +0200, Erik Skultety <eskultet@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@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@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@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
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@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@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@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@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
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@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@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@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@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 :|
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@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@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@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@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 :|
On Tue, Jun 19, 2018 at 05:03 PM +0200, "Daniel P. Berrangé" <berrange@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@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@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@linux.ibm.com> > Reviewed-by: Boris Fiuczynski <fiuczy@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
On Tue, Jun 19, 2018 at 05:03 PM +0200, "Daniel P. Berrangé" <berrange@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@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@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@linux.ibm.com> > Reviewed-by: Boris Fiuczynski <fiuczy@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
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@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@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@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@linux.ibm.com> > > Reviewed-by: Boris Fiuczynski <fiuczy@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 :|
On Tue, Jun 19, 2018 at 04:55 PM +0200, "Daniel P. Berrangé" <berrange@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@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@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@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@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
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@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@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@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@linux.ibm.com> > Reviewed-by: Boris Fiuczynski <fiuczy@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 :|
On Tue, Jun 19, 2018 at 05:30 PM +0200, "Daniel P. Berrangé" <berrange@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@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@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@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@linux.ibm.com> > > Reviewed-by: Boris Fiuczynski <fiuczy@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
On Mon, Jun 18, 2018 at 09:47 AM +0200, Erik Skultety <eskultet@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@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@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@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
participants (3)
-
Daniel P. Berrangé -
Erik Skultety -
Marc Hartmayer