| Message ID | 20220512030442.2530552-11-joel@joelfernandes.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Implement call_rcu_lazy() and miscellaneous fixes | expand |
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? Thanx, Paul > --- > kernel/rcu/tree.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c > index ebdf6f7c9023..3baf29014f86 100644 > --- a/kernel/rcu/tree.c > +++ b/kernel/rcu/tree.c > @@ -3407,7 +3407,7 @@ static void kfree_rcu_monitor(struct work_struct *work) > // be that the work is in the pending state when > // channels have been detached following by each > // other. > - queue_rcu_work(system_wq, &krwp->rcu_work); > + queue_rcu_work_lazy(system_wq, &krwp->rcu_work); > } > } > > -- > 2.36.0.550.gb090851708-goog >
> 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. -- Uladzislau Rezki
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? thanks, - Joel
> 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
diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c index ebdf6f7c9023..3baf29014f86 100644 --- a/kernel/rcu/tree.c +++ b/kernel/rcu/tree.c @@ -3407,7 +3407,7 @@ static void kfree_rcu_monitor(struct work_struct *work) // be that the work is in the pending state when // channels have been detached following by each // other. - queue_rcu_work(system_wq, &krwp->rcu_work); + queue_rcu_work_lazy(system_wq, &krwp->rcu_work); } }
Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org> --- kernel/rcu/tree.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)