From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1030295AbcHEH7D (ORCPT ); Fri, 5 Aug 2016 03:59:03 -0400 Received: from mx2.suse.de ([195.135.220.15]:58733 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1758725AbcHEH6I (ORCPT ); Fri, 5 Aug 2016 03:58:08 -0400 Message-ID: <1470383892.2094.13.camel@suse.cz> Subject: Re: [PATCH] sched/cputime: Mitigate performance regression in times()/clock_gettime() From: Giovanni Gherdovich To: Peter Zijlstra Cc: Ingo Molnar , Mike Galbraith , Stanislaw Gruszka , linux-kernel@vger.kernel.org, Mel Gorman , mgorman@techsingularity.net Date: Fri, 05 Aug 2016 09:58:12 +0200 In-Reply-To: <20160803103442.GC7141@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> <20160803100254.GE6879@twins.programming.kicks-ass.net> <20160803103442.GC7141@twins.programming.kicks-ass.net> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.16.5 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, 2016-08-03 at 12:02 +0200, Peter Zijlstra wrote: > 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. Oh! Thanks for pointing that out. > > > > 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. Ok. > > 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 > } Ok. On Wed, 2016-08-03 at 12:34 +0200, Peter Zijlstra wrote: > On Wed, Aug 03, 2016 at 12:02:54PM +0200, Peter Zijlstra wrote: > > /* > > * 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; > #else > struct sched_entity *curr = task_rq(p)->cfs_rq->curr; > #endif > > Would be the 'right' thing for !cgroup stuffs Ok, thanks. With a bit of help I figured that you actually meant to write #else struct sched_entity *curr = (&task_rq(p)->cfs)->curr; #endif Will add that. V2 is on its way; I previously said I would put "From: Mike" in the patch header, but since then I discussed with Mike and we agreed that the patch will be "From: Giovanni" and Suggested-by: Mike. Regards, Giovanni