Re: [RFC v1 10/14] kfree/rcu: Queue RCU work via queue_rcu_work_lazy()

From: Uladzislau Rezki <urezki@gmail.com>
To: Joel Fernandes <joel@joelfernandes.org>
Cc: Uladzislau Rezki <urezki@gmail.com>,
	"Paul E. McKenney" <paulmck@kernel.org>,
	rcu@vger.kernel.org, rushikesh.s.kadam@intel.com,
	neeraj.iitr10@gmail.com, frederic@kernel.org,
	rostedt@goodmis.org
Subject: Re: [RFC v1 10/14] kfree/rcu: Queue RCU work via queue_rcu_work_lazy()
Date: Sat, 14 May 2022 21:10:02 +0200	[thread overview]
Message-ID: <Yn/+il1h0hdaXzrP@pc638.lan> (raw)
In-Reply-To: <Yn+91Q3fgteCj+td@google.com>

> On Fri, May 13, 2022 at 04:55:34PM +0200, Uladzislau Rezki wrote:
> > > On Thu, May 12, 2022 at 03:04:38AM +0000, Joel Fernandes (Google) wrote:
> > > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> > > 
> > > Again, given that kfree_rcu() is doing its own laziness, is this really
> > > helping?  If so, would it instead make sense to adjust the kfree_rcu()
> > > timeouts?
> > > 
> > IMHO, this patch does not help much. Like Paul has mentioned we use
> > batching anyway.
> 
> I think that depends on the value of KFREE_DRAIN_JIFFIES. It it set to 20ms
> in the code. The batching with call_rcu_lazy() is set to 10k jiffies which is
> longer which is at least 10 seconds on a 1000HZ system. Before I added this
> patch, I was seeing more frequent queue_rcu_work() calls which were starting
> grace periods. I am not sure though how much was the power saving by
> eliminating queue_rcu_work() , I just wanted to make it go away.
> 
> Maybe, instead of this patch, can we make KFREE_DRAIN_JIFFIES a tunable or
> boot parameter so systems can set it appropriately? Or we can increase the
> default kfree_rcu() drain time considering that we do have a shrinker in case
> reclaim needs to happen.
> 
> Thoughts?
> 
Agree. We need to change behaviour of the simple KFREE_DRAIN_JIFFIES.
One thing is that we can relay on shrinker. So making the default drain
interval, say, 1 sec sounds reasonable to me and swith to shorter one
if the page is full:

diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index 222d59299a2a..89b356cee643 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -3249,6 +3249,7 @@ EXPORT_SYMBOL_GPL(call_rcu);
 
 
 /* Maximum number of jiffies to wait before draining a batch. */
+#define KFREE_DRAIN_JIFFIES_SEC (HZ)
 #define KFREE_DRAIN_JIFFIES (HZ / 50)
 #define KFREE_N_BATCHES 2
 #define FREE_N_CHANNELS 2
@@ -3685,6 +3686,20 @@ add_ptr_to_bulk_krc_lock(struct kfree_rcu_cpu **krcp,
 	return true;
 }
 
+static bool
+is_krc_page_full(struct kfree_rcu_cpu *krcp)
+{
+	int i;
+
+	// Check if a page is full either for first or second channels.
+	for (i = 0; i < FREE_N_CHANNELS && krcp->bkvhead[i]; i++) {
+		if (krcp->bkvhead[i]->nr_records == KVFREE_BULK_MAX_ENTR)
+			return true;
+	}
+
+	return false;
+}
+
 /*
  * Queue a request for lazy invocation of the appropriate free routine
  * after a grace period.  Please note that three paths are maintained,
@@ -3701,6 +3716,7 @@ void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func)
 {
 	unsigned long flags;
 	struct kfree_rcu_cpu *krcp;
+	unsigned long delay;
 	bool success;
 	void *ptr;
 
@@ -3749,7 +3765,11 @@ void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func)
 	if (rcu_scheduler_active == RCU_SCHEDULER_RUNNING &&
 	    !krcp->monitor_todo) {
 		krcp->monitor_todo = true;
-		schedule_delayed_work(&krcp->monitor_work, KFREE_DRAIN_JIFFIES);
+
+		delay = is_krc_page_full(krcp) ?
+			KFREE_DRAIN_JIFFIES:KFREE_DRAIN_JIFFIES_SEC;
+
+		schedule_delayed_work(&krcp->monitor_work, delay);
 	}
 
 unlock_return:

please note it is just for illustration because the patch is not completed.
As for parameters, i think we can add both to access over /sys/module/...

--
Uladzislau Rezki
