From: Teng Qin <qinteng@fb.com>
To: Peter Zijlstra <peterz@infradead.org>, Alexei Starovoitov <ast@fb.com>
CC: "mingo@redhat.com" <mingo@redhat.com>,
        "bgregg@netflix.com" <bgregg@netflix.com>,
        "daniel@iogearbox.net" <daniel@iogearbox.net>,
        Yonghong Song <yhs@fb.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Kernel Team <Kernel-team@fb.com>
Subject: Re: [PATCH tip 0/3] Improvements of scheduler related Tracepoints
Thread-Topic: [PATCH tip 0/3] Improvements of scheduler related Tracepoints
Thread-Index: AQHTdRkZHKMDTTHuv0OHRgDZuT83S6NDT+MAgABr+gCAAEmFAP//jqqA
Date: Fri, 15 Dec 2017 08:53:30 +0000
Message-ID: <935FB65D-395D-4122-8E40-75CA04FF2111@fb.com>
References: <20171214202044.1629279-1-qinteng@fb.com>
 <20171214204932.GH3326@worktop> <1632e487-ee65-b50d-85e5-82f42c69fea1@fb.com>
 <20171215073908.myx3wgka7qimcmsg@hirez.programming.kicks-ass.net>
In-Reply-To: <20171215073908.myx3wgka7qimcmsg@hirez.programming.kicks-ass.net>
Accept-Language: en-US
Content-Language: en-US
spamdiagnosticoutput: 1:99
spamdiagnosticmetadata: NSPM
Content-Type: text/plain; charset="utf-8"
Content-ID: <89DA444A90581242B9D61DF299E2E0E3@namprd15.prod.outlook.com>
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: ed3144f0-5085-4e0e-d666-08d5439950ba
X-MS-Exchange-CrossTenant-originalarrivaltime: 15 Dec 2017 08:53:30.4709
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 8ae927fe-1255-47a7-a2af-5f3a069daaa2
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN2PR15MB0976
X-OriginatorOrg: fb.com
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-12-15_03:,,
 signatures=0
X-Proofpoint-Spam-Reason: safe
X-FB-Internal: Safe
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Transfer-Encoding: 8bit
X-MIME-Autoconverted: from base64 to 8bit by nfs id vBF8sXfX009406
Content-Length: 2590
Lines: 54


On 12/14/17, 23:40, "Peter Zijlstra" <peterz@infradead.org> wrote:
> On Thu, Dec 14, 2017 at 07:16:00PM -0800, Alexei Starovoitov wrote:
> > On 12/14/17 12:49 PM, Peter Zijlstra wrote:
> > > On Thu, Dec 14, 2017 at 12:20:41PM -0800, Teng Qin wrote:
> > > > This set of commits attempts to improve three scheduler related
> > > > Tracepoints: sched_switch, sched_process_fork, sched_process_exit.
> > > >
> > > > Firstly, these commit add additional flag values, namely preempt,
> > > > clone_flags and group_dead to these Tracepoints, to make information
> > > > exposed via the Tracepoints more useful and complete.
> > > >
> > > > Secondly, these commits exposes task_struct pointers in these
> > > > Tracepoints. The task_struct pointers are arguments of the Tracepoints
> > > > and currently only used to compute struct field values. But for BPF
> > > > programs attached to these Tracepoints, we may want to read additional
> > > > task information via the task_struct pointers. This is currently either
> > > > impossible, or we have to make assumption of whether the Tracepoint is
> > > > running from previous / parent or next / child, and use current pointer
> > > > instead. Exposing the task_struct pointers explicitly makes such use
> > > > case easier and more reliable.
> > > >
> > >
> > > NAK
> >
> > not sure what is the concern here.
> > Is it first or second part of the above ?
>
> Definitely the second, but also the first. You know I would have ripped
> out all scheduler tracepoints if I could have. They're a pain in the
> arse.
>
> A lot of people want to add to the tracepoints, with the end result that
> they'll end up a big bloated pile of useless crap. The first part is
> just the pieces you want added.
>
> As to the second, that's complete crap; that just makes everything
> slower for bodies benefit. If you register a traceprobe you already get
> access to these things.

To have access to related task_struct is one of the main purposes of these
patches. Take sched_switch as an example. We depend on the implementation
of the Tracepoint is called from prev or next (which could, although unlikedly,
change) and use current to get that task_struct, which feels, correct
me if I'm wrong, kind of defeating the purpose of Tracepoints being more
implementation-independent than kprobes. Then we have to figure out another
Tracepoint or most likely a kprobe function to get the other (prev or next)
task_struct.

> I think your problem is that you use perf to get access to the
> tracepoints, which them means you have to do disgusting things like
> this.