Re: [PATCH RFC v1 1/2] rcu/tree: Add basic support for kfree_rcu batching

[Joel Fernandes <joel@joelfernandes.org>]

On Fri, Aug 09, 2019 at 08:16:19AM -0700, Paul E. McKenney wrote:
> On Thu, Aug 08, 2019 at 07:30:14PM -0400, Joel Fernandes wrote:
[snip]
> > > But I could make it something like:
> > > 1. Letting ->head grow if ->head_free busy
> > > 2. If head_free is busy, then just queue/requeue the monitor to try again.
> > > 
> > > This would even improve performance, but will still risk going out of memory.
> > 
> > It seems I can indeed hit an out of memory condition once I changed it to
> > "letting list grow" (diff is below which applies on top of this patch) while
> > at the same time removing the schedule_timeout(2) and replacing it with
> > cond_resched() in the rcuperf test.  I think the reason is the rcuperf test
> > starves the worker threads that are executing in workqueue context after a
> > grace period and those are unable to get enough CPU time to kfree things fast
> > enough. But I am not fully sure about it and need to test/trace more to
> > figure out why this is happening.
> > 
> > If I add back the schedule_uninterruptibe_timeout(2) call, the out of memory
> > situation goes away.
> > 
> > Clearly we need to do more work on this patch.
> > 
> > In the regular kfree_rcu_no_batch() case, I don't hit this issue. I believe
> > that since the kfree is happening in softirq context in the _no_batch() case,
> > it fares better. The question then I guess is how do we run the rcu_work in a
> > higher priority context so it is not starved and runs often enough. I'll
> > trace more.
> > 
> > Perhaps I can also lower the priority of the rcuperf threads to give the
> > worker thread some more room to run and see if anything changes. But I am not
> > sure then if we're preparing the code for the real world with such
> > modifications.
> > 
> > Any thoughts?
> 
> Several!  With luck, perhaps some are useful.  ;-)
> 
> o	Increase the memory via kvm.sh "--memory 1G" or more.  The
> 	default is "--memory 500M".

Thanks, this definitely helped.

> o	Leave a CPU free to run things like the RCU grace-period kthread.
> 	You might also need to bind that kthread to that CPU.
> 
> o	Alternatively, use the "rcutree.kthread_prio=" boot parameter to
> 	boost the RCU kthreads to real-time priority.  This won't do
> 	anything for ksoftirqd, though.

I will try these as well.

> 
> o	Along with the above boot parameter, use "rcutree.use_softirq=0"
> 	to cause RCU to use kthreads instead of softirq.  (You might well
> 	find issues in priority setting as well, but might as well find
> 	them now if so!)

Doesn't think one actually reduce the priority of the core RCU work? softirq
will always have higher priority than any there. So wouldn't that have the
effect of not reclaiming things fast enough? (Or, in my case not scheduling
the rcu_work which does the reclaim).

> o	With any of the above, invoke rcu_momentary_dyntick_idle() along
> 	with cond_resched() in your kfree_rcu() loop.  This simulates
> 	a trip to userspace for nohz_full CPUs, so if this helps for
> 	non-nohz_full CPUs, adjustments to the kernel might be called for.

Ok, will try it.

Save these bullet points for future reference! ;-)  thanks,

 - Joel


> 
> Probably others, but this should do for a start.
> 
> 							Thanx, Paul
> 
> > thanks,
> > 
> >  - Joel
> > 
> > ---8<-----------------------
> > 
> > >From 098d62e5a1b84a11139236c9b1f59e7f32289b40 Mon Sep 17 00:00:00 2001
> > From: "Joel Fernandes (Google)" <joel@joelfernandes.org>
> > Date: Thu, 8 Aug 2019 16:29:58 -0400
> > Subject: [PATCH] Let list grow
> > 
> > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> > ---
> >  kernel/rcu/rcuperf.c |  2 +-
> >  kernel/rcu/tree.c    | 52 +++++++++++++++++++-------------------------
> >  2 files changed, 23 insertions(+), 31 deletions(-)
> > 
> > diff --git a/kernel/rcu/rcuperf.c b/kernel/rcu/rcuperf.c
> > index 34658760da5e..7dc831db89ae 100644
> > --- a/kernel/rcu/rcuperf.c
> > +++ b/kernel/rcu/rcuperf.c
> > @@ -654,7 +654,7 @@ kfree_perf_thread(void *arg)
> >  			}
> >  		}
> >  
> > -		schedule_timeout_uninterruptible(2);
> > +		cond_resched();
> >  	} while (!torture_must_stop() && ++l < kfree_loops);
> >  
> >  	kfree(alloc_ptrs);
> > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > index bdbd483606ce..bab77220d8ac 100644
> > --- a/kernel/rcu/tree.c
> > +++ b/kernel/rcu/tree.c
> > @@ -2595,7 +2595,7 @@ EXPORT_SYMBOL_GPL(call_rcu);
> >  
> >  
> >  /* Maximum number of jiffies to wait before draining batch */
> > -#define KFREE_DRAIN_JIFFIES 50
> > +#define KFREE_DRAIN_JIFFIES (HZ / 20)
> >  
> >  /*
> >   * Maximum number of kfree(s) to batch, if limit is hit
> > @@ -2684,27 +2684,19 @@ static void kfree_rcu_drain_unlock(struct kfree_rcu_cpu *krc,
> >  {
> >  	struct rcu_head *head, *next;
> >  
> > -	/* It is time to do bulk reclaim after grace period */
> > -	krc->monitor_todo = false;
> > +	/* It is time to do bulk reclaim after grace period. */
> >  	if (queue_kfree_rcu_work(krc)) {
> >  		spin_unlock_irqrestore(&krc->lock, flags);
> >  		return;
> >  	}
> >  
> > -	/*
> > -	 * Use non-batch regular call_rcu for kfree_rcu in case things are too
> > -	 * busy and batching of kfree_rcu could not be used.
> > -	 */
> > -	head = krc->head;
> > -	krc->head = NULL;
> > -	krc->kfree_batch_len = 0;
> > -	spin_unlock_irqrestore(&krc->lock, flags);
> > -
> > -	for (; head; head = next) {
> > -		next = head->next;
> > -		head->next = NULL;
> > -		__call_rcu(head, head->func, -1, 1);
> > +	/* Previous batch did not get free yet, let us try again soon. */
> > +	if (krc->monitor_todo == false) {
> > +		schedule_delayed_work_on(smp_processor_id(),
> > +				&krc->monitor_work,  KFREE_DRAIN_JIFFIES/4);
> > +		krc->monitor_todo = true;
> >  	}
> > +	spin_unlock_irqrestore(&krc->lock, flags);
> >  }
> >  
> >  /*
> > -- 
> > 2.23.0.rc1.153.gdeed80330f-goog
> > 
>