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
<jferlan(a)redhat.com> 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 <mhartmay(a)linux.ibm.com>
>> Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
>> Reviewed-by: Bjoern Walk <bwalk(a)linux.ibm.com>
>> ---
>> 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