Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752910AbdC1OOm (ORCPT ); Tue, 28 Mar 2017 10:14:42 -0400 Received: from foss.arm.com ([217.140.101.70]:49796 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752401AbdC1OOj (ORCPT ); Tue, 28 Mar 2017 10:14:39 -0400 Subject: Re: [RFC PATCH 4/5] sched/events: Introduce sched_entity load tracking trace event To: Peter Zijlstra References: <20170328063541.12912-1-dietmar.eggemann@arm.com> <20170328063541.12912-5-dietmar.eggemann@arm.com> <20170328080824.uujenx4e6n5kwk5d@hirez.programming.kicks-ass.net> Cc: Ingo Molnar , LKML , Matt Fleming , Vincent Guittot , Steven Rostedt , Morten Rasmussen , Juri Lelli , Patrick Bellasi From: Dietmar Eggemann Message-ID: <4162eba0-47f0-f695-42e9-66655b11a50a@arm.com> Date: Tue, 28 Mar 2017 16:13:45 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: <20170328080824.uujenx4e6n5kwk5d@hirez.programming.kicks-ass.net> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 1816 Lines: 44 On 03/28/2017 10:08 AM, Peter Zijlstra wrote: > On Tue, Mar 28, 2017 at 07:35:40AM +0100, Dietmar Eggemann wrote: >> diff --git a/include/trace/events/sched.h b/include/trace/events/sched.h >> index 51db8a90e45f..647cfaf528fd 100644 >> --- a/include/trace/events/sched.h >> +++ b/include/trace/events/sched.h >> @@ -566,14 +566,15 @@ TRACE_EVENT(sched_wake_idle_without_ipi, >> #ifdef CONFIG_SMP >> #ifdef CREATE_TRACE_POINTS >> static inline >> -int __trace_sched_cpu(struct cfs_rq *cfs_rq) >> +int __trace_sched_cpu(struct cfs_rq *cfs_rq, struct sched_entity *se) >> { >> #ifdef CONFIG_FAIR_GROUP_SCHED >> - struct rq *rq = cfs_rq->rq; >> + struct rq *rq = cfs_rq ? cfs_rq->rq : NULL; >> #else >> - struct rq *rq = container_of(cfs_rq, struct rq, cfs); >> + struct rq *rq = cfs_rq ? container_of(cfs_rq, struct rq, cfs) : NULL; >> #endif >> - return cpu_of(rq); >> + return rq ? cpu_of(rq) >> + : task_cpu((container_of(se, struct task_struct, se))); >> } > > So here you duplicate lots of FAIR_GROUP internals. So why do you then > have to expose group_cfs_rq() in the previous patch? > Not having group_cfs_rq() available made the trace event code too confusing. But like I mentioned in the second to last paragraph in the cover letter, having all necessary cfs accessor-functions (rq_of(), task_of(), etc.) available would definitely streamline the coding effort of these trace events. Do you think that making them public in include/linux/sched.h is the way to go? What about the namespace issue with other sched classes? Should they be exported with the name they have right now (since cfs was there first) or should they be renamed to cfs_task_of() and rq_of_cfs_rq() etc. ? RT and Deadline class already have the own (private) accessor-functions (e.g. dl_task_of() or rq_of_dl_rq()).