Re: [PATCH v3] mm/vmalloc: Use dedicated unbound workqueues for vmap drain

[Baoquan He <bhe@redhat.com>]

On 04/01/26 at 05:47pm, Baoquan He wrote:
> On 03/31/26 at 10:23pm, Uladzislau Rezki (Sony) wrote:
> > drain_vmap_area_work() function can take >10ms to complete
> > when there are many accumulated vmap areas in a system with
> > high CPU count, causing workqueue watchdog warnings when run
> > via schedule_work():
> > 
> >   workqueue: drain_vmap_area_work hogged CPU for >10000us
> > 
> > Move the top-level drain work to a dedicated WQ_UNBOUND
> > workqueue so the scheduler can run this background work
> > on any available CPU, improving responsiveness. Use the
> > WQ_MEM_RECLAIM to ensure forward progress under memory
> > pressure.
> > 
> > Move purge helpers to separate WQ_UNBOUND | WQ_MEM_RECLAIM
> > workqueue. This allows drain_vmap_work to wait for helpers
> > completion without creating dependency on the same rescuer
> > thread and avoid a potential parent/child deadlock.
> ...snip...  
> > @@ -2385,29 +2390,31 @@ static bool __purge_vmap_area_lazy(unsigned long start, unsigned long end,
> >  		nr_purge_helpers = atomic_long_read(&vmap_lazy_nr) / lazy_max_pages();
> >  		nr_purge_helpers = clamp(nr_purge_helpers, 1U, nr_purge_nodes) - 1;
> >  
> > -		for_each_cpu(i, &purge_nodes) {
> > -			vn = &vmap_nodes[i];
> > +		for_each_vmap_node(vn) {
> > +			vn->work_queued = false;
> > +
> > +			if (list_empty(&vn->purge_list))
> > +				continue;
> >  
> >  			if (nr_purge_helpers > 0) {
> >  				INIT_WORK(&vn->purge_work, purge_vmap_node);
> > +				vn->work_queued = schedule_drain_vmap_work(
> > +					READ_ONCE(drain_vmap_helpers_wq), &vn->purge_work);
> 
> The new schedule_drain_vmap_work() could submit all purge_work on one
> CPU, do we need use queue_work_on(cpu, wq, work) instead?

Forgot the specified WQ_UNBOUND on alloc_workqueue(), sorry for the
noise. Then this patch looks great to me.

> 
> >  
> > -				if (cpumask_test_cpu(i, cpu_online_mask))
> > -					schedule_work_on(i, &vn->purge_work);
> > -				else
> > -					schedule_work(&vn->purge_work);
> > -
> > -				nr_purge_helpers--;
> > -			} else {
> > -				vn->purge_work.func = NULL;
> > -				purge_vmap_node(&vn->purge_work);
> > -				nr_purged_areas += vn->nr_purged;
> > +				if (vn->work_queued) {
> > +					nr_purge_helpers--;
> > +					continue;
> > +				}
> >  			}
> > -		}
> >  
> > -		for_each_cpu(i, &purge_nodes) {
> > -			vn = &vmap_nodes[i];
> > +			/* Sync path. Process locally. */
> > +			purge_vmap_node(&vn->purge_work);
> > +			nr_purged_areas += vn->nr_purged;
> > +		}
> >  
> > -			if (vn->purge_work.func) {
> > +		/* Wait for completion if queued any. */
> > +		for_each_vmap_node(vn) {
> > +			if (vn->work_queued) {
> >  				flush_work(&vn->purge_work);
> >  				nr_purged_areas += vn->nr_purged;
> >  			}
> ...snip...
> > +
> > +static int __init vmalloc_init_workqueue(void)
> > +{
> > +	struct workqueue_struct *drain_wq, *helpers_wq;
> 
> Maybe there's one local variable is enough like below:
> 
> 	struct workqueue_struct *wq;
> 	unsigned int flags = WQ_UNBOUND | WQ_MEM_RECLAIM;
> 
> 	wq = alloc_workqueue("vmap_drain", flags, 0);
> 	WARN_ON_ONCE(wq == NULL);
> 	WRITE_ONCE(drain_vmap_wq, wq);
> 
> 	wq = alloc_workqueue("vmap_drain_helpers", flags, 0);
> 	WARN_ON_ONCE(wq == NULL);
> 	WRITE_ONCE(drain_vmap_helpers_wq, wq);
> 
> 	return 0;
> }
> 
> Just personal preference on nitpick, not strong opionion.
>