Re: [libvirt] [PATCH 2/2] rpc: Fix segmentation fault when no worker pool is available

2018-06-20 Thread Daniel P . Berrangé 
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é" 
>  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 
> >> > >  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 
> >> > > >>  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 
> >> > > >> > Reviewed-by: Boris Fiuczynski 
> >> > > >> > ---
> >> > > >> >  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 :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 2/2] rpc: Fix segmentation fault when no worker pool is available

2018-06-19 Thread Marc Hartmayer 
On Tue, Jun 19, 2018 at 05:30 PM +0200, "Daniel P. Berrangé" 
 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é" 
>>  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 
>> >> >  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 
>> >> > >>  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 
>> >> > >> > Reviewed-by: Boris Fiuczynski 
>> >> > >> > ---
>> >> > >> >  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-

Re: [libvirt] [PATCH 2/2] rpc: Fix segmentation fault when no worker pool is available

2018-06-19 Thread Daniel P . Berrangé 
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é" 
>  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 
> >> >  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 
> >> > >>  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 
> >> > >> > Reviewed-by: Boris Fiuczynski 
> >> > >> > ---
> >> > >> >  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 :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 2/2] rpc: Fix segmentation fault when no worker pool is available

2018-06-19 Thread Marc Hartmayer 
On Tue, Jun 19, 2018 at 05:03 PM +0200, "Daniel P. Berrangé" 
 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 
>> > >  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 
>> > > >>  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 
>> > > >> > Reviewed-by: Boris Fiuczynski 
>> > > >> > ---
>> > > >> >  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


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 2/2] rpc: Fix segmentation fault when no worker pool is available

2018-06-19 Thread Marc Hartmayer 
On Tue, Jun 19, 2018 at 04:55 PM +0200, "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 
>> >  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 
>> > >>  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 
>> > >> > Reviewed-by: Boris Fiuczynski 
>> > >> > ---
>> > >> >  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


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 2/2] rpc: Fix segmentation fault when no worker pool is available

2018-06-19 Thread Daniel P . Berrangé 
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 
> > >  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 
> > > >>  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 
> > > >> > Reviewed-by: Boris Fiuczynski 
> > > >> > ---
> > > >> >  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 :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 2/2] rpc: Fix segmentation fault when no worker pool is available

2018-06-19 Thread Daniel P . Berrangé 
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  
> > 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 
> > >>  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 
> > >> > Reviewed-by: Boris Fiuczynski 
> > >> > ---
> > >> >  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 :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 2/2] rpc: Fix segmentation fault when no worker pool is available

2018-06-19 Thread Erik Skultety 
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  
> 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 
> >>  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 
> >> > Reviewed-by: Boris Fiuczynski 
> >> > ---
> >> >  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

Re: [libvirt] [PATCH 2/2] rpc: Fix segmentation fault when no worker pool is available

2018-06-18 Thread Marc Hartmayer 
On Mon, Jun 18, 2018 at 09:47 AM +0200, Erik Skultety  
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 
>>  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 
>> > Reviewed-by: Boris Fiuczynski 
>> > ---
>> >  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


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 2/2] rpc: Fix segmentation fault when no worker pool is available

2018-06-18 Thread Marc Hartmayer 
On Mon, Jun 18, 2018 at 09:47 AM +0200, Erik Skultety  
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 
>>  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 
>> > Reviewed-by: Boris Fiuczynski 
>> > ---
>> >  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


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 2/2] rpc: Fix segmentation fault when no worker pool is available

2018-06-18 Thread Erik Skultety 
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 
>  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 
> > Reviewed-by: Boris Fiuczynski 
> > ---
> >  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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 2/2] rpc: Fix segmentation fault when no worker pool is available

2018-06-15 Thread Marc Hartmayer 
On Fri, Jun 15, 2018 at 01:39 PM +0200, Marc Hartmayer  
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 
> Reviewed-by: Boris Fiuczynski 
> ---
>  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


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 2/2] rpc: Fix segmentation fault when no worker pool is available

2018-06-15 Thread Marc Hartmayer 
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 
Reviewed-by: Boris Fiuczynski 
---
 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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list