From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1752910AbdC1OOm (ORCPT <rfc822;w@1wt.eu>);
        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 <rfc822;linux-kernel@vger.kernel.org>);
        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 <peterz@infradead.org>
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 <mingo@kernel.org>, LKML <linux-kernel@vger.kernel.org>,
        Matt Fleming <matt@codeblueprint.co.uk>,
        Vincent Guittot <vincent.guittot@linaro.org>,
        Steven Rostedt <rostedt@goodmis.org>,
        Morten Rasmussen <morten.rasmussen@arm.com>,
        Juri Lelli <juri.lelli@arm.com>,
        Patrick Bellasi <patrick.bellasi@arm.com>
From: Dietmar Eggemann <dietmar.eggemann@arm.com>
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: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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()).