On Wed, Oct 21, 2020 at 10:54:25PM -0700, Darrick J. Wong wrote:
> On Thu, Oct 22, 2020 at 04:15:31PM +1100, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@xxxxxxxxxx>
> >
> > Existing users of workqueues have bound maximum queue depths in
> > their external algorithms (e.g. prefetch counts). For parallelising
> > work that doesn't have an external bound, allow workqueues to
> > throttle incoming requests at a maximum bound. bounded workqueues
>
> Nit: capitalize the 'B' in 'bounded'.
>
> > also need to distribute work over all worker threads themselves as
> > there is no external bounding or worker function throttling
> > provided.
> >
> > Existing callers are not throttled and retain direct control of
> > worker threads, only users of the new create interface will be
> > throttled and concurrency managed.
> >
> > Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> > ---
> > libfrog/workqueue.c | 42 +++++++++++++++++++++++++++++++++++++++---
> > libfrog/workqueue.h | 4 ++++
> > 2 files changed, 43 insertions(+), 3 deletions(-)
> >
> > diff --git a/libfrog/workqueue.c b/libfrog/workqueue.c
> > index fe3de4289379..e42b2a2f678b 100644
> > --- a/libfrog/workqueue.c
> > +++ b/libfrog/workqueue.c
> > @@ -40,13 +40,21 @@ workqueue_thread(void *arg)
> > }
> >
> > /*
> > - * Dequeue work from the head of the list.
> > + * Dequeue work from the head of the list. If the queue was
> > + * full then send a wakeup if we're configured to do so.
> > */
> > assert(wq->item_count > 0);
> > + if (wq->max_queued && wq->item_count == wq->max_queued)
> > + pthread_cond_signal(&wq->queue_full);
> > +
> > wi = wq->next_item;
> > wq->next_item = wi->next;
> > wq->item_count--;
> >
> > + if (wq->max_queued && wq->next_item) {
> > + /* more work, wake up another worker */
> > + pthread_cond_signal(&wq->wakeup);
>
> Hmm. The net effect of this is that we wake up workers faster when a
> ton of work comes in, right?
Effectively. What it does is increase the concurrency of processing
only when the current worker threads cannot keep up with the
incoming work....
> And I bet none of the current workqueue
> users have suffered from this because they queue a bunch of work and
> then call workqueue_terminate, which wakes all the threads, and they
> never go to sleep again.
>
> Does it make sense to simplify the test to "if (wq->next_item) {"?
Perhaps so, but I didn't want to make subtle changes to the way the
prefetch stuff works - that's a tangled ball of string that is easy
to deadlock and really hard to debug....
> Other than that, looks good!
Ta!
CHeers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
