> 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@xxxxxxxxxxxxxxxxx>
> > >
> > > 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
