From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757717AbcHCKl0 (ORCPT ); Wed, 3 Aug 2016 06:41:26 -0400 Received: from merlin.infradead.org ([205.233.59.134]:46168 "EHLO merlin.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757669AbcHCKlX (ORCPT ); Wed, 3 Aug 2016 06:41:23 -0400 Date: Wed, 3 Aug 2016 12:02:54 +0200 From: Peter Zijlstra To: Giovanni Gherdovich Cc: Ingo Molnar , Mike Galbraith , Stanislaw Gruszka , linux-kernel@vger.kernel.org, Mel Gorman , mgorman@techsingularity.net Subject: Re: [PATCH] sched/cputime: Mitigate performance regression in times()/clock_gettime() Message-ID: <20160803100254.GE6879@twins.programming.kicks-ass.net> References: <1469542034-9542-1-git-send-email-ggherdovich@suse.cz> <20160802103729.GG6862@twins.programming.kicks-ass.net> <1470175492.1849.3.camel@suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1470175492.1849.3.camel@suse.cz> User-Agent: Mutt/1.5.23.1 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Aug 03, 2016 at 12:04:52AM +0200, Giovanni Gherdovich wrote: > > > +#if defined(CONFIG_FAIR_GROUP_SCHED) > > > > This here wants a comment on why we're doing this. Because I'm sure > > that if someone were to read this code in a few weeks they'd go > > WTF!? > > I had that config variable set in the machine I was testing on, and > thought that for some reason it was related to my observations. I will > repeat the experiment without it, and if I obtain the same results I > will drop the conditional. Otherwise I will motivate its necessity. > No, I meant we want a comment here explaining the reason for these prefetches. You'll need the #ifdef because se->cfs_rq doesn't exist otherwise. > --- a/kernel/sched/core.c > +++ b/kernel/sched/core.c > @@ -2998,6 +2998,11 @@ unsigned long long task_sched_runtime(struct > task_struct *p) > * thread, breaking clock_gettime(). > */ > if (task_current(rq, p) && task_on_rq_queued(p)) { > +#if defined(CONFIG_FAIR_GROUP_SCHED) > + struct sched_entity *curr = (&p->se)->cfs_rq->curr; > + prefetch(curr); > + prefetch(&curr->exec_start); > +#endif > update_rq_clock(rq); > p->sched_class->update_curr(rq); > } > -- -- >8 -- -- >8 -- -- >8 -- -- >8 -- -- >8 -- -- >8 -- -- >8 > > I post below the snippets of generated code with and without CSE that > I got running 'disassemble /m task_sched_runtime' in gdb; you'll see > they're identical. If you prefer the explicit hint I'll include it in > v2, but it's probably safe to say it isn't needed. I much prefer the manual CSE, its much more readable. Also, maybe pull the whole thing into a helper function with a descriptive name, like: /* * XXX comment on why this is needed goes here... */ static inline void prefetch_curr_exec_start(struct task_struct *p) { #ifdef CONFIG_FAIR_GROUP_SCHED struct sched_entity *curr = (&p->se)->cfs_rq->curr; prefetch(curr); prefetch(&curr->exec_start); #endif }