From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S965470AbaH0XdM (ORCPT ); Wed, 27 Aug 2014 19:33:12 -0400 Received: from mga02.intel.com ([134.134.136.20]:63754 "EHLO mga02.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S964814AbaH0XdL (ORCPT ); Wed, 27 Aug 2014 19:33:11 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.04,414,1406617200"; d="scan'208";a="564447259" Message-ID: <1409182369.27939.9.camel@schen9-desk2.jf.intel.com> Subject: Re: [PATCH v2] sched: Reduce contention in update_cfs_rq_blocked_load From: Tim Chen To: Jason Low Cc: Paul Turner , Peter Zijlstra , Ingo Molnar , Ben Segall , LKML Date: Wed, 27 Aug 2014 16:32:49 -0700 In-Reply-To: <1409160893.31379.24.camel@j-VirtualBox> References: <1409094682.29189.23.camel@j-VirtualBox> <1409160893.31379.24.camel@j-VirtualBox> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.8.5 (3.8.5-2.fc19) Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, 2014-08-27 at 10:34 -0700, Jason Low wrote: > On Tue, 2014-08-26 at 16:24 -0700, Paul Turner wrote: > > On Tue, Aug 26, 2014 at 4:11 PM, Jason Low wrote: > > > Based on perf profiles, the update_cfs_rq_blocked_load function constantly > > > shows up as taking up a noticeable % of system run time. This is especially > > > apparent on larger numa systems. > > > > > > Much of the contention is in __update_cfs_rq_tg_load_contrib when we're > > > updating the tg load contribution stats. However, it was noticed that the > > > values often don't get modified by much. In fact, much of the time, they > > > don't get modified at all. However, the update can always get attempted due > > > to force_update. > > > > > > In this patch, we remove the force_update in only the > > > __update_cfs_rq_tg_load_contrib. Thus the tg load contrib stats now get > > > modified only if the delta is large enough (in the current code, they get > > > updated when the delta is larger than 12.5%). This is a way to rate-limit > > > the updates while largely keeping the values accurate. > > > > > > When testing this change with AIM7 workloads, we found that it was able to > > > reduce the overhead of the function by up to a factor of 20x. > > > > Looks reasonable. > > > > > > > > Cc: Yuyang Du > > > Cc: Waiman Long > > > Cc: Mel Gorman > > > Cc: Mike Galbraith > > > Cc: Rik van Riel > > > Cc: Aswin Chandramouleeswaran > > > Cc: Chegu Vinod > > > Cc: Scott J Norton > > > Signed-off-by: Jason Low > > > --- > > > kernel/sched/fair.c | 10 ++++------ > > > 1 files changed, 4 insertions(+), 6 deletions(-) > > > > > > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c > > > index fea7d33..7a6e18b 100644 > > > --- a/kernel/sched/fair.c > > > +++ b/kernel/sched/fair.c > > > @@ -2352,8 +2352,7 @@ static inline u64 __synchronize_entity_decay(struct sched_entity *se) > > > } > > > > > > #ifdef CONFIG_FAIR_GROUP_SCHED > > > -static inline void __update_cfs_rq_tg_load_contrib(struct cfs_rq *cfs_rq, > > > - int force_update) > > > +static inline void __update_cfs_rq_tg_load_contrib(struct cfs_rq *cfs_rq) > > > { > > > struct task_group *tg = cfs_rq->tg; > > > long tg_contrib; > > > @@ -2361,7 +2360,7 @@ static inline void __update_cfs_rq_tg_load_contrib(struct cfs_rq *cfs_rq, > > > tg_contrib = cfs_rq->runnable_load_avg + cfs_rq->blocked_load_avg; > > > tg_contrib -= cfs_rq->tg_load_contrib; > > > > > > - if (force_update || abs(tg_contrib) > cfs_rq->tg_load_contrib / 8) { > > > > Another option with slightly higher accuracy would be to increase the > > sensitivity here when force_update == 1. > > > > E.g.: > > abs(tg_contrib) > cfs_rq->tg_load_contrib / (8 * (1 + force_update))) { ... > > > > Alternatively we could bound total inaccuracy: > > int divisor = force_update ? NR_CPUS : 8; > > if (abs(tg_contrib) > cfs_rq->tg_load_contrib / divisor) { ... > > > > > > [ And probably rename force_update to want_update ] > > Out of the 2 additional options, I think the first one is better. The > other option of using NR_CPUS looks like we're increasing the update > rate as the system gets larger, and its the larger systems that are > typically more affected by the contention. Probably num_present_cpus() will be better than NR_CPUS, which can be much larger than the actual cpus present. > > Do you prefer either of the 2 other options over this v2 patch? If so, I > can test and send out a new patch, otherwise, I'll keep this current v2 > patch. If there are multiple non-forced updates, option 1's error seems to accumulate and non-bounded as we do not actually update? Is this a concern? Thanks. Tim