On Wed, Feb 26, 2025 at 11:59:53AM +0100, Vlastimil Babka wrote:
> On 2/25/25 7:21 PM, Uladzislau Rezki wrote:
> >>
> > WQ_MEM_RECLAIM-patch fixes this for me:
>
> Sounds good, can you send a formal patch then?
>
Do you mean both? Test case and fix? I can :)
> Some nits below:
>
> > <snip>
> > diff --git a/mm/slab_common.c b/mm/slab_common.c
> > index 4030907b6b7d..1b5ed5512782 100644
> > --- a/mm/slab_common.c
> > +++ b/mm/slab_common.c
> > @@ -1304,6 +1304,8 @@ module_param(rcu_min_cached_objs, int, 0444);
> > static int rcu_delay_page_cache_fill_msec = 5000;
> > module_param(rcu_delay_page_cache_fill_msec, int, 0444);
> >
> > +static struct workqueue_struct *rcu_reclaim_wq;
> > +
> > /* Maximum number of jiffies to wait before draining a batch. */
> > #define KFREE_DRAIN_JIFFIES (5 * HZ)
> > #define KFREE_N_BATCHES 2
> > @@ -1632,10 +1634,10 @@ __schedule_delayed_monitor_work(struct kfree_rcu_cpu *krcp)
> > if (delayed_work_pending(&krcp->monitor_work)) {
> > delay_left = krcp->monitor_work.timer.expires - jiffies;
> > if (delay < delay_left)
> > - mod_delayed_work(system_unbound_wq, &krcp->monitor_work, delay);
> > + mod_delayed_work(rcu_reclaim_wq, &krcp->monitor_work, delay);
> > return;
> > }
> > - queue_delayed_work(system_unbound_wq, &krcp->monitor_work, delay);
> > + queue_delayed_work(rcu_reclaim_wq, &krcp->monitor_work, delay);
> > }
> >
> > static void
> > @@ -1733,7 +1735,7 @@ kvfree_rcu_queue_batch(struct kfree_rcu_cpu *krcp)
> > // "free channels", the batch can handle. Break
> > // the loop since it is done with this CPU thus
> > // queuing an RCU work is _always_ success here.
> > - queued = queue_rcu_work(system_unbound_wq, &krwp->rcu_work);
> > + queued = queue_rcu_work(rcu_reclaim_wq, &krwp->rcu_work);
> > WARN_ON_ONCE(!queued);
> > break;
> > }
> > @@ -1883,7 +1885,7 @@ run_page_cache_worker(struct kfree_rcu_cpu *krcp)
> > if (rcu_scheduler_active == RCU_SCHEDULER_RUNNING &&
> > !atomic_xchg(&krcp->work_in_progress, 1)) {
> > if (atomic_read(&krcp->backoff_page_cache_fill)) {
> > - queue_delayed_work(system_unbound_wq,
> > + queue_delayed_work(rcu_reclaim_wq,
> > &krcp->page_cache_work,
> > msecs_to_jiffies(rcu_delay_page_cache_fill_msec));
> > } else {
> > @@ -2120,6 +2122,10 @@ void __init kvfree_rcu_init(void)
> > int i, j;
> > struct shrinker *kfree_rcu_shrinker;
> >
> > + rcu_reclaim_wq = alloc_workqueue("rcu_reclaim",
>
> Should we name it "kvfree_rcu_reclaim"? rcu_reclaim sounds too generic
> as if it's part of rcu itself?
>
> > + WQ_UNBOUND | WQ_MEM_RECLAIM, 0);
>
> Do we want WQ_SYSFS? Or maybe only when someone asks, with a use case?
>
If someone asks, IMO.
--
Uladzislau Rezki
