Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753490AbdLSTot (ORCPT ); Tue, 19 Dec 2017 14:44:49 -0500 Received: from bombadil.infradead.org ([65.50.211.133]:49803 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753182AbdLSTgw (ORCPT ); Tue, 19 Dec 2017 14:36:52 -0500 Date: Tue, 19 Dec 2017 20:36:48 +0100 From: Peter Zijlstra To: subhra mazumdar Cc: linux-kernel@vger.kernel.org, mingo@redhat.com Subject: Re: [RFC PATCH] sched: Improve scalability of select_idle_sibling using SMT balance Message-ID: <20171219193648.55oxgpbosruavlby@hirez.programming.kicks-ass.net> References: <20171208200754.4738-1-subhra.mazumdar@oracle.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20171208200754.4738-1-subhra.mazumdar@oracle.com> User-Agent: NeoMutt/20170609 (1.8.3) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2097 Lines: 81 On Fri, Dec 08, 2017 at 12:07:54PM -0800, subhra mazumdar wrote: > +static inline void > +sd_context_switch(struct sched_domain *sd, struct rq *rq, int util) > +{ > + struct sched_group *sg_cpu; > + > + /* atomically add/subtract the util */ > + sg_cpu = sd->sg_cpu; > + if (util > 0) > + atomic_inc( > + (atomic_t *)(&(sg_cpu->utilization))); > + else > + atomic_dec( > + (atomic_t *)(&(sg_cpu->utilization))); Whahah, lol, no! > +} > + > /* > * context_switch - switch to the new MM and the new thread's register state. > */ > @@ -2751,6 +2766,51 @@ context_switch(struct rq *rq, struct task_struct *prev, > struct task_struct *next, struct rq_flags *rf) > { > struct mm_struct *mm, *oldmm; > + int this_cpu = rq->cpu; > + struct sched_domain *sd; > + unsigned int cond; > + > + cond = ((prev != rq->idle) << 1) | (next != rq->idle); > + sd = rcu_dereference(per_cpu(sd_llc, this_cpu)); That one is RCU, not RCU-sched protected.. > + /* > + * From sd_llc downward update the SMT utilization. > + * Skip the lowest level 0. > + */ > + for_each_lower_domain(sd) { > + if (sd->level == 0) > + break; > + if (rq->initial_util == UTIL_UNINITIALIZED) { > + switch (cond) { > + case PREV_IDLE_NEXT_NIDLE: > + case PREV_NIDLE_NEXT_NIDLE: > + sd_context_switch(sd, rq, SMT_THREAD_UTIL); > + break; > + case PREV_NIDLE_NEXT_IDLE: > + case PREV_IDLE_NEXT_IDLE: > + break; > + } > + } else { > + switch (cond) { > + case PREV_IDLE_NEXT_NIDLE: > + sd_context_switch(sd, rq, SMT_THREAD_UTIL); > + break; > + case PREV_NIDLE_NEXT_IDLE: > + sd_context_switch(sd, rq, -SMT_THREAD_UTIL); > + break; > + case PREV_IDLE_NEXT_IDLE: > + case PREV_NIDLE_NEXT_NIDLE: > + break; > + } > + } > + } > + > + if (sd) { > + if (next == rq->idle) > + rq->initial_util = UTIL_IDLE; > + else > + rq->initial_util = UTIL_BUSY; > + } WTH do you even think this is reasonable? > prepare_task_switch(rq, prev, next); > And I still have no idea what the patch does, but I can't be bothered to reverse engineer it just now.