linux-kernel - Re: [RFC PATCH 4/5] sched/events: Introduce sched

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4162eba0-47f0-f695-42e9-66655b11a50a@arm.com>
Date:   Tue, 28 Mar 2017 16:13:45 +0200
From:   Dietmar Eggemann <dietmar.eggemann@....com>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     Ingo Molnar <mingo@...nel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Matt Fleming <matt@...eblueprint.co.uk>,
        Vincent Guittot <vincent.guittot@...aro.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Morten Rasmussen <morten.rasmussen@....com>,
        Juri Lelli <juri.lelli@....com>,
        Patrick Bellasi <patrick.bellasi@....com>
Subject: Re: [RFC PATCH 4/5] sched/events: Introduce sched_entity load
 tracking trace event

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