linux-kernel - Re: [PATCH] Sched: Add a tracepoint to track rq->nr

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]

Date:   Wed, 24 Jun 2020 13:10:21 +0100
From:   Qais Yousef <qais.yousef@....com>
To:     Phil Auld <pauld@...hat.com>
Cc:     linux-kernel@...r.kernel.org, Ingo Molnar <mingo@...hat.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Vincent Guittot <vincent.guittot@...aro.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Juri Lelli <juri.lelli@...hat.com>,
        Mel Gorman <mgorman@...e.de>
Subject: Re: [PATCH] Sched: Add a tracepoint to track rq->nr_running

Hi Phil

On 06/23/20 15:38, Phil Auld wrote:

[...]

> > This is a very specific call site, so I guess it looks fine to pass very
> > specific info too.
> > 
> > But I think we can do better by just passing struct rq and add a new helper
> > sched_trace_rq_nr_running() (see the bottom of fair.c for a similar helper
> > functions for tracepoints).
> > 
> > This will allow the user to extract, cpu, nr_running and potentially other info
> > while only pass a single argument to the tracepoint. Potentially extending its
> > future usefulness.
> 
> I can certainly add a sched_trace_rq_nr_running helper and pass the *rq if
> you think that is really important. 

As I said, this is a very specific call site, so passing specific info should
be fine, so not really important.

My general view on this (which is influenced by what Peter asked for when we
first introduced this) is that it's better to allow a trace point to
extract more signals from this specific call site by passing generic info and
let the event code/module do what it wants.

But the idea behind these tracepoints is that they can evolve when they need
to. So I don't think we should hung up on this if it makes things unnecessarily
complex.

> 
> I'd prefer to keep the count field though as that is the only way to tell
> if this is an add_nr_running or sub_nr_running from looking at a single
> trace event.

Passing the count field is fine by me...

> 
> I could make it two different tracepoints.  Would that be better? To me that
> seemed more complicated though. The tooling would need to look at it
> different events and there would be more kernel change.

... but splitting the tracepoint doesn't look pretty.

If passing the rq and the count is enough for you, I'd vote this is better. If
not, then I won't insist into twisting things too much for the sake of it.

Thanks

--
Qais Yousef