[libvirt] PATCH: 18/25: Dynamic thread workers pool

[Jim Meyering]

"Daniel P. Berrange" <berrange at redhat.com> wrote:
> On Fri, Jan 16, 2009 at 12:10:35PM +0000, Richard W.M. Jones wrote:
>> On Tue, Jan 13, 2009 at 05:46:08PM +0000, Daniel P. Berrange wrote:
>> > @@ -1948,6 +2000,26 @@ static int qemudRunLoop(struct qemud_ser
>> >              }
>> >          }
>> >
>> > +        /* If number of active workers exceeds both the min_workers
>> > +         * threshold and the number of clients, then kill some
>> > +         * off */
>> > +        for (i = 0 ; (i < server->nworkers &&
>> > +                      server->nactiveworkers > server->nclients &&
>> > +                      server->nactiveworkers > min_workers) ; i++) {
>> > +
>> > +            if (server->workers[i].active &&
>> > +                !server->workers[i].processing) {
>> > +                server->workers[i].quit = 1;
>> > +
>> > +                virCondBroadcast(&server->job);
>> > +                virMutexUnlock(&server->lock);
>> > +                pthread_join(server->workers[i].thread, NULL);
>> > +                virMutexLock(&server->lock);
>> > +                server->workers[i].active = 0;
>> > +                server->nactiveworkers--;
>> > +            }
>> > +        }
>> > +
>>
>> Doesn't this cause the main loop to hang -- eg. if we happen to try to
>> kill of a worker which is doing some lengthy operation?
>
> If the worker is currently processing an RPC call, its 'processing'
> flag will be set, and thus we won't consider it a candidate for
> killing off.
>
>> > +struct qemud_worker {
>> > +    pthread_t thread;
>> > +    int active :1;
>> > +    int processing :1;
>> > +    int quit : 1;
>>
>> I guess maybe I'm unclear about the meaning of these flags.  What's
>> the difference between active & processing?
>
> I'll add comments to this struct before commiting. Their meanings
> are:
>
>      active: the thread actually exists
>  processing: the thread is currently processing an RPC call

Please consider using some name other than "active",
since it is so close in meaning to "processing".
Maybe something like "created", "live" or "alive"?

>        quit: the thread has been asked to quit
>
>
> At startup we malloc enough 'struct qemud_worker' objects to hold the
> entire 'max_workers' set, but we only start threads for 'min_workers.
> This is what the 'active' field tracks.  The 'processing' flag is there
> so that when we want to kill off a thread, we won't block waiting for
> a thread that's currently busy working. And the 'quit' flag is that
> condition that a thread checks upon waking up from its condition variable
> sleep to see if it was asked to quit.
>
> Regards,
> Daniel