From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758010AbcAYULs (ORCPT ); Mon, 25 Jan 2016 15:11:48 -0500 Received: from outbound-smtp08.blacknight.com ([46.22.139.13]:55541 "EHLO outbound-smtp08.blacknight.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757166AbcAYULq (ORCPT ); Mon, 25 Jan 2016 15:11:46 -0500 Date: Mon, 25 Jan 2016 20:11:42 +0000 From: Mel Gorman To: Ingo Molnar Cc: Peter Zijlstra , Matt Fleming , Mike Galbraith , LKML Subject: Re: [PATCH] sched: Make schedstats a runtime tunable that is disabled by default Message-ID: <20160125201142.GJ3162@techsingularity.net> References: <1453716331-21029-1-git-send-email-mgorman@techsingularity.net> <20160125112606.GV6357@twins.programming.kicks-ass.net> <20160125133944.GE3162@techsingularity.net> <20160125145944.GZ6357@twins.programming.kicks-ass.net> <20160125154635.GB4031@gmail.com> <20160125170716.GI3162@techsingularity.net> <20160125184000.GA6249@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-15 Content-Disposition: inline In-Reply-To: <20160125184000.GA6249@gmail.com> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Jan 25, 2016 at 07:40:00PM +0100, Ingo Molnar wrote: > > * Mel Gorman wrote: > > > On Mon, Jan 25, 2016 at 04:46:35PM +0100, Ingo Molnar wrote: > > > > Of course, it'll be our luck that tracking the data for these > > > > tracepoints is the most expensive part of schedstats ... > > > > > > > > Ingo? > > > > > > IIRC it needed only a small subset of schedstats to make those tracepoints work. > > > > > > We already have too much overhead in the scheduler as-is - and the extra cache > > > footprint does not even show on the typically cache-rich enterprise CPUs most of > > > the scalability testing goes on. > > > > > > My minimum requirement for such runtime enablement would be to make it entirely > > > static-branch patched and triggered at the call sites as well - not hidden inside > > > schedstat functions. > > > > > > > As it is, it's static-branch patched but I'm struggling to see why they cannot > > be hidden in the schedstat_* functions which are just preprocessor macros. The > > checks could be put in the callsites but it's a lot of updates and I don't think > > the end result would be very nice to read. > > So I was judging by: > > @@ -755,7 +755,12 @@ static void > update_stats_wait_end(struct cfs_rq *cfs_rq, struct sched_entity *se) > { > struct task_struct *p; > - u64 delta = rq_clock(rq_of(cfs_rq)) - se->statistics.wait_start; > + u64 delta; > + > + if (static_branch_unlikely(&sched_schedstats)) > + return; > + > > which puts a static branch inside a real function, not preprocessor macros. > Ok, I see your point, thanks. The vast majority of the checks are in the schedstat_[inc|dec|set) helpers. update_stats_wait_end() is an oddity in that it's a large function that only exists in schedstat and does a number of calculations. Ideally update_stats_wait would be updated too. In the next revision, I'll create a schedstats_enabled() helper that is an alias of static_branch_unlikely() and returns 0 if stats are not configured in for use at the call-site of large functions like this. That has the added benefit of avoiding any function call overhead. I'm less concerned with the exact coding style at the moment. I'm more concerned about whether people are ok with the dead tracepoints dead and hidden stats when schedstat is disabled at runtime by default. -- Mel Gorman SUSE Labs