Re: [PATCH] thread-pool: Allow at least 1 thread in thread_pool_adjust_max_threads_to_work()

[Peter Xu <peterx@redhat.com>]

On Tue, May 26, 2026 at 11:06:58PM +0200, Maciej S. Szmigiero wrote:
> On 26.05.2026 22:33, Peter Xu wrote:
> > On Thu, May 21, 2026 at 09:06:07PM +0200, Maciej S. Szmigiero wrote:
> > > From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>
> > > 
> > > thread_pool_adjust_max_threads_to_work() is supposed to give each task its
> > > own thread by setting the pool max thread count limit accordingly.
> > > 
> > > However, if there aren't any tasks currently in the pool the pool max
> > > thread count will be set to 0, which will trigger an assertion failure
> > > in thread_pool_set_max_threads() - because setting this value would
> > > completely block the pool by not allowing it to process any submitted
> > > tasks.
> > > 
> > > This also can happen if a task is submitted via
> > > thread_pool_submit_immediate() to an empty pool but the task completes so
> > > quickly that by the time this function calls
> > > thread_pool_adjust_max_threads_to_work() the pool again has no unfinished
> > > tasks in it.
> > 
> > Sorry for a late comment. Just curious: how easy is this to reproduce?
> 
> It's difficult to reproduce in most setups.
> 
> My main VFIO live migration setup never hit it for more than a year, other
> similar setup hit it recently 3 times.
> 
> On the other hand, putting sleep(5) in the middle of
> thread_pool_submit_immediate() makes it reproduce nearly always for me.

Thanks, I'll attach this info to the patch when queuing.

> 
> > > 
> > > Fix this by making sure that the pool is allowed to create at least 1
> > > thread.
> > 
> > But then it means we have no work and then we will create one thread does
> > nothing..
> 
> thread_pool_adjust_max_threads_to_work() is currently called only from
> thread_pool_submit_immediate().
> 
> If the API user truly wants no threads in the pool for time being
> (even though this will completely block the pool) they can use
> thread_pool_submit() to submit their task(s).
> This won't call thread_pool_adjust_max_threads_to_work() by itself.
> 
> Also, since the thread pool is created with zero initial threads by
> default the patch does *not* mean that the each newly created pool
> will have one idle thread now.

This is another small problem.  I think the ideal case is we create threads
before hand, not on-demand here.  It should be faster. I don't know how
much, maybe it's not measurable, but IIUC that's one good thing about
exclusive thread pool that we didn't really leverage.

I think the system on both sides should know exactly how many threads we
need, hence they can create them before downtime starts.  During the
process, we shouldn't need to adjust num of threads, also avoid the bug
like this.  But I'll leave that to you to decide if it's worthwhile.

> 
> > 
> > I suspect the real culprit is we released the cur_work_lock during the
> > whole process of thread_pool_submit_immediate().  If we take it during the
> > whole window this will be a no-issue too.
> 
> True, however this will need splitting both thread_pool_submit() and
> thread_pool_adjust_max_threads_to_work() into two versions each: one
> which takes pool->cur_work_lock, another which does not and then
> take pool->cur_work_lock on their behalf in thread_pool_submit_immediate().
> 
> Not to mention this would also mean putting the whole g_thread_pool_push()
> -> g_thread_pool_start_thread() machinery under pool->cur_work_lock in this
> case instead of just pool->cur_work increment.
> 
> I think doing this such way would add unnecessary complexity considering
> that its alternative of this patch is a single-line trivial change.
> 
> > The other question is, if it is awkward to manually adjust num of threads,
> > shall we set num to be -1 (unlimited) while pool created?
> 
> We can't - Glib thread pool API does not allow unlimited exclusive pools.

If we don't care about pre-init of thread pools, I'm actually not sure if
we need exclusive pool at all.. maybe it'll still help, to e.g. inherit
scheduler setup of migration thread.

But it's ok anyway, I'll queue this patch for now.

Thanks,

-- 
Peter Xu