Re: [libvirt] [PATCH v2 2/6] rpc: Initialize a worker pool for max_workers=0 as well
On 07/20/2018 03:47 AM, Marc Hartmayer wrote:
> On Thu, Jul 19, 2018 at 04:52 PM +0200, John Ferlan
> wrote:
>> On 07/03/2018 07:37 AM, Marc Hartmayer wrote:
>>> Semantically, there is no difference between an uninitialized worker
>>> pool and an initialized worker pool with zero workers. Let's allow the
>>> worker pool to be initialized for max_workers=0 as well then which
>>> makes the API more symmetric and simplifies code. Validity of the
>>> worker pool is delegated to virThreadPoolGetMaxWorkersLocked instead.
>>>
>>> This patch fixes segmentation faults in
>>> virNetServerGetThreadPoolParameters and
>>> virNetServerSetThreadPoolParameters for the case when no worker pool
>>> is actually initialized (max_workers=0).
>>>
>>> Signed-off-by: Marc Hartmayer
>>> Reviewed-by: Boris Fiuczynski
>>> Reviewed-by: Bjoern Walk
>>> ---
>>> src/libvirt_private.syms | 4 +++
>>> src/rpc/virnetserver.c | 13 -
>>> src/util/virthreadpool.c | 73
>>>
>>> src/util/virthreadpool.h | 8 ++
>>> 4 files changed, 73 insertions(+), 25 deletions(-)
>>>
>>
>> So it seems there's multiple things going on in this patch - maybe
>> they're semantically tied and I'm missing it ;-)...
>>
>> The virNetServerNew to not inhibit the call to virThreadPoolNew if
>> max_workers == 0 would seem to be easily separable with the other places
>> that would check if srv->workers == NULL before continuing.
>
> Is the code still safe if the worker count changes after getting the
> worker count?
>
> If yes, then I can split the patch if you want to.
>
I have pushed patch 1, 3-6 - so I think "order wise" we're perhaps safer
at least since there's checking w/r/t various setting when max is = 0 or
an attempt to set max = 0.
Beyond that, if you grab max workers before unlocking @srv, then I would
think you're as safe as you would be if you kept the log throughout the
top half of the if statement. We know we cannot dispose of srv and that
later on if something has called it quits nothing is going to be added
anyway.
John
>>
>> I don't understand why virNetServerDispatchNewMessage needs anything
>> more than if (virThreadPoolGetMaxWorkers(srv->workers)
>>
>> If I think back to the previous patch, then why not:
>>
>> size_t workers = 0;
>> ...
>>
>> if (srv->workers)
>> workers = virThreadPoolGetMaxWorkers(srv->workers);
>
> The reason is/was that my original code assumed it’s allowed to switch
> between zero and non-zero worker counts. My intention was to hold the
> lock for srv->workers (and therefore forbid any changes on the worker
> count) as long as needed.
>
>>
>> /* we can unlock @srv since @prog can only become invalid in case
>> * of disposing @srv, but let's grab a ref first to ensure nothing
>> * else disposes of it before we use it. */
>> virObjectRef(srv);
>> virObjectUnlock(srv);
>>
>> if (workers > 0) {
>> ...
>>
>> In the long run, I don't think it's been the desire to expose so many
>> virthreadpool.c API's - especially the locks.
>
> Yes, I don’t like it either.
>
>>
>> John
>>
>>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>>> index ffe5dfd19b11..aa496ddf8012 100644
>
> […snip]
>
> --
> 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
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 2/6] rpc: Initialize a worker pool for max_workers=0 as well
On Thu, Jul 19, 2018 at 04:52 PM +0200, John Ferlan wrote:
> On 07/03/2018 07:37 AM, Marc Hartmayer wrote:
>> Semantically, there is no difference between an uninitialized worker
>> pool and an initialized worker pool with zero workers. Let's allow the
>> worker pool to be initialized for max_workers=0 as well then which
>> makes the API more symmetric and simplifies code. Validity of the
>> worker pool is delegated to virThreadPoolGetMaxWorkersLocked instead.
>>
>> This patch fixes segmentation faults in
>> virNetServerGetThreadPoolParameters and
>> virNetServerSetThreadPoolParameters for the case when no worker pool
>> is actually initialized (max_workers=0).
>>
>> Signed-off-by: Marc Hartmayer
>> Reviewed-by: Boris Fiuczynski
>> Reviewed-by: Bjoern Walk
>> ---
>> src/libvirt_private.syms | 4 +++
>> src/rpc/virnetserver.c | 13 -
>> src/util/virthreadpool.c | 73
>>
>> src/util/virthreadpool.h | 8 ++
>> 4 files changed, 73 insertions(+), 25 deletions(-)
>>
>
> So it seems there's multiple things going on in this patch - maybe
> they're semantically tied and I'm missing it ;-)...
>
> The virNetServerNew to not inhibit the call to virThreadPoolNew if
> max_workers == 0 would seem to be easily separable with the other places
> that would check if srv->workers == NULL before continuing.
Is the code still safe if the worker count changes after getting the
worker count?
If yes, then I can split the patch if you want to.
>
> I don't understand why virNetServerDispatchNewMessage needs anything
> more than if (virThreadPoolGetMaxWorkers(srv->workers)
>
> If I think back to the previous patch, then why not:
>
> size_t workers = 0;
> ...
>
> if (srv->workers)
> workers = virThreadPoolGetMaxWorkers(srv->workers);
The reason is/was that my original code assumed it’s allowed to switch
between zero and non-zero worker counts. My intention was to hold the
lock for srv->workers (and therefore forbid any changes on the worker
count) as long as needed.
>
> /* we can unlock @srv since @prog can only become invalid in case
> * of disposing @srv, but let's grab a ref first to ensure nothing
> * else disposes of it before we use it. */
> virObjectRef(srv);
> virObjectUnlock(srv);
>
> if (workers > 0) {
> ...
>
> In the long run, I don't think it's been the desire to expose so many
> virthreadpool.c API's - especially the locks.
Yes, I don’t like it either.
>
> John
>
>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>> index ffe5dfd19b11..aa496ddf8012 100644
[…snip]
--
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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 2/6] rpc: Initialize a worker pool for max_workers=0 as well
On 07/03/2018 07:37 AM, Marc Hartmayer wrote:
> Semantically, there is no difference between an uninitialized worker
> pool and an initialized worker pool with zero workers. Let's allow the
> worker pool to be initialized for max_workers=0 as well then which
> makes the API more symmetric and simplifies code. Validity of the
> worker pool is delegated to virThreadPoolGetMaxWorkersLocked instead.
>
> This patch fixes segmentation faults in
> virNetServerGetThreadPoolParameters and
> virNetServerSetThreadPoolParameters for the case when no worker pool
> is actually initialized (max_workers=0).
>
> Signed-off-by: Marc Hartmayer
> Reviewed-by: Boris Fiuczynski
> Reviewed-by: Bjoern Walk
> ---
> src/libvirt_private.syms | 4 +++
> src/rpc/virnetserver.c | 13 -
> src/util/virthreadpool.c | 73
>
> src/util/virthreadpool.h | 8 ++
> 4 files changed, 73 insertions(+), 25 deletions(-)
>
So it seems there's multiple things going on in this patch - maybe
they're semantically tied and I'm missing it ;-)...
The virNetServerNew to not inhibit the call to virThreadPoolNew if
max_workers == 0 would seem to be easily separable with the other places
that would check if srv->workers == NULL before continuing.
I don't understand why virNetServerDispatchNewMessage needs anything
more than if (virThreadPoolGetMaxWorkers(srv->workers)
If I think back to the previous patch, then why not:
size_t workers = 0;
...
if (srv->workers)
workers = virThreadPoolGetMaxWorkers(srv->workers);
/* we can unlock @srv since @prog can only become invalid in case
* of disposing @srv, but let's grab a ref first to ensure nothing
* else disposes of it before we use it. */
virObjectRef(srv);
virObjectUnlock(srv);
if (workers > 0) {
...
In the long run, I don't think it's been the desire to expose so many
virthreadpool.c API's - especially the locks.
John
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index ffe5dfd19b11..aa496ddf8012 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -3005,11 +3005,15 @@ virThreadPoolGetCurrentWorkers;
> virThreadPoolGetFreeWorkers;
> virThreadPoolGetJobQueueDepth;
> virThreadPoolGetMaxWorkers;
> +virThreadPoolGetMaxWorkersLocked;
> virThreadPoolGetMinWorkers;
> virThreadPoolGetPriorityWorkers;
> +virThreadPoolLock;
> virThreadPoolNewFull;
> virThreadPoolSendJob;
> +virThreadPoolSendJobLocked;
> virThreadPoolSetParameters;
> +virThreadPoolUnlock;
>
>
> # util/virtime.h
> diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
> index 091e3ef23406..fdee08fee7cd 100644
> --- a/src/rpc/virnetserver.c
> +++ b/src/rpc/virnetserver.c
> @@ -203,7 +203,8 @@ virNetServerDispatchNewMessage(virNetServerClientPtr
> client,
> * of disposing @srv */
> virObjectUnlock(srv);
>
> -if (srv->workers) {
> +virThreadPoolLock(srv->workers);
> +if (virThreadPoolGetMaxWorkersLocked(srv->workers) > 0) {
> virNetServerJobPtr job;
>
> if (VIR_ALLOC(job) < 0)
> @@ -218,7 +219,7 @@ virNetServerDispatchNewMessage(virNetServerClientPtr
> client,
> }
>
> virObjectRef(client);
> -if (virThreadPoolSendJob(srv->workers, priority, job) < 0) {
> +if (virThreadPoolSendJobLocked(srv->workers, priority, job) < 0) {
> virObjectUnref(client);
> VIR_FREE(job);
> virObjectUnref(prog);
> @@ -228,10 +229,12 @@ virNetServerDispatchNewMessage(virNetServerClientPtr
> client,
> if (virNetServerProcessMsg(srv, client, prog, msg) < 0)
> goto error;
> }
> +virThreadPoolUnlock(srv->workers);
>
> return;
>
> error:
> +virThreadPoolUnlock(srv->workers);
> virNetMessageFree(msg);
> virNetServerClientClose(client);
> }
> @@ -363,8 +366,7 @@ virNetServerPtr virNetServerNew(const char *name,
> if (!(srv = virObjectLockableNew(virNetServerClass)))
> return NULL;
>
> -if (max_workers &&
> -!(srv->workers = virThreadPoolNew(min_workers, max_workers,
> +if (!(srv->workers = virThreadPoolNew(min_workers, max_workers,
>priority_workers,
>virNetServerHandleJob,
>srv)))
> @@ -575,21 +577,18 @@ virJSONValuePtr
> virNetServerPreExecRestart(virNetServerPtr srv)
> goto error;
>
> if (virJSONValueObjectAppendNumberUint(object, "min_workers",
> - srv->workers == NULL ? 0 :
>
> virThreadPoolGetMinWorkers(srv->workers)) < 0) {
> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> _("Cannot set min_workers data in JSON document"));
> goto error;
> }
> if (virJSONValueObjectAppendNumberUint(object, "max_workers",
>
[libvirt] [PATCH v2 2/6] rpc: Initialize a worker pool for max_workers=0 as well
Semantically, there is no difference between an uninitialized worker
pool and an initialized worker pool with zero workers. Let's allow the
worker pool to be initialized for max_workers=0 as well then which
makes the API more symmetric and simplifies code. Validity of the
worker pool is delegated to virThreadPoolGetMaxWorkersLocked instead.
This patch fixes segmentation faults in
virNetServerGetThreadPoolParameters and
virNetServerSetThreadPoolParameters for the case when no worker pool
is actually initialized (max_workers=0).
Signed-off-by: Marc Hartmayer
Reviewed-by: Boris Fiuczynski
Reviewed-by: Bjoern Walk
---
src/libvirt_private.syms | 4 +++
src/rpc/virnetserver.c | 13 -
src/util/virthreadpool.c | 73
src/util/virthreadpool.h | 8 ++
4 files changed, 73 insertions(+), 25 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index ffe5dfd19b11..aa496ddf8012 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -3005,11 +3005,15 @@ virThreadPoolGetCurrentWorkers;
virThreadPoolGetFreeWorkers;
virThreadPoolGetJobQueueDepth;
virThreadPoolGetMaxWorkers;
+virThreadPoolGetMaxWorkersLocked;
virThreadPoolGetMinWorkers;
virThreadPoolGetPriorityWorkers;
+virThreadPoolLock;
virThreadPoolNewFull;
virThreadPoolSendJob;
+virThreadPoolSendJobLocked;
virThreadPoolSetParameters;
+virThreadPoolUnlock;
# util/virtime.h
diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
index 091e3ef23406..fdee08fee7cd 100644
--- a/src/rpc/virnetserver.c
+++ b/src/rpc/virnetserver.c
@@ -203,7 +203,8 @@ virNetServerDispatchNewMessage(virNetServerClientPtr client,
* of disposing @srv */
virObjectUnlock(srv);
-if (srv->workers) {
+virThreadPoolLock(srv->workers);
+if (virThreadPoolGetMaxWorkersLocked(srv->workers) > 0) {
virNetServerJobPtr job;
if (VIR_ALLOC(job) < 0)
@@ -218,7 +219,7 @@ virNetServerDispatchNewMessage(virNetServerClientPtr client,
}
virObjectRef(client);
-if (virThreadPoolSendJob(srv->workers, priority, job) < 0) {
+if (virThreadPoolSendJobLocked(srv->workers, priority, job) < 0) {
virObjectUnref(client);
VIR_FREE(job);
virObjectUnref(prog);
@@ -228,10 +229,12 @@ virNetServerDispatchNewMessage(virNetServerClientPtr
client,
if (virNetServerProcessMsg(srv, client, prog, msg) < 0)
goto error;
}
+virThreadPoolUnlock(srv->workers);
return;
error:
+virThreadPoolUnlock(srv->workers);
virNetMessageFree(msg);
virNetServerClientClose(client);
}
@@ -363,8 +366,7 @@ virNetServerPtr virNetServerNew(const char *name,
if (!(srv = virObjectLockableNew(virNetServerClass)))
return NULL;
-if (max_workers &&
-!(srv->workers = virThreadPoolNew(min_workers, max_workers,
+if (!(srv->workers = virThreadPoolNew(min_workers, max_workers,
priority_workers,
virNetServerHandleJob,
srv)))
@@ -575,21 +577,18 @@ virJSONValuePtr
virNetServerPreExecRestart(virNetServerPtr srv)
goto error;
if (virJSONValueObjectAppendNumberUint(object, "min_workers",
- srv->workers == NULL ? 0 :
virThreadPoolGetMinWorkers(srv->workers)) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("Cannot set min_workers data in JSON document"));
goto error;
}
if (virJSONValueObjectAppendNumberUint(object, "max_workers",
- srv->workers == NULL ? 0 :
virThreadPoolGetMaxWorkers(srv->workers)) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("Cannot set max_workers data in JSON document"));
goto error;
}
if (virJSONValueObjectAppendNumberUint(object, "priority_workers",
- srv->workers == NULL ? 0 :
virThreadPoolGetPriorityWorkers(srv->workers)) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("Cannot set priority_workers data in JSON document"));
diff --git a/src/util/virthreadpool.c b/src/util/virthreadpool.c
index 10f2bd2c3a03..f18eafb35deb 100644
--- a/src/util/virthreadpool.c
+++ b/src/util/virthreadpool.c
@@ -318,17 +318,27 @@ size_t virThreadPoolGetMinWorkers(virThreadPoolPtr pool)
return ret;
}
-size_t virThreadPoolGetMaxWorkers(virThreadPoolPtr pool)
+
+size_t
+virThreadPoolGetMaxWorkersLocked(virThreadPoolPtr pool)
+{
+return pool->maxWorkers;
+}
+
+
+size_t
+virThreadPoolGetMaxWorkers(virThreadPoolPtr pool)
{
size_t ret;
virMutexLock(>mutex);
-ret =
